-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-30530][SQL][FOLLOW-UP] Remove unnecessary codes and fix comments accordingly in UnivocityParser #27287
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
|
cc @cloud-fan and @MaxGekk. It's rather minor but wanted to clean up here. |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala
Outdated
Show resolved
Hide resolved
|
Test build #117081 has finished for PR 27287 at commit
|
|
Test build #117087 has finished for PR 27287 at commit
|
|
Test build #117077 has finished for PR 27287 at commit
|
|
retest this please |
1 similar comment
|
retest this please |
|
Something wrong is going on in #27287 (comment). This exception shouldn't happen: It was added by #27166, and the limit shouldn't be reachable. The test doesn't register any log appenders: spark/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala Lines 1731 to 1761 in d4c6ec6
|
|
I propose to assign names to log appenders, and print the names in exception. So, we will know which one wasn't removed. |
|
Test build #117097 has finished for PR 27287 at commit
|
|
Jenkins, retest this please |
|
Test build #117121 has finished for PR 27287 at commit
|
|
Test build #117124 has finished for PR 27287 at commit
|
|
Merged to master. |
What changes were proposed in this pull request?
This PR proposes to clean up
UnivocityParser.Why are the changes needed?
It will slightly improve the performance since we don't do unnecessary computation for Array concatenations/creation.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Manually ran the existing tests.