HIVE-28729: Apply nulls order setting in Reduce Sink operator of join branches #5626
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asf-ci-hive
added
tests pending
tests unstable
and removed
tests pending
tests unstable
labels
zabetak
approved these changes
Jan 31, 2025
| @@ -56,20 +57,28 @@ | |||
| */ | |||
| public class HiveInsertExchange4JoinRule extends RelOptRule { | |||
|
|
|||
| protected static transient final Logger LOG = LoggerFactory | |||
| .getLogger(HiveInsertExchange4JoinRule.class); | |||
| protected static final Logger LOG = LoggerFactory.getLogger(HiveInsertExchange4JoinRule.class); | |||
There was a problem hiding this comment.
The logger does not seem to be used so I guess we can drop it entirely along with the relevant imports.
Comment on lines
63
to
75
| public static HiveInsertExchange4JoinRule exchangeBelowMultiJoin( | ||
| RelFieldCollation.NullDirection defaultAscNullDirection) { | ||
| return new HiveInsertExchange4JoinRule(HiveMultiJoin.class, defaultAscNullDirection); | ||
| } | ||
|
|
||
|
|
||
| /** Rule that creates Exchange operators under a Join operator. */ | ||
| public static final HiveInsertExchange4JoinRule EXCHANGE_BELOW_JOIN = | ||
| new HiveInsertExchange4JoinRule(Join.class); | ||
| public static HiveInsertExchange4JoinRule exchangeBelowJoin( | ||
| RelFieldCollation.NullDirection defaultAscNullDirection) { | ||
| return new HiveInsertExchange4JoinRule(Join.class, defaultAscNullDirection); | ||
| } | ||
|
|
||
| private final RelFieldCollation.NullDirection defaultAscNullDirection; |
There was a problem hiding this comment.
I don't see a big advantage in introducing the static factories. Since the constructor is public we could just keep that one. If we want to make the rule creation shorter then we could have a static factory with a shorter name (e.g., of(clazz,nullDirection))
There was a problem hiding this comment.
Removed the factory methods. Using constructor in CalcitePlanner
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveInsertExchange4JoinRule.java
Show resolved
Hide resolved
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
tests unstable
labels
zabetak
approved these changes
Feb 3, 2025
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Use the default null order configured via
hive.default.nulls.lastin the Calcite ruleHiveInsertExchange4JoinRule.javawhen creating sort keys. This rule is used only whenhive.cbo.returnpath.hiveopis enabled.Why are the changes needed?
The setting
hive.default.nulls.lastdoesn't have affect to theHiveSortExchangeoperators created by this rule and the RS operators built from them.Does this PR introduce any user-facing change?
Explain output of query plans has join operation may change.
Is the change a dependency upgrade?
No.
How was this patch tested?