Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend extract function to support DateTime data types #227

Closed
wants to merge 3 commits into from

Conversation

mehant
Copy link
Contributor

@mehant mehant commented Apr 2, 2014

Currently extract() function only supports extracting time units from an Interval data type. This patch extends the extract() function to support DateTime.

In StandardConvertletTable() we reinterpret the extract() function to be a '/' expression based on the input data types. The problem is for Drill we cannot rewrite the extract() function this early because the data types are still 'ANY' and we cannot determine the multiplier to be used in the '/' expression while rewriting it.

Drill will extend the StandardConvertletTable() and override the convertExtract() method to simply treat it like a regular function and not rewrite it.

mehant added 2 commits April 1, 2014 17:25
Add new getPlanner() methods in Frameworks.java to accept a SqlRexConvertletTable
Raise an exception in StandardConvertletTable if extracting from a DATETTIME data type.
@julianhyde
Copy link
Owner

Can you add some tests to SqlOperatorBaseTest (maybe add a method below testExtractFunc). Then we can see that this function produces the right results.

}

/** Piece of code to be run in a context where a planner is available. The
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indent

@mehant
Copy link
Contributor Author

mehant commented Apr 2, 2014

"Can you add some tests to SqlOperatorBaseTest (maybe add a method below testExtractFunc). Then we can see that this function produces the right results."

Did you want me to add negative tests, because StandardConvertletTable.convertExtract() will currently throw an exception if the second input is a DATETIME type. Or did you want me to modify StandardConvertletTable, because the current multipliers in convertExtract() wouldn't work with DATETIME types.

@julianhyde
Copy link
Owner

Can you write a test. If you can't easily get it to pass then you can add @ignore.

In the long run, it will benefit you Drill developers, when you get round to calling SqlOperatorTest on your own operator implementations.

Add test to SqlOperatorBaseTest.
Use a singleton in StandardConvertletTable.
Minor cleanup
Remove newly added constructor in PlannerImpl
Remove newly added getPlanner method in Frameworks.
@mehant
Copy link
Contributor Author

mehant commented Apr 2, 2014

Thanks for the quick reply. I've added tests to SqlOperatorBaseTest. convertExtract() function in StandardConvertletTable will need to be enhanced to handle DATETIME types. I can create a separate issue to address this and work on it.

Made the other changes suggested by you.

@julianhyde julianhyde closed this in d897d8d Apr 3, 2014
ldming pushed a commit to ldming/mycalcite that referenced this pull request Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants