-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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
[SPARK-28624][SQL][TESTS] Run date.sql via Thrift Server #28721
Conversation
Test build #123508 has finished for PR 28721 at commit
|
retest this please |
Looks nice if the tests passed. |
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 <max.gekk@gmail.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>
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.sql
and 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: