-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-28527][SQL][TEST] Re-run all the tests in SQLQueryTestSuite via Thrift Server #25567
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
|
Please note that this PR need to test with |
|
Test build #109639 has finished for PR 25567 at commit
|
|
retest this please |
|
Test build #109641 has finished for PR 25567 at commit
|
|
retest this please |
|
So, only |
|
Test build #109657 has finished for PR 25567 at commit
|
|
Yes. It works. But |
org.apache.spark.sql.catalyst.parser.ParseException:
no viable alternative at input 'left'(line 1, pos 10)
== SQL ==
select i, left('ahoj', i), right('ahoj', i) from range(-5, 6) t(i) order by i
----------^^^
|
Test build #109665 has finished for PR 25567 at commit
|
|
retest this please |
|
Test build #109666 has finished for PR 25567 at commit
|
|
retest this please |
|
Test build #109672 has finished for PR 25567 at commit
|
|
Oh! @wangyum sorry I didn't know you opened a PR here. |
|
Test build #109702 has finished for PR 25567 at commit
|
|
retest this please |
|
Test build #109710 has finished for PR 25567 at commit
|
|
|
retest this please |
|
Test build #109715 has finished for PR 25567 at commit
|
|
|
retest this please |
|
Test build #109718 has finished for PR 25567 at commit
|
|
retest this please |
|
Test build #109722 has finished for PR 25567 at commit
|
|
Merged to master. |
|
Hi, @wangyum and @HyukjinKwon .
|
|
Could we ignore this test on jenkins? |
|
I plan to ignore this test: #25592 We can run it manually. |
|
Since I already reverted the first trial of your this PR, I'm honestly reluctant to revert this by myself again. Let me ping PMCs since we need to fix this in anyway urgently. Hi, @gatorsmile , @cloud-fan , @HyukjinKwon , @srowen . Could you make a decision to this? @wangyum is suggesting to add |
|
I think we can ignore them for now. Looks @wangyum going to fix the flakiness. |
|
I am going to merge #25592. If we prefer to revert them, we can revert both - I don't mind. |
| (1 to 3).foldLeft(Try(startThriftServer(listeningPort, 0))) { case (started, attempt) => | ||
| started.orElse { | ||
| listeningPort += 1 | ||
| Try(startThriftServer(listeningPort, attempt)) |
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.
@wangyum
I think this is the cause of flakiness.
This goes down to https://github.com/apache/spark/blob/master/sql/hive-thriftserver/v1.2.1/src/main/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java#L178, which starts the service in an asynchronous background thread. It can race and try to connect in the test before the server actually gets initialized.
Moving it to beforeAll() and adding a few seconds sleep should help.
Or we could transplant the whole way from HiveThriftServer2Test that starts uses the start-thriftserver.sh script and monitors it's log until its started. Would it be possible to make HiveThriftServer2Test a trait that can be mixed in here?
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.
Thank you so much for the investigation, @juliuszsompolski !
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.
Move it to beforeAll() also unstable: #25567 (comment)
Anyway, I will try to move it to beforeAll() again.
| * 2. Support DESC command. | ||
| * 3. Support SHOW command. | ||
| */ | ||
| class ThriftServerQueryTestSuite extends SQLQueryTestSuite { |
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 am trying to troubleshoot an jenkins build failure on this this, but the regular command which I use to run SQLQueryTestSuite
SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/test-only *ThriftServerQueryTestSuite -- -z interval.sql"
Could someone help me to run ThriftServerQueryTestSuite?
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.
@wangyum, can you write some guides in the documentation about how we run each tests?
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.
What changes were proposed in this pull request?
This PR build a test framework that directly re-run all the tests in
SQLQueryTestSuitevia Thrift Server. But it's a little different fromSQLQueryTestSuite:DESCcommand andSHOWcommand becauseSQLQueryTestSuiteformatted the output.When building this framework, found two bug:
SPARK-28624:
make_dateis inconsistent when reading from tableSPARK-28611: Histogram's height is different
found two features that ThriftServer can not support:
SPARK-28636: ThriftServer can not support decimal type with negative scale
SPARK-28637: ThriftServer can not support interval type
Also, found two inconsistent behavior:
SPARK-28620: Double type returned for float type in Beeline/JDBC
SPARK-28619: The golden result file is different when tested by
bin/spark-sqlWhy are the changes needed?
Improve the overall test coverage for Thrift Server.
Does this PR introduce any user-facing change?
No
How was this patch tested?
N/A