[CALCITE-2888] Enhance RexImplicationChecker to better handle cast by jh3507 · Pull Request #1081 · apache/calcite

jh3507 · 2019-03-01T18:49:24Z

make RexImplicationChecker use executor's reduce function when generating a value for literal(VisitorDataContext)
allow other implementations of RexExecutor

Change-Id: Ie343535c1061d0927356aa37332c048306c5739a

hsyuan · 2019-03-02T01:36:36Z

core/src/main/java/org/apache/calcite/plan/VisitorDataContext.java

-}

+  private static RexNode convertAndRemoveCast(RexBuilder builder, RexExecutor executor,
+                                     RexNode literal, RelDataType toType) {


the name literal is a little bit misleading, because it may not be literal.

Thanks. Updated.

danny0405 · 2019-03-02T04:39:48Z

core/src/main/java/org/apache/calcite/plan/VisitorDataContext.java


  public static DataContext of(RelDataType rowType, RexNode rex) {
+    return of(rowType, rex, null, null);
+  }


useless constructor

This has come from the previous definition. I think useless is somewhat subjective expression. This method is a public static method in a public class. I personally don't make this function call, but someone might be already using it. I didn't want to abruptly remove a public method. Also, giving nulls to builder and executor would make it possible to bypass the executor so that the output you get from it would be exactly the same as before. So, I think this can be useful especially for those who think this change is not necessary :) That being said, I am okay with removing existing 'of' methods if the community is okay with it.

Also, as far as I know, this can never be a constructor.

danny0405 · 2019-03-02T04:47:40Z

-1, I think it's not a good idea to make so muck changes just to move constant reduction logic into RexImplicationChecker, because there is a rule/rules can do this.

Actually for production, the RexExecutor is pluggable for users, so the cast is not much big problem.

danny0405 · 2019-03-02T04:51:02Z

core/src/main/java/org/apache/calcite/rex/RexExecutor.java

+   * @return Objects returned after applying generated code with dataValues.
+   */
+  Object[] execute(RexBuilder rexBuilder, List<RexNode> exps,
+                   RelDataType rowType,  DataContext dataValues);


-1, this should not be made into an interface method, because from this signature or comment, user can see nothing about the relationship with RexExecutable which could got from another method.

I agree with the point that the comment is misleaing. Thanks for noticing it. I have updated the comment. But, in terms of what should be included in the interface, I think one that is commonly used among the various implementations should go into the interface. execution is needed when checking if a value is satisfiable in the filter. So, I think it should be implemented. Delegating it to the existing RexExecutionImpl would be one possible solution if implementing it is too much effort.

- make RexImplicationChecker use executor's reduce function when generating value for literal(in VisitorDataContext) - allow other versions of RexExecutor Change-Id: Ie343535c1061d0927356aa37332c048306c5739a

