generated from amazon-archives/__template_Custom
-
Notifications
You must be signed in to change notification settings - Fork 181
[BugFix] Fix the bug when boolean comparison condition is simplifed to field #5071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
songkant-aws
wants to merge
2
commits into
opensearch-project:main
Choose a base branch
from
songkant-aws:boolean-comparison-expression-fix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 changes: 9 additions & 0 deletions
9
...t/src/test/resources/expectedOutput/calcite/explain_filter_query_string_with_boolean.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| calcite: | ||
| logical: | | ||
| LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) | ||
| LogicalProject(firstname=[$1]) | ||
| LogicalFilter(condition=[$12]) | ||
| LogicalFilter(condition=[query_string(MAP('query', 'firstname:Amber':VARCHAR))]) | ||
| CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]]) | ||
| physical: | | ||
| CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]], PushDownContext=[[PROJECT->[firstname, male], FILTER->AND(query_string(MAP('query', 'firstname:Amber':VARCHAR)), $1), PROJECT->[firstname], LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","query":{"bool":{"must":[{"query_string":{"query":"firstname:Amber","fields":[],"type":"best_fields","default_operator":"or","max_determinized_states":10000,"enable_position_increments":true,"fuzziness":"AUTO","fuzzy_prefix_length":0,"fuzzy_max_expansions":50,"phrase_slop":0,"escape":false,"auto_generate_synonyms_phrase_query":true,"fuzzy_transpositions":true,"boost":1.0}},{"term":{"male":{"value":true,"boost":1.0}}}],"adjust_pure_negative":true,"boost":1.0}},"_source":{"includes":["firstname"],"excludes":[]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)]) |
9 changes: 9 additions & 0 deletions
9
...test/resources/expectedOutput/calcite/explain_filter_query_string_with_boolean_false.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| calcite: | ||
| logical: | | ||
| LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) | ||
| LogicalProject(firstname=[$1]) | ||
| LogicalFilter(condition=[NOT($12)]) | ||
| LogicalFilter(condition=[query_string(MAP('query', 'firstname:Amber':VARCHAR))]) | ||
| CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]]) | ||
| physical: | | ||
| CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]], PushDownContext=[[PROJECT->[firstname, male], FILTER->AND(query_string(MAP('query', 'firstname:Amber':VARCHAR)), NOT($1)), PROJECT->[firstname], LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","query":{"bool":{"must":[{"query_string":{"query":"firstname:Amber","fields":[],"type":"best_fields","default_operator":"or","max_determinized_states":10000,"enable_position_increments":true,"fuzziness":"AUTO","fuzzy_prefix_length":0,"fuzzy_max_expansions":50,"phrase_slop":0,"escape":false,"auto_generate_synonyms_phrase_query":true,"fuzzy_transpositions":true,"boost":1.0}},{"term":{"male":{"value":false,"boost":1.0}}}],"adjust_pure_negative":true,"boost":1.0}},"_source":{"includes":["firstname"],"excludes":[]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)]) |
55 changes: 55 additions & 0 deletions
55
integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5054.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| setup: | ||
| - do: | ||
| query.settings: | ||
| body: | ||
| transient: | ||
| plugins.calcite.enabled: true | ||
|
|
||
| --- | ||
| teardown: | ||
| - do: | ||
| query.settings: | ||
| body: | ||
| transient: | ||
| plugins.calcite.enabled: false | ||
|
|
||
| --- | ||
| "Fix boolean field comparison pushdown": | ||
| - skip: | ||
| features: | ||
| - headers | ||
| - allowed_warnings | ||
| - do: | ||
| indices.create: | ||
| index: test-boolean | ||
| body: | ||
| mappings: | ||
| properties: | ||
| is_internal: | ||
| type: boolean | ||
| name: | ||
| type: keyword | ||
|
|
||
| - do: | ||
| bulk: | ||
| index: test-boolean | ||
| refresh: true | ||
| body: | ||
| - '{"index": {}}' | ||
| - '{ "is_internal": true, "name": "doc1" }' | ||
| - '{"index": {}}' | ||
| - '{ "is_internal": false, "name": "doc2" }' | ||
| - '{"index": {}}' | ||
| - '{ "is_internal": true, "name": "doc3" }' | ||
|
|
||
| - do: | ||
| allowed_warnings: | ||
| - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled' | ||
| headers: | ||
| Content-Type: 'application/json' | ||
| ppl: | ||
| body: | ||
| query: source=test-boolean | where is_internal=true | fields name | ||
|
|
||
| - match: { total: 2 } | ||
| - length: { datarows: 2 } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,7 +225,13 @@ public static QueryExpression analyzeExpression( | |
| requireNonNull(expression, "expression"); | ||
| try { | ||
| // visits expression tree | ||
| QueryExpression queryExpression = (QueryExpression) expression.accept(visitor); | ||
| Expression result = expression.accept(visitor); | ||
| // When a boolean field is used directly as a filter condition (e.g., `where male` after | ||
| // Calcite simplifies `where male = true`), convert NamedFieldExpression to a term query. | ||
| if (result instanceof NamedFieldExpression namedField && namedField.isBooleanType()) { | ||
| return QueryExpression.create(namedField).isTrue(); | ||
| } | ||
| QueryExpression queryExpression = (QueryExpression) result; | ||
| return queryExpression; | ||
| } catch (Throwable e) { | ||
| if (e instanceof UnsupportedScriptException) { | ||
|
|
@@ -566,7 +572,18 @@ private QueryExpression prefix(RexCall call) { | |
| throw new PredicateAnalyzerException(message); | ||
| } | ||
|
|
||
| QueryExpression expr = (QueryExpression) call.getOperands().get(0).accept(this); | ||
| Expression operandExpr = call.getOperands().get(0).accept(this); | ||
| // Handle NOT(boolean_field) - generate term query with false value | ||
| // This covers cases like: male = false -> NOT($12) | ||
| if (operandExpr instanceof NamedFieldExpression namedField && namedField.isBooleanType()) { | ||
| return QueryExpression.create(namedField).isFalse(); | ||
| } | ||
| QueryExpression expr = (QueryExpression) operandExpr; | ||
| // Handle NOT(IS_TRUE(boolean_field)) - convert to term query with false value | ||
| // This covers cases where IS_TRUE was explicitly applied | ||
| if (expr instanceof SimpleQueryExpression simpleExpr && simpleExpr.isBooleanFieldIsTrue()) { | ||
| return QueryExpression.create(simpleExpr.rel).isFalse(); | ||
| } | ||
|
Comment on lines
+582
to
+586
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| return expr.not(); | ||
| } | ||
|
|
||
|
|
@@ -582,6 +599,12 @@ private QueryExpression postfix(RexCall call) { | |
|
|
||
| if (call.getKind() == SqlKind.IS_TRUE) { | ||
| Expression qe = call.getOperands().get(0).accept(this); | ||
| // When IS_TRUE is applied to a boolean field reference (e.g., IS_TRUE(boolean_field)), | ||
| // create a QueryExpression from the NamedFieldExpression and call isTrue(). | ||
| // When IS_TRUE is applied to a predicate (already evaluated), qe is a QueryExpression. | ||
| if (qe instanceof NamedFieldExpression namedField && namedField.isBooleanType()) { | ||
| return QueryExpression.create(namedField).isTrue(); | ||
| } | ||
| return ((QueryExpression) qe).isTrue(); | ||
| } | ||
|
|
||
|
|
@@ -797,8 +820,12 @@ private QueryExpression andOr(RexCall call) { | |
| public Expression tryAnalyzeOperand(RexNode node) { | ||
| try { | ||
| Expression expr = node.accept(this); | ||
| if (expr instanceof NamedFieldExpression) { | ||
| return expr; | ||
| // When a boolean field is used directly as a filter condition (e.g., `where male` after | ||
| // Calcite simplifies `where male = true`), convert NamedFieldExpression to a term query. | ||
| if (expr instanceof NamedFieldExpression namedField && namedField.isBooleanType()) { | ||
| QueryExpression qe = QueryExpression.create(namedField).isTrue(); | ||
| qe.updateAnalyzedNodes(node); | ||
| return qe; | ||
| } | ||
| QueryExpression qe = (QueryExpression) expr; | ||
| if (!qe.isPartial()) { | ||
|
|
@@ -1057,6 +1084,10 @@ QueryExpression isTrue() { | |
| throw new PredicateAnalyzerException("isTrue cannot be applied to " + this.getClass()); | ||
| } | ||
|
|
||
| QueryExpression isFalse() { | ||
| throw new PredicateAnalyzerException("isFalse cannot be applied to " + this.getClass()); | ||
| } | ||
|
|
||
| QueryExpression in(LiteralExpression literal) { | ||
| throw new PredicateAnalyzerException("in cannot be applied to " + this.getClass()); | ||
| } | ||
|
|
@@ -1182,6 +1213,8 @@ static class SimpleQueryExpression extends QueryExpression { | |
| private RexNode analyzedRexNode; | ||
| private final NamedFieldExpression rel; | ||
| private QueryBuilder builder; | ||
| // Flag indicating this expression represents IS_TRUE on a boolean field | ||
| private boolean isBooleanFieldIsTrue = false; | ||
|
|
||
| private String getFieldReference() { | ||
| return rel.getReference(); | ||
|
|
@@ -1393,11 +1426,29 @@ public QueryExpression multiMatch( | |
|
|
||
| @Override | ||
| public QueryExpression isTrue() { | ||
| // Ignore istrue if ISTRUE(predicate) and will support ISTRUE(field) later. | ||
| // builder = termQuery(getFieldReferenceForTermQuery(), true); | ||
| // When IS_TRUE is called on a boolean field directly (e.g., IS_TRUE(field)), | ||
| // generate a term query with value true. | ||
| // When called on an already-evaluated predicate (builder already set), | ||
| // return as-is. | ||
| if (builder == null) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to override |
||
| builder = termQuery(getFieldReferenceForTermQuery(), true); | ||
| isBooleanFieldIsTrue = true; | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public QueryExpression isFalse() { | ||
| // When IS_FALSE or NOT(IS_TRUE) is called on a boolean field, | ||
| // generate a term query with value false. | ||
| builder = termQuery(getFieldReferenceForTermQuery(), false); | ||
| return this; | ||
| } | ||
|
|
||
| boolean isBooleanFieldIsTrue() { | ||
| return isBooleanFieldIsTrue; | ||
| } | ||
|
|
||
| @Override | ||
| public QueryExpression in(LiteralExpression literal) { | ||
| Collection<?> collection = (Collection<?>) literal.value(); | ||
|
|
@@ -1659,6 +1710,17 @@ boolean isTextType() { | |
| return type != null && type.getOriginalExprType() instanceof OpenSearchTextType; | ||
| } | ||
|
|
||
| boolean isBooleanType() { | ||
| if (type == null) { | ||
| return false; | ||
| } | ||
| // Check if the type is a boolean type. For OpenSearchDataType, check exprCoreType. | ||
| if (type instanceof OpenSearchDataType osType) { | ||
| return ExprCoreType.BOOLEAN.equals(osType.getExprCoreType()); | ||
| } | ||
| return ExprCoreType.BOOLEAN.equals(type); | ||
| } | ||
|
|
||
| boolean isMetaField() { | ||
| return OpenSearchConstants.METADATAFIELD_TYPE_MAP.containsKey(getRootName()); | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using failed query
source=test url=http | where is_internal=truein #5054