-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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-12976][SQL] Add LazilyGenerateOrdering and use it for RangePartitioner of Exchange. #10894
Conversation
Test build #49975 has finished for PR 10894 at commit
|
Cool! Are there any remaining usages of |
Yes, there are some places to use it to interpret ( |
Test build #50956 has finished for PR 10894 at commit
|
Test build #50957 has finished for PR 10894 at commit
|
this(ordering.map(BindReferences.bindReference(_, inputSchema))) | ||
|
||
@transient | ||
lazy val generatedOrdering = GenerateOrdering.generate(ordering) |
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.
Will this use of lazy val
incur a performance penalty due to having to check whether it has been initialized on each call to compare()
? If so, I wonder whether we could eagerly initialize the ordering in the constructor then re-initialize on executors as part of readObject()
, e.g. @transient private[this] var generatedOrdering = ...
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.
Ah, yes, it might cause a performance penalty.
I'll try to rewrite as you mentioned.
Test build #51167 has finished for PR 10894 at commit
|
I spot one other place where we might be able to use this in Also, one minor naming suggestion: I'd probably call this Aside from those two minor changes, this looks good to me. Sorry for missing those suggestions on the first review pass. |
I renamed And I found |
Test build #51225 has finished for PR 10894 at commit
|
…to fail code generation caused by UnresolvedAttribute.
Test build #51232 has finished for PR 10894 at commit
|
This looks good to me, so I'm going to merge it into master. Thanks @ueshin! Regarding |
@JoshRosen Thank you for merging this. |
Add
LazilyGenerateOrdering
to support generated ordering forRangePartitioner
ofExchange
instead ofInterpretedOrdering
.