Skip to content

Commit c85ed67

Browse files
committed
fix tests
Signed-off-by: Jialiang Liang <jiallian@amazon.com>
1 parent 8521325 commit c85ed67

5 files changed

Lines changed: 99 additions & 30 deletions

File tree

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

Lines changed: 85 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -182,8 +182,8 @@ public RelNode analyze(UnresolvedPlan unresolved, CalcitePlanContext context) {
182182
context.enableFilterAccumulation();
183183
try {
184184
unresolved.accept(this, context);
185-
context.flushFilterConditions(); // Flush accumulated conditions before returning
186-
return context.relBuilder.peek(); // Get the result after flushing
185+
context.flushFilterConditions();
186+
return context.relBuilder.peek();
187187
} finally {
188188
context.disableFilterAccumulation();
189189
}
@@ -192,6 +192,17 @@ public RelNode analyze(UnresolvedPlan unresolved, CalcitePlanContext context) {
192192
}
193193
}
194194

195+
/**
196+
* Flushes accumulated filter conditions before schema-changing operations. This prevents
197+
* RexInputRef index mismatches that occur when filters reference field indices from the old
198+
* schema.
199+
*/
200+
private void flushFiltersBeforeSchemaChange(CalcitePlanContext context) {
201+
if (context.isFilterAccumulationEnabled() && context.hasPendingFilterConditions()) {
202+
context.flushFilterConditions();
203+
}
204+
}
205+
195206
@Override
196207
public RelNode visitRelation(Relation node, CalcitePlanContext context) {
197208
DataSourceSchemaIdentifierNameResolver nameResolver =
@@ -407,10 +418,7 @@ private boolean containsSubqueryExpression(Node expr) {
407418
public RelNode visitProject(Project node, CalcitePlanContext context) {
408419
visitChildren(node, context);
409420

410-
// Flush accumulated filter conditions before schema-changing operations
411-
if (context.isFilterAccumulationEnabled() && context.hasPendingFilterConditions()) {
412-
context.flushFilterConditions();
413-
}
421+
flushFiltersBeforeSchemaChange(context);
414422

415423
if (isSingleAllFieldsProject(node)) {
416424
return handleAllFieldsProject(node, context);
@@ -886,6 +894,9 @@ public RelNode visitPatterns(Patterns node, CalcitePlanContext context) {
886894
@Override
887895
public RelNode visitEval(Eval node, CalcitePlanContext context) {
888896
visitChildren(node, context);
897+
898+
flushFiltersBeforeSchemaChange(context);
899+
889900
node.getExpressionList()
890901
.forEach(
891902
expr -> {
@@ -1155,6 +1166,9 @@ private Pair<List<RexNode>, List<AggCall>> resolveAttributesForAggregation(
11551166
/** Visits an aggregation for stats command */
11561167
@Override
11571168
public RelNode visitAggregation(Aggregation node, CalcitePlanContext context) {
1169+
// Flush accumulated filter conditions before schema-changing aggregation operations
1170+
flushFiltersBeforeSchemaChange(context);
1171+
11581172
Argument.ArgumentMap statsArgs = Argument.ArgumentMap.of(node.getArgExprList());
11591173
Boolean bucketNullable = (Boolean) statsArgs.get(Argument.BUCKET_NULLABLE).getValue();
11601174
int nGroup = node.getGroupExprList().size() + (Objects.nonNull(node.getSpan()) ? 1 : 0);
@@ -2273,10 +2287,26 @@ private RelNode mergeTableAndResolveColumnConflict(
22732287
@Override
22742288
public RelNode visitMultisearch(Multisearch node, CalcitePlanContext context) {
22752289
List<RelNode> subsearchNodes = new ArrayList<>();
2290+
// Save the current filter accumulation state - we'll process each subsearch independently
2291+
boolean wasFilterAccumulationEnabled = context.isFilterAccumulationEnabled();
2292+
22762293
for (UnresolvedPlan subsearch : node.getSubsearches()) {
22772294
UnresolvedPlan prunedSubSearch = subsearch.accept(new EmptySourcePropagateVisitor(), null);
2278-
prunedSubSearch.accept(this, context);
2295+
2296+
// Temporarily disable filter accumulation so each subsearch gets its own independent
2297+
// lifecycle via analyze(). This prevents filter state from bleeding across branches.
2298+
if (wasFilterAccumulationEnabled) {
2299+
context.disableFilterAccumulation();
2300+
}
2301+
2302+
// Use analyze() to let each subsearch determine its own filter accumulation needs
2303+
analyze(prunedSubSearch, context);
22792304
subsearchNodes.add(context.relBuilder.build());
2305+
2306+
// Restore filter accumulation state for the next iteration
2307+
if (wasFilterAccumulationEnabled) {
2308+
context.enableFilterAccumulation();
2309+
}
22802310
}
22812311

22822312
// Use shared schema merging logic that handles type conflicts via field renaming
@@ -3273,8 +3303,12 @@ private RexNode createOptimizedTransliteration(
32733303
* RelNodes. This is used to detect queries with multiple regex/filter operations that could cause
32743304
* deep Filter RelNode chains and memory exhaustion.
32753305
*
3306+
* <p>Stops counting at schema-changing operations (like Aggregation, Project with computed
3307+
* expressions) to avoid enabling filter accumulation across schema boundaries, which would cause
3308+
* RexInputRef index mismatches.
3309+
*
32763310
* @param plan the UnresolvedPlan to analyze
3277-
* @return the count of filtering operations found
3311+
* @return the count of filtering operations found before the first schema-changing operation
32783312
*/
32793313
private int countFilteringOperations(UnresolvedPlan plan) {
32803314
if (plan == null) {
@@ -3284,8 +3318,25 @@ private int countFilteringOperations(UnresolvedPlan plan) {
32843318
int count = 0;
32853319

32863320
// Count this node if it's a filtering operation
3287-
if (plan instanceof Regex || plan instanceof Filter) {
3321+
// BUT: Don't count Filter nodes that contain function calls, as they can cause
3322+
// type mismatches when accumulated and flushed later
3323+
if (plan instanceof Regex) {
32883324
count = 1;
3325+
} else if (plan instanceof Filter) {
3326+
Filter filterNode = (Filter) plan;
3327+
if (!containsFunctionCall(filterNode.getCondition())) {
3328+
count = 1;
3329+
}
3330+
}
3331+
3332+
// Stop counting at schema-changing operations to prevent accumulation across schema boundaries
3333+
// Schema-changing operations include: Aggregation, Eval, Project (with computed expressions),
3334+
// Window, StreamWindow, etc.
3335+
if (plan instanceof Aggregation
3336+
|| plan instanceof Eval
3337+
|| plan instanceof Window
3338+
|| plan instanceof StreamWindow) {
3339+
return count; // Don't recurse into children beyond schema changes
32893340
}
32903341

32913342
// Recursively count filtering operations in children
@@ -3299,4 +3350,29 @@ private int countFilteringOperations(UnresolvedPlan plan) {
32993350

33003351
return count;
33013352
}
3353+
3354+
/**
3355+
* Checks if an expression contains any function calls. Filter expressions with function calls can
3356+
* cause type mismatches when accumulated and flushed later, so we exclude them from filter
3357+
* accumulation.
3358+
*/
3359+
private boolean containsFunctionCall(UnresolvedExpression expr) {
3360+
if (expr == null) {
3361+
return false;
3362+
}
3363+
3364+
if (expr instanceof org.opensearch.sql.ast.expression.Function) {
3365+
return true;
3366+
}
3367+
3368+
// Check children recursively
3369+
for (Node child : expr.getChild()) {
3370+
if (child instanceof UnresolvedExpression
3371+
&& containsFunctionCall((UnresolvedExpression) child)) {
3372+
return true;
3373+
}
3374+
}
3375+
3376+
return false;
3377+
}
33023378
}

integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push.yaml

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,7 @@ calcite:
22
logical: |
33
LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
44
LogicalProject(age=[$8])
5-
LogicalFilter(condition=[>($3, 10000)])
6-
LogicalFilter(condition=[<($8, 40)])
7-
LogicalFilter(condition=[>($8, 30)])
8-
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
5+
LogicalFilter(condition=[AND(SEARCH($8, Sarg[(30..40)]), >($3, 10000))])
6+
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
97
physical: |
108
CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[balance, age], FILTER->AND(SEARCH($1, Sarg[(30..40)]), >($0, 10000)), PROJECT->[age], LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","query":{"bool":{"must":[{"range":{"age":{"from":30.0,"to":40.0,"include_lower":false,"include_upper":false,"boost":1.0}}},{"range":{"balance":{"from":10000,"to":null,"include_lower":false,"include_upper":true,"boost":1.0}}}],"adjust_pure_negative":true,"boost":1.0}},"_source":{"includes":["age"],"excludes":[]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)])
Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
calcite:
22
logical: |
33
LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
4-
LogicalFilter(condition=[<($0, DATE('2018-11-09 00:00:00.000000000':VARCHAR))])
5-
LogicalFilter(condition=[>($0, DATE('2016-12-08 00:00:00.123456789':VARCHAR))])
6-
LogicalProject(yyyy-MM-dd=[$83])
7-
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_date_formats]])
4+
LogicalFilter(condition=[AND(>($0, DATE('2016-12-08 00:00:00.123456789':VARCHAR)), <($0, DATE('2018-11-09 00:00:00.000000000':VARCHAR)))])
5+
LogicalProject(yyyy-MM-dd=[$83])
6+
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_date_formats]])
87
physical: |
98
CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_date_formats]], PushDownContext=[[PROJECT->[yyyy-MM-dd], FILTER->SEARCH($0, Sarg[('2016-12-08':VARCHAR..'2018-11-09':VARCHAR)]:VARCHAR), LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","query":{"range":{"yyyy-MM-dd":{"from":"2016-12-08","to":"2018-11-09","include_lower":false,"include_upper":false,"boost":1.0}}},"_source":{"includes":["yyyy-MM-dd"],"excludes":[]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)])

integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push_compare_timestamp_string.yaml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@ calcite:
22
logical: |
33
LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
44
LogicalProject(account_number=[$0], firstname=[$1], address=[$2], birthdate=[$3], gender=[$4], city=[$5], lastname=[$6], balance=[$7], employer=[$8], state=[$9], age=[$10], email=[$11], male=[$12])
5-
LogicalFilter(condition=[<($3, TIMESTAMP('2018-11-09 00:00:00.000000000':VARCHAR))])
6-
LogicalFilter(condition=[>($3, TIMESTAMP('2016-12-08 00:00:00.000000000':VARCHAR))])
7-
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]])
5+
LogicalFilter(condition=[AND(>($3, TIMESTAMP('2016-12-08 00:00:00.000000000':VARCHAR)), <($3, TIMESTAMP('2018-11-09 00:00:00.000000000':VARCHAR)))])
6+
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]])
87
physical: |
98
CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]], PushDownContext=[[PROJECT->[account_number, firstname, address, birthdate, gender, city, lastname, balance, employer, state, age, email, male], FILTER->SEARCH($3, Sarg[('2016-12-08 00:00:00':VARCHAR..'2018-11-09 00:00:00':VARCHAR)]:VARCHAR), LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","query":{"range":{"birthdate":{"from":"2016-12-08T00:00:00.000Z","to":"2018-11-09T00:00:00.000Z","include_lower":false,"include_upper":false,"format":"date_time","boost":1.0}}},"_source":{"includes":["account_number","firstname","address","birthdate","gender","city","lastname","balance","employer","state","age","email","male"],"excludes":[]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)])

ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLMultisearchTest.java

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -183,29 +183,26 @@ public void testMultisearchWithStats() {
183183
+ " LogicalAggregate(group=[{0}], count=[COUNT()])\n"
184184
+ " LogicalProject(type=[$8])\n"
185185
+ " LogicalUnion(all=[true])\n"
186-
+ " LogicalFilter(condition=[=($7, 10)])\n"
187-
+ " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],"
186+
+ " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],"
188187
+ " SAL=[$5], COMM=[$6], DEPTNO=[$7], type=['accounting':VARCHAR])\n"
188+
+ " LogicalFilter(condition=[=($7, 10)])\n"
189189
+ " LogicalTableScan(table=[[scott, EMP]])\n"
190-
+ " LogicalFilter(condition=[=($7, 20)])\n"
191-
+ " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],"
190+
+ " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],"
192191
+ " SAL=[$5], COMM=[$6], DEPTNO=[$7], type=['research':VARCHAR])\n"
192+
+ " LogicalFilter(condition=[=($7, 20)])\n"
193193
+ " LogicalTableScan(table=[[scott, EMP]])\n";
194194
verifyLogical(root, expectedLogical);
195195

196-
// SparkSQL reflects Filter above Project due to flush logic
197196
String expectedSparkSql =
198197
"SELECT COUNT(*) `count`, `type`\n"
199-
+ "FROM (SELECT *\n"
200198
+ "FROM (SELECT `EMPNO`, `ENAME`, `JOB`, `MGR`, `HIREDATE`, `SAL`, `COMM`, `DEPTNO`,"
201199
+ " 'accounting' `type`\n"
202-
+ "FROM `scott`.`EMP`) `t`\n"
200+
+ "FROM `scott`.`EMP`\n"
203201
+ "WHERE `DEPTNO` = 10\n"
204202
+ "UNION ALL\n"
205-
+ "SELECT *\n"
206-
+ "FROM (SELECT `EMPNO`, `ENAME`, `JOB`, `MGR`, `HIREDATE`, `SAL`, `COMM`, `DEPTNO`,"
203+
+ "SELECT `EMPNO`, `ENAME`, `JOB`, `MGR`, `HIREDATE`, `SAL`, `COMM`, `DEPTNO`,"
207204
+ " 'research' `type`\n"
208-
+ "FROM `scott`.`EMP`) `t1`\n"
205+
+ "FROM `scott`.`EMP`\n"
209206
+ "WHERE `DEPTNO` = 20) `t3`\n"
210207
+ "GROUP BY `type`";
211208
verifyPPLToSparkSQL(root, expectedSparkSql);

0 commit comments

Comments
 (0)