-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-28624][SQL][TESTS] Run date.sql via Thrift Server #28721
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
|
Test build #123508 has finished for PR 28721 at commit
|
|
retest this please |
|
Looks nice if the tests passed. |
HyukjinKwon
left a comment
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.
LGTM too if tests pass
|
Test build #123512 has finished for PR 28721 at commit
|
|
This fix can be merged into branch-3.0? I'm not really sure that |
|
Let me merge it to master first. From reading the JIRA, seems it's fixed only in the master. |
|
Merged to master. |
### What changes were proposed in this pull request? Enable `date.sql` and run it via Thrift Server in `ThriftServerQueryTestSuite`. ### Why are the changes needed? To improve test coverage. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By running the enabled tests via: ``` $ build/sbt -Phive-thriftserver "hive-thriftserver/test-only *ThriftServerQueryTestSuite -- -z date.sql" ``` Closes apache#28721 from MaxGekk/enable-date.sql-for-thrift. Authored-by: Max Gekk <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>
|
I had an offline discussion with @MaxGekk and @cloud-fan. Per #28723 (comment), I will revert this too. Again, technically it was fine to merge in a way because the skipped tests passed. I am here reverting this rather for the management purpose - the JIRA isn't resolved yet because we need to discuss if the result is correct or not. |
What changes were proposed in this pull request?
Enable
date.sqland run it via Thrift Server inThriftServerQueryTestSuite.Why are the changes needed?
To improve test coverage.
Does this PR introduce any user-facing change?
No
How was this patch tested?
By running the enabled tests via: