Skip to content

[CALCITE-739] Extend RexUtil.pullFactors to recognize additional common factors (follow-up) - #5286

Merged
rubenada merged 1 commit into
apache:mainfrom
rubenada:CALCITE-739_followUp
Sep 24, 2026
Merged

rubenada merged 1 commit into
apache:mainfrom
rubenada:CALCITE-739_followUp

Conversation

@rubenada

@rubenada rubenada commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-739

Changes Proposed

The original patch for CALCITE-739 (51e6cb1) implemented some extra factoring, but it also (unnecessarily) may alter the "orientation" of comparison predicates. This can have a certain impact on downstream projects which verify plan forms, e.g. in Hive we detected 150+ tests that fail simply because they expect $a=$b and now they get $b=$a (without any extra pull factors being involved).
This was not the purpose of CALCITE-739.
With some minor adjustments, we should be able to maintain the new feature of extra factoring in RexUtil.pullFactors without changing unnecessarily the authored orientation of certain predicates:

  • drop default: return normalizeComparison(rex); in pull() (RexUtil.java:2970): it does no factoring at all, it is pure canonicalization
  • in commonFactors, store the original as the value: map.put(normalized, conjunction) rather than map.put(normalized, normalized), so pulled-out factors keep their authored orientation

These changes will reduce the side effects of the original commit (facilitating the eventual upgrade to 1.43 for downstream projects that test plan shapes), keeping the improvement of CALCITE-739 (see specific test RexProgramTest#testSimplifyComparisonSymmetry still passes with the proposed adjustment).

@rubenada

Copy link
Copy Markdown
Contributor Author

@xuzifu666 sorry I did not participate of the review of your original PR #5246, could you please take a look at this follow-up proposal?

@xuzifu666 xuzifu666 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the improvement! normalizeComparison is indeed unnecessary.

@sonarqubecloud

Copy link
Copy Markdown

@rubenada rubenada added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 23, 2026
@rubenada
rubenada merged commit afdea2e into apache:main Sep 24, 2026
20 checks passed
@rubenada

Copy link
Copy Markdown
Contributor Author

Thanks for the review @xuzifu666 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants