-
Notifications
You must be signed in to change notification settings - Fork 28k
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-6566][SQL]: Related changes for newer parquet version #5889
Conversation
Merged build triggered. |
Merged build started. |
Test build #31760 has started for PR 5889 at commit |
Test build #31760 has finished for PR 5889 at commit
|
Merged build finished. Test PASSed. |
Test PASSed. |
Thanks for doing this! This has been on my todo list for a long time :) We're currently terribly busy working on features that are expected to be delivered in 1.4 release. Will come back to this once I finish my work at hand. |
(I'm so glad that we can finally remove the messy |
Merged build triggered. |
Merged build started. |
Test build #32089 has started for PR 5889 at commit |
Test build #32089 has finished for PR 5889 at commit
|
Merged build finished. Test PASSed. |
Test PASSed. |
Merged build triggered. |
Merged build started. |
Test build #33570 has started for PR 5889 at commit |
Test build #33570 has finished for PR 5889 at commit
|
Merged build finished. Test PASSed. |
Test PASSed. |
Merged build triggered. |
Merged build started. |
Merged build finished. Test FAILed. |
cc @liancheng I have rebased. |
ok to test |
Merged build triggered. |
Merged build started. |
Test build #34285 has started for PR 5889 at commit |
@saucam It seemed to be Jenkins issue rather than the problem of you PR. |
Test build #34285 has finished for PR 5889 at commit
|
Merged build finished. Test FAILed. |
if (value == null) { | ||
return false | ||
} | ||
return hSet.contains(value) |
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.
Generally we try to avoid using return
in Scala code. (You may find a detailed guide here.) For this method, we'd prefer:
value != null && hSet.contains(value)
BTW, why the name hSet
? Maybe valueSet
instead?
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.
Oh, just saw the hset
field in InSet
, still prefer valueSet
here though...
May want to update the title of this PR to be clearer, since it looks like the dep. bump was done in a separate PR. |
incorporated review comments retest please |
Merged build triggered. |
Merged build started. |
Test build #34291 has started for PR 5889 at commit |
Test build #34291 has finished for PR 5889 at commit
|
Merged build finished. Test PASSed. |
@liancheng looks ok to you now ? |
@saucam Sorry for the late reply. This LGTM now. The inefficient code path in Parquet still exists (sequentially retrieving |
This brings in major improvement in that footers are not read on the driver. This also cleans up the code in parquetTableOperations, where we had to override getSplits to eliminate multiple listStatus calls. cc liancheng are there any other changes we need for this ? Author: Yash Datta <Yash.Datta@guavus.com> Closes apache#5889 from saucam/parquet_1.6 and squashes the following commits: d1bf41e [Yash Datta] SPARK-7340: Fix scalastyle and incorporate review comments c9aa042 [Yash Datta] SPARK-7340: Use the new user defined filter predicate for pushing down inset into parquet 56bc750 [Yash Datta] SPARK-7340: Change parquet version to latest release
This brings in major improvement in that footers are not read on the driver. This also cleans up the code in parquetTableOperations, where we had to override getSplits to eliminate multiple listStatus calls.
cc @liancheng
are there any other changes we need for this ?