Skip to content

feat: Add rewrite for IN special form#15663

Open
pramodsatya wants to merge 3 commits intofacebookincubator:mainfrom
pramodsatya:in_rw_null_fix
Open

feat: Add rewrite for IN special form#15663
pramodsatya wants to merge 3 commits intofacebookincubator:mainfrom
pramodsatya:in_rw_null_fix

Conversation

@pramodsatya
Copy link
Collaborator

@pramodsatya pramodsatya commented Dec 1, 2025

Rewrites IN special form when the value being searched for is constant.

  • Returns true if value is in the IN-list.
  • Removes constant elements from IN-list that do not match with value.

Constant expression comparison for these 2 rules uses null-as-indeterminate null comparison mode in order to maintain consistency with the function implementation of IN predicate.
Tests are added to ensure the correctness issue from #15648 is resolved.

Examples:

  • 5 IN (NULL, a, 5) ==> true
  • 5 IN (1, 2, a, b) ==> 5 IN (a, b)

Cherry-picked from #15488.

Depends on #15705, #15708.

@netlify
Copy link

netlify bot commented Dec 1, 2025

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 2b22eeb
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/69327a885a47fe0008d8b12e

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 1, 2025
@aditi-pandit
Copy link
Collaborator

@pramodsatya : Thanks for the code. Code looks good. Please rebase to fix the build issues.

@mbasmanova : PTAL.

Copilot AI review requested due to automatic review settings December 2, 2025 05:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds an optimization for the IN special form by introducing a rewrite rule that simplifies IN expressions when the search value is a constant. The rewrite returns true when the value is found in the IN-list, deduplicates NULL values, and removes non-matching constant values from the IN-list.

Key changes:

  • Implements compile-time optimization for IN expressions with constant search values
  • Reduces runtime overhead by pre-filtering IN-lists during expression compilation
  • Maintains correctness by preserving NULL semantics in SQL IN predicates

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
velox/functions/prestosql/InRewrite.h Header file defining the InRewrite class with static rewrite and registration methods
velox/functions/prestosql/InRewrite.cpp Core implementation of IN expression optimization logic
velox/functions/prestosql/tests/InRewriteTest.cpp Test cases covering basic rewrite scenarios including NULL handling and constant elimination
velox/functions/prestosql/tests/CMakeLists.txt Adds InRewriteTest.cpp to the test build configuration
velox/functions/prestosql/CMakeLists.txt Adds InRewrite.cpp to the library build configuration
velox/functions/prestosql/registration/GeneralFunctionsRegistration.cpp Registers the IN rewrite rule during function initialization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment on lines +23 to +24
/// a non-NULL constant and the IN-list is not constant. Returns `true` if
/// `value` is in the IN-list. Removes constant expressions from IN-list that
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The documentation states "the IN-list is not constant" but this constraint is not actually verified in the implementation. The rewrite will work correctly even when all elements in the IN-list are constants. Consider either removing this phrase from the documentation or clarifying what it means (e.g., "when not all elements of the IN-list are constants" or "when the entire IN expression cannot be fully evaluated to a constant").

Suggested change
/// a non-NULL constant and the IN-list is not constant. Returns `true` if
/// `value` is in the IN-list. Removes constant expressions from IN-list that
/// a non-NULL constant. Returns `true` if `value` is in the IN-list, regardless
/// of whether the IN-list is constant or not. Removes constant expressions from IN-list that

Copilot uses AI. Check for mistakes.
@mbasmanova
Copy link
Contributor

@pramodsatya

addressing concerns from #15648.

Would you expand this to explain what wasn't working before and how it is fixed now? Please, clarify which tests were added to verify the fix.

@pramodsatya
Copy link
Collaborator Author

Thanks @mbasmanova, @aditi-pandit. I made changes to the IN rewrite to ensure constant expression comparison follows the same null handling mode, null-as-indeterminate, as the function implementation. This should effectively resolve #15648. I have verified the fix with reproducer attached in the issue, extended it into a test, and updated the PR description accordingly.

The rewrite now relies on ConstantTypedExpr being able to support user-supplied null handling mode, and I have extracted this change into #15705. Could you please take another look?

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants