Skip to content

[CALCITE-7450] ValuesReduceRule incorrectly drops tuples when filter condition is irreducible#4846

Merged
caicancai merged 1 commit intoapache:mainfrom
darpan-e6:CALCITE-7450
Mar 26, 2026
Merged

[CALCITE-7450] ValuesReduceRule incorrectly drops tuples when filter condition is irreducible#4846
caicancai merged 1 commit intoapache:mainfrom
darpan-e6:CALCITE-7450

Conversation

@darpan-e6
Copy link
Copy Markdown
Contributor

@darpan-e6 darpan-e6 commented Mar 25, 2026

Summary

  • When a filter condition on VALUES cannot be reduced to RexLiteral by RexExecutorImpl,it returns the expression unchanged.
  • ValuesReduceRule.apply() then calls isAlwaysTrue() on the unreduced RexCall, which returns false, causing the rule to incorrectly drop the tuple.
  • The fix checks whether the reduced value is a RexLiteral before evaluating isAlwaysTrue(); if it is not reducible, the rule bails out and leaves the plan unchanged.

Test plan

  • Added ValuesReduceRuleTest with a RAND() function that cannot be reduced by Janino.
  • The test verifies the plan is unchanged after applying PROJECT_FILTER_VALUES_MERGE — tuples are preserved.

@darpan-e6 darpan-e6 force-pushed the CALCITE-7450 branch 3 times, most recently from eaf324e to 715dd8f Compare March 25, 2026 16:19
@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Mar 25, 2026
@darpan-e6 darpan-e6 force-pushed the CALCITE-7450 branch 2 times, most recently from ca59c54 to b2e1f1d Compare March 25, 2026 17:55
@caicancai
Copy link
Copy Markdown
Member

caicancai commented Mar 26, 2026

@darpan-e6 As discussed in the email, we do not need to retain the email addresses of Claude's co-authors.

@darpan-e6
Copy link
Copy Markdown
Contributor Author

darpan-e6 commented Mar 26, 2026

@darpan-e6 As discussed in the email, we do not need to retain the email addresses of Claude's co-authors.

I'd like to keep it if that's okay, Claude did help me write the test, and it feels right to acknowledge that. Happy to discuss if there's a policy reason to remove it though!

Also, I haven't received any mail from you on this.

@caicancai
Copy link
Copy Markdown
Member

@darpan-e6 As discussed in the email, we do not need to retain the email addresses of Claude's co-authors.

I'd like to keep it if that's okay, Claude did help me write the test, and it feels right to acknowledge that. Happy to discuss if there's a policy reason to remove it though!

Also, I haven't received any mail from you on this.

You can check the Calcite mailing list; there's discussion about this there.

@darpan-e6
Copy link
Copy Markdown
Contributor Author

@darpan-e6 As discussed in the email, we do not need to retain the email addresses of Claude's co-authors.

I'd like to keep it if that's okay, Claude did help me write the test, and it feels right to acknowledge that. Happy to discuss if there's a policy reason to remove it though!
Also, I haven't received any mail from you on this.

You can check the Calcite mailing list; there's discussion about this there.

Okay, makes sense. I will remove it.

…condition is irreducible

When the filter condition contains a UDF that cannot be reduced to a
RexLiteral (e.g., no Janino CallImplementor), isAlwaysTrue() returns
false for the unreduced RexCall, causing the rule to incorrectly drop
the tuple. The fix checks whether the reduced value is a RexLiteral
before evaluating isAlwaysTrue(); if it is not, the rule bails out
and leaves the plan unchanged.
@sonarqubecloud
Copy link
Copy Markdown

@caicancai caicancai merged commit 93c792f into apache:main Mar 26, 2026
19 checks passed
@zabetak
Copy link
Copy Markdown
Member

zabetak commented Mar 26, 2026

FYI: We have a whole framework in place for testing rules based on RelOptFixture and DiffRepository. The test in this PR would be much shorter and easier to read if it was re-using existing code. Ideally, the test should be either moved to RelOptRulesTest class or follow the paradigm of AggregateFilterToFilteredAggregateRuleTest.

@darpan-e6 If you have some time to rework the test raise a new PR and ping me for review.

@caicancai
Copy link
Copy Markdown
Member

Oh, my question, I forgot, sorry.

@darpan-e6
Copy link
Copy Markdown
Contributor Author

I was not aware of it. Thanks @zabetak, I will do that. Allow me some time (like 4-5 hours).

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.

4 participants