-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-37492][SQL] Optimize Orc test code with withAllNativeOrcReaders #34723
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
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #145669 has finished for PR 34723 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.
Sorry, but hold on please, @beliefer .
Although this PR itself is correct, the previous commit seems to misleading.
withAllOrcReaders is not correct in Apache Spark context because this only tests native OrcReaders. We have hive ORC reader too.
|
Let's proceed this after #34733 . |
I see. |
|
BTW, is this PR include all instances like this, @beliefer ? Could you check other places too? |
8c464a0 to
96bb4f5
Compare
Yes. I checked every places. |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
dongjoon-hyun
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.
| SQLConf.ORC_VECTORIZED_READER_ENABLED.key -> vectorized) { | ||
| withSQLConf( | ||
| SQLConf.ORC_IMPLEMENTATION.key -> orcImpl) { | ||
| withAllNativeOrcReaders { |
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.
Ur, @beliefer . This is technically correct, but logically misleading again. When orcImpl=hive, we are not testing nativeOrcReaders. The original code is more natural.
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'll merge this PR without this part.
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.
@dongjoon-hyun Thank you for the reminder.
|
Test build #145738 has finished for PR 34723 at commit
|
What changes were proposed in this pull request?
This PR used to optimize some Orc test code in OrcSourceSuite.scala
Why are the changes needed?
Improve code.
Does this PR introduce any user-facing change?
'No'. Just improve code for UT.
How was this patch tested?
Jenkins test.