HIVE-29734: Cleanup Bug.java after Calcite upgrade 1.42.0 - #6720
HIVE-29734: Cleanup Bug.java after Calcite upgrade 1.42.0#6720rubenada wants to merge 12 commits into
Conversation
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
… seem identical to Calcite's version, the latter causes issues with certain tests: - Testing / split-14 / PostProcess / testCliDriver[lvj_mapjoin] – org.apache.hadoop.hive.cli.split13.TestMiniLlapLocalCliDriver - Testing / split-05 / PostProcess / testCliDriver[lateral_view_cbo] – org.apache.hadoop.hive.cli.split12.TestMiniLlapLocalCliDriver - Testing / split-05 / PostProcess / testCliDriver[lateral_view_ppd] – org.apache.hadoop.hive.cli.split12.TestMiniLlapLocalCliDriver - Testing / split-10 / PostProcess / testCliDriver[tez_union_udtf] – org.apache.hadoop.hive.cli.TestMiniTezCliDriver - Testing / split-19 / PostProcess / testCliDriver[union26] – org.apache.hadoop.hive.cli.split27.TestMiniLlapLocalCliDriver - Testing / split-04 / PostProcess / testCliDriver[nonmr_fetch] – org.apache.hadoop.hive.cli.split7.TestCliDriver
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
rubenada
changed the title
zabetak
left a comment
zabetak
left a comment
There was a problem hiding this comment.
Left some suggestions for further cleanup but overall looks good.
| private static final class JoinLeftEmptyRuleConfig extends HiveRuleConfig | ||
| implements PruneEmptyRules.JoinLeftEmptyRuleConfig { | ||
| } |
There was a problem hiding this comment.
Can we do a bit better in terms of cleanup and drop this kind of trivial extension class? Maybe we can instantiate the rules directly using the configs available in Calcite. For example:
public static final RelOptRule JOIN_LEFT_INSTANCE = PruneEmptyRules.JoinLeftEmptyRuleConfig.DEFAULT
.withRelBuilderFactory(HiveRelFactories.HIVE_BUILDER).toRule();
public static final RelOptRule JOIN_RIGHT_INSTANCE = PruneEmptyRules.JoinRightEmptyRuleConfig.DEFAULT
.withRelBuilderFactory(HiveRelFactories.HIVE_BUILDER).toRule();
Maybe this allow us to drop also some SEMI/ANTI rule instances. Can you check if its feasible?
There was a problem hiding this comment.
I think it can be done, on it....
BTW, I noticed there's getJoinRightInstance for HiveJoin, HiveSemiJoin and HiveAntiJoin; but for getJoinLeftInstance there's only HiveJoin, HiveSemiJoin.
Is there a specific reason why HiveAntiJoin is not considered for JoinLeftEmptyRule?
Was it intentional or just a slip?
If we refactor to use the above suggestion, we will also get JoinLeftEmptyRule for Anti...
| private static final class CorrelateLeftEmptyRuleConfig extends HiveRuleConfig | ||
| implements PruneEmptyRules.CorrelateLeftEmptyRuleConfig { | ||
| } |
There was a problem hiding this comment.
Likewise let's re-evaluate how much we need the trivial extension classes.
There was a problem hiding this comment.
Not blocking for the PR but if we can test and get rid of the entire class as part of this cleanup it would be great.
There was a problem hiding this comment.
How about setting calcite.default.charset property in saffron and get rid of the entire class?
|
|
||
| # Change logger level to DEBUG, to see all application of CBO rules and the operators they produce | ||
| logger.CBORuleLogger.name = org.apache.hadoop.hive.ql.optimizer.calcite.RuleEventLogger | ||
| logger.CBORuleLogger.name = org.apache.calcite.plan.RuleEventLogger |
There was a problem hiding this comment.
Did you verify that the logs are displayed as expected after the change?
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
|
3 participants
What changes were proposed in this pull request?
Cleanup Bug.java after Calcite upgrade 1.42.0, i.e. remove ad-hoc pieces of code with fixes on Hive side that are no longer required, since these fixes are now on Calcite side.
Items removed:
withPruneInputOfAggregateflag asfalse(otherwise we'll get regressions in several tests), but I have updated the corresponding Bug item to referenceCALCITE_6391.HiveFilterTableFunctionTransposeRule) has diverged from the Calcite built-in version, so this change is not straightforward (using the Calcite one as it is causes regressions in several tests). For the moment, I leftHiveFilterTableFunctionTransposeRuleand created a follow-up sub-task (HIVE-29840) under the ticket dedicated to this purpose HIVE-29738 CBO: Replace in-house rules and code with rules and code provided by Calcite.Why are the changes needed?
Remove unnecessary code.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
No real change is expected with this adjustment, so existing tests should pass with no regressions.