…ng) & [CALCITE-3797] RelMdPredicates should not assume the RexExecutor instance is RexExecutorImpl (apache#9) ## DX-94069: [CALCITE-3439] Support Intersect and Minus in RelMdPredicates (Jin Xing)  This pr is part of the efforts for Dremio Calcite 1.22 migration. This cherry-pick focuses on adding support for Intersect and Minus in RelMdPredicates, while currently only Union is supported. Dremio side change: dremio/dremio#8130 ### Cherry picks:  ---- **Commit:** 76f9847 **Conflicts Resolved:** {If applicable, explanation of any deviations from the original commit that were made.} 1. It is noteworthy that Dremio build hit an NPE with CALCITE-3439 cherry-pick alone, which is caused by the null executor set [during DremioVolcanoPlanner initialization when building RelOptPlanner in RelOptPlannerSpecBuild.](https://github.com/dremio/dremio/blob/33772cbe1af9d629e7512c86ed1b8430a7fad37d/community/sabot/kernel/src/test/java/com/dremio/test/specs/RelOptPlannerSpecBuild.java#L46). Therefore, we club [CALCITE-3797](https://github.com/sabrinayuu/dcalcite/tree/CALCITE-3797) with [CALCITE-3439](https://github.com/sabrinayuu/dcalcite/tree/CALCITE-3439) because CALCITE-3797 brings in [the fix](apache@bf3fb49#diff-23e11f70f80e09eb099a586a4ae5ed00807fd9412045427c37491cac2dd6946aR425), with which `getPredicates(Union union, RelMetadataQuery mq)` will use a constant executor `RexUtil.EXECUTOR` when the planner has a null executor. 2. To address the flaky test RelMetadataTest.testPullUpPredicatesFromUnion2 caused by [the same hash code shared by predicates with different expression order](dremio/calcite#9 (comment)), we partially cherry picked [CALCITE-4073](apache@a4fa054#diff-6137557db145272ce78c72b01e41c06950b47362fb40d9f9268fec3ca84cd11cR2047-R2053). **Original Description:** https://issues.apache.org/jira/browse/CALCITE-3439 Currently only Union is supported in RelMdPredicates. We might also support Intersect and Minus; This issue was found when resolving [CALCITE-3428](https://issues.apache.org/jira/browse/CALCITE-3428) ---- sabrinayuu/dcalcite#1 **Commit:** bf3fb49 **Conflicts Resolved:** Ran into discrepancies when updating file RexImplicationChecker: Upstream: https://github.com/apache/calcite/blob/calcite-1.22.0/core/src/main/java/org/apache/calcite/plan/RexImplicationChecker.java#L252-L258 Our branch: https://github.com/dremio/calcite/blob/1.22-dremio/core/src/main/java/org/apache/calcite/plan/RexImplicationChecker.java#L267-L269 The difference is caused by [DX-14980](https://dremio.atlassian.net/browse/DX-14980?focusedCommentId=258804) which introduced a fix only on Dremio Calcite fork to enhance the cast handling in RexImplicationChecker but the same change [did not make it to the upstream Calcite... ](apache#1081) [(CALCITE-2888)](https://issues.apache.org/jira/browse/CALCITE-2888). Fortunately, the difference that matters to this pr is a minor code refactor that can be reverted and then apply the original cherrypick. **Original Description:** https://issues.apache.org/jira/browse/CALCITE-3797 The RexExecutor is actually pluggable, we should not do such an assumption. --------- Co-authored-by: jinxing <jinxing.corey@gmail.com> Co-authored-by: yuzhao.cyz <yuzhao.cyz@gmail.com>

…ng) & [CALCITE-3797] RelMdPredicates should not assume the RexExecutor instance is RexExecutorImpl (apache#9) RelMdPredicates (Jin Xing)  This pr is part of the efforts for Dremio Calcite 1.22 migration. This cherry-pick focuses on adding support for Intersect and Minus in RelMdPredicates, while currently only Union is supported. Dremio side change: dremio/dremio#8130  ---- **Commit:** 76f9847 **Conflicts Resolved:** {If applicable, explanation of any deviations from the original commit that were made.} 1. It is noteworthy that Dremio build hit an NPE with CALCITE-3439 cherry-pick alone, which is caused by the null executor set [during DremioVolcanoPlanner initialization when building RelOptPlanner in RelOptPlannerSpecBuild.](https://github.com/dremio/dremio/blob/33772cbe1af9d629e7512c86ed1b8430a7fad37d/community/sabot/kernel/src/test/java/com/dremio/test/specs/RelOptPlannerSpecBuild.java#L46). Therefore, we club [CALCITE-3797](https://github.com/sabrinayuu/dcalcite/tree/CALCITE-3797) with [CALCITE-3439](https://github.com/sabrinayuu/dcalcite/tree/CALCITE-3439) because CALCITE-3797 brings in [the fix](apache@bf3fb49#diff-23e11f70f80e09eb099a586a4ae5ed00807fd9412045427c37491cac2dd6946aR425), with which `getPredicates(Union union, RelMetadataQuery mq)` will use a constant executor `RexUtil.EXECUTOR` when the planner has a null executor. 2. To address the flaky test RelMetadataTest.testPullUpPredicatesFromUnion2 caused by [the same hash code shared by predicates with different expression order](dremio/calcite#9 (comment)), we partially cherry picked [CALCITE-4073](apache@a4fa054#diff-6137557db145272ce78c72b01e41c06950b47362fb40d9f9268fec3ca84cd11cR2047-R2053). **Original Description:** https://issues.apache.org/jira/browse/CALCITE-3439 Currently only Union is supported in RelMdPredicates. We might also support Intersect and Minus; This issue was found when resolving [CALCITE-3428](https://issues.apache.org/jira/browse/CALCITE-3428) ---- sabrinayuu/dcalcite#1 **Commit:** bf3fb49 **Conflicts Resolved:** Ran into discrepancies when updating file RexImplicationChecker: Upstream: https://github.com/apache/calcite/blob/calcite-1.22.0/core/src/main/java/org/apache/calcite/plan/RexImplicationChecker.java#L252-L258 Our branch: https://github.com/dremio/calcite/blob/1.22-dremio/core/src/main/java/org/apache/calcite/plan/RexImplicationChecker.java#L267-L269 The difference is caused by [DX-14980](https://dremio.atlassian.net/browse/DX-14980?focusedCommentId=258804) which introduced a fix only on Dremio Calcite fork to enhance the cast handling in RexImplicationChecker but the same change [did not make it to the upstream Calcite... ](apache#1081) [(CALCITE-2888)](https://issues.apache.org/jira/browse/CALCITE-2888). Fortunately, the difference that matters to this pr is a minor code refactor that can be reverted and then apply the original cherrypick. **Original Description:** https://issues.apache.org/jira/browse/CALCITE-3797 The RexExecutor is actually pluggable, we should not do such an assumption. --------- Co-authored-by: jinxing <jinxing.corey@gmail.com> Co-authored-by: yuzhao.cyz <yuzhao.cyz@gmail.com>

jh3507 force-pushed the CALCITE-2888 branch from f74a473 to 54450f0 Compare March 1, 2019 19:00

hsyuan reviewed Mar 2, 2019

View reviewed changes

danny0405 reviewed Mar 2, 2019

View reviewed changes

[CALCITE-2888] Enhance RexImplicationChecker to better handle cast

8db0d7b

- make RexImplicationChecker use executor's reduce function when generating value for literal(in VisitorDataContext) - allow other versions of RexExecutor Change-Id: Ie343535c1061d0927356aa37332c048306c5739a

jh3507 force-pushed the CALCITE-2888 branch from 54450f0 to 8db0d7b Compare March 2, 2019 07:48

julianhyde force-pushed the master branch from 2c19098 to f3758a6 Compare September 16, 2019 17:44

danny0405 force-pushed the master branch 2 times, most recently from 80f411d to ca27fe9 Compare November 30, 2019 07:52

vlsi force-pushed the master branch 2 times, most recently from 49cb002 to 8768a23 Compare December 29, 2019 12:07

hsyuan force-pushed the master branch from e42a79b to a0ef3c9 Compare March 29, 2020 16:53

vlsi force-pushed the master branch from e4458fa to 5462be9 Compare July 17, 2020 20:20

vlsi force-pushed the master branch from fd20efd to 3311d45 Compare December 9, 2020 20:10

julianhyde force-pushed the master branch from 52c1284 to d4e1eea Compare March 1, 2021 02:56

vlsi force-pushed the master branch from 7f65cf2 to 4bc9166 Compare March 24, 2021 09:43

zabetak force-pushed the master branch from f14cf4c to dcbc493 Compare March 10, 2022 09:13

julianhyde force-pushed the main branch from fa65a2e to 1226d1a Compare June 20, 2022 20:27

asfgit force-pushed the main branch from 9fc50f2 to e2f949d Compare September 10, 2022 16:37

asfgit force-pushed the main branch from f8f8a51 to a326bd2 Compare January 25, 2023 07:39

julianhyde force-pushed the main branch 2 times, most recently from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

libenchao force-pushed the main branch from 47db81a to 0be8eae Compare November 10, 2023 13:18

F21 force-pushed the main branch from 7d38212 to cacf36a Compare February 17, 2025 03:33

asolimando force-pushed the main branch from 19400ab to 2d5ec10 Compare May 28, 2025 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-2888] Enhance RexImplicationChecker to better handle cast#1081

[CALCITE-2888] Enhance RexImplicationChecker to better handle cast#1081
jh3507 wants to merge 1 commit intoapache:mainfrom
jh3507:CALCITE-2888

jh3507 commented Mar 1, 2019

Uh oh!

hsyuan Mar 2, 2019

Uh oh!

jh3507 Mar 2, 2019

Uh oh!

danny0405 Mar 2, 2019

Uh oh!

jh3507 Mar 2, 2019

Uh oh!

danny0405 commented Mar 2, 2019 •

edited

Loading

Uh oh!

danny0405 Mar 2, 2019

Uh oh!

jh3507 Mar 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jh3507 commented Mar 1, 2019

Uh oh!

hsyuan Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

jh3507 Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

danny0405 Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

jh3507 Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

danny0405 commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danny0405 Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

jh3507 Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danny0405 commented Mar 2, 2019 •

edited

Loading