Skip to content

Conversation

@ahkcs
Copy link
Contributor

@ahkcs ahkcs commented Sep 9, 2025

Description

Try to resolve the CI failure in #4036

Adding double negation handling logic to resolve the following issue:

Issue: The query not firstname not in ('Amber', 'Dale') should return 2 rows but returns 998 rows in 3.3.
I ran explain queries to it, and here is the result:

  • Working (direct IN): {"bool": {"should": [{"term": "Amber"}, {"term": "Dale"}]}}
  • Broken (double negation): {"bool": {"must_not": [{"bool": {"must_not": [{"bool": {"should": [{"term": "Amber"}, {"term": "Dale"}]}}]}}]}}
    It seems like the double negation was not handled properly in version 3.3

Signed-off-by: Kai Huang <ahkcs@amazon.com>

@Override
public Expression visitNot(Not node, AnalysisContext context) {
// Handle double negation: NOT(NOT(X)) -> X
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we make such change in PredicateAnalyzer? That's the place we create query builder directly. So no transformation will happen between our expression and dsl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we apply the fix in ExpressionAnalyzer, we can prevent the nested must_not[must_not[...]] structure from appearing in explain queries.
Also, in the failing test case, the query uses Legacy engine with calcite disabled, so PredicateAnalyzer was never used

@ahkcs ahkcs requested a review from qianheng-aws September 10, 2025 02:13
@Swiddis Swiddis added the infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label Sep 10, 2025
@ahkcs ahkcs closed this Sep 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants