-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-3407][SQL]Add Date type support #2344
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
Conversation
|
QA tests have started for PR 2344 at commit
|
|
QA tests have finished for PR 2344 at commit
|
|
QA tests have started for PR 2344 at commit
|
|
QA tests have finished for PR 2344 at commit
|
|
Ha, first pass in recent days! |
.gitignore
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are considering removing the need for this special config file in #2263 so maybe best to leave this out.
|
This looks pretty awesome! I'd like to do a quick pass and make sure there aren't any missing places, but overall this looks complete to me. Is there a reason the PR title still says WIP? |
|
The last thing here is to enable
|
|
QA tests have started for PR 2344 at commit
|
|
QA tests have finished for PR 2344 at commit
|
|
QA tests have started for PR 2344 at commit
|
|
QA tests have finished for PR 2344 at commit
|
|
Just rebase code, the failure is in spark-core and not in this patch. |
|
test this please |
|
QA tests have started for PR 2344 at commit
|
|
QA tests have finished for PR 2344 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't check this in Hive, but probably it's better to convert the Date => Timestamp, since Timestamp is more precise. The same for the rule In .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get the same result from hive, seems we have to change everything to string here.
|
A few comments on returns null V.S. raise exception in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using .getTime to do the comparison would be much more efficient. When caching a DateType column, this function is really a critical path.
|
Left some minor comments, otherwise LGTM. Also, do we want to add Python API in this PR or a separate one? |
|
I can see most of the reviews are focused on comparing and ordering. I'd like to fix those comparing rules in a separated PR. I tested what you declared here the other day, that was also different from what Hive did. Also Python API is can be a separated PR. |
|
QA tests have started for PR 2344 at commit
|
|
QA tests have started for PR 2344 at commit
|
|
QA tests have started for PR 2344 at commit
|
|
QA tests have finished for PR 2344 at commit
|
|
Test FAILed. |
|
QA tests have finished for PR 2344 at commit
|
|
Test FAILed. |
|
Seems we are getting something wrong with orc in jenkins... My local test is fine. |
|
I guess this one is related: 411cf29 |
|
QA tests have finished for PR 2344 at commit
|
|
Test FAILed. |
|
retest this please. |
|
QA tests have started for PR 2344 at commit
|
|
QA tests have finished for PR 2344 at commit
|
|
Test FAILed. |
|
retest this please. |
|
QA tests have started for PR 2344 at commit
|
|
QA tests have finished for PR 2344 at commit
|
|
Test PASSed. |
|
Thanks for working on this huge feature! Merged to master. |
No description provided.