Skip to content

Commit e4d086a

Browse files
committed
refactor: throw exception if pushdown cannot be applied
Signed-off-by: Lantao Jin <ltjin@amazon.com>
1 parent cbcdbd6 commit e4d086a

24 files changed

Lines changed: 429 additions & 47 deletions

core/src/main/java/org/opensearch/sql/ast/expression/Argument.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
@EqualsAndHashCode(callSuper = false)
2222
public class Argument extends UnresolvedExpression {
2323
public static final String BUCKET_NULLABLE = "bucket_nullable";
24+
public static final String NESTED = "nested";
2425

2526
private final String argName;
2627
private final Literal value;

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

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1047,7 +1047,9 @@ private Pair<List<RexNode>, List<AggCall>> aggregateWithTrimming(
10471047
// \- Scan t
10481048
List<RexInputRef> trimmedRefs = new ArrayList<>();
10491049
trimmedRefs.addAll(PlanUtils.getInputRefs(resolvedGroupByList)); // group-by keys first
1050-
trimmedRefs.addAll(PlanUtils.getInputRefsFromAggCall(resolvedAggCallList));
1050+
List<RexInputRef> aggCallRefs = PlanUtils.getInputRefsFromAggCall(resolvedAggCallList);
1051+
trimmedRefs.addAll(aggCallRefs);
1052+
List<Boolean> nestedList = isNestedAggregation(context.relBuilder, aggCallRefs);
10511053
context.relBuilder.project(trimmedRefs);
10521054

10531055
// Re-resolve all attributes based on adding trimmed Project.
@@ -1060,6 +1062,9 @@ private Pair<List<RexNode>, List<AggCall>> aggregateWithTrimming(
10601062
context.relBuilder.aggregate(
10611063
context.relBuilder.groupKey(reResolved.getLeft()), reResolved.getRight());
10621064
if (hintBucketNonNull) PlanUtils.addIgnoreNullBucketHintToAggregate(context.relBuilder);
1065+
if (nestedList.stream().anyMatch(b -> b)) {
1066+
PlanUtils.addNestedHintToAggregate(context.relBuilder, nestedList);
1067+
}
10631068
// During aggregation, Calcite projects both input dependencies and output group-by fields.
10641069
// When names conflict, Calcite adds numeric suffixes (e.g., "value0").
10651070
// Apply explicit renaming to restore the intended aliases.
@@ -1068,6 +1073,18 @@ private Pair<List<RexNode>, List<AggCall>> aggregateWithTrimming(
10681073
return Pair.of(reResolved.getLeft(), reResolved.getRight());
10691074
}
10701075

1076+
/**
1077+
* Return a list of the neste aggregation flag. For example: aggCalls: [count(), count(a.b),
1078+
* avg(a.c)] -> aggCallRefs [1, 2] -> nestedList [true, true]
1079+
*/
1080+
private List<Boolean> isNestedAggregation(RelBuilder relBuilder, List<RexInputRef> aggCallRefs) {
1081+
return aggCallRefs.stream()
1082+
.map(r -> relBuilder.peek().getRowType().getFieldNames().get(r.getIndex()))
1083+
.map(name -> org.apache.commons.lang3.StringUtils.substringBefore(name, "."))
1084+
.map(root -> relBuilder.field(root).getType().getSqlTypeName() == SqlTypeName.ARRAY)
1085+
.toList();
1086+
}
1087+
10711088
/**
10721089
* Imitates {@code Registrar.registerExpression} of {@link RelBuilder} to derive the output order
10731090
* of group-by keys after aggregation.

core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@
5252
import org.apache.calcite.jdbc.CalcitePrepare;
5353
import org.apache.calcite.jdbc.CalciteSchema;
5454
import org.apache.calcite.jdbc.Driver;
55-
import org.apache.calcite.linq4j.function.Function0;
5655
import org.apache.calcite.plan.Context;
5756
import org.apache.calcite.plan.Contexts;
5857
import org.apache.calcite.plan.Convention;
@@ -175,8 +174,11 @@ public Connection connect(
175174
}
176175

177176
@Override
178-
protected Function0<CalcitePrepare> createPrepareFactory() {
179-
return OpenSearchPrepareImpl::new;
177+
public CalcitePrepare createPrepare() {
178+
if (prepareFactory != null) {
179+
return prepareFactory.get();
180+
}
181+
return new OpenSearchPrepareImpl();
180182
}
181183
}
182184

@@ -298,10 +300,10 @@ public OpenSearchCalcitePreparingStmt(
298300

299301
@Override
300302
protected PreparedResult implement(RelRoot root) {
301-
Hook.PLAN_BEFORE_IMPLEMENTATION.run(root);
302-
RelDataType resultType = root.rel.getRowType();
303-
boolean isDml = root.kind.belongsTo(SqlKind.DML);
304303
if (root.rel instanceof Scannable scannable) {
304+
Hook.PLAN_BEFORE_IMPLEMENTATION.run(root);
305+
RelDataType resultType = root.rel.getRowType();
306+
boolean isDml = root.kind.belongsTo(SqlKind.DML);
305307
final Bindable bindable = dataContext -> scannable.scan();
306308

307309
return new PreparedResultImpl(

core/src/main/java/org/opensearch/sql/calcite/utils/PPLHintStrategyTable.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,12 @@ public class PPLHintStrategyTable {
1919
() ->
2020
HintStrategyTable.builder()
2121
.hintStrategy(
22-
"stats_args",
22+
"agg_args",
23+
(hint, rel) -> {
24+
return rel instanceof LogicalAggregate;
25+
})
26+
.hintStrategy(
27+
"nested_agg",
2328
(hint, rel) -> {
2429
return rel instanceof LogicalAggregate;
2530
})

core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@
6464
import org.opensearch.sql.calcite.CalcitePlanContext;
6565
import org.opensearch.sql.expression.function.BuiltinFunctionName;
6666
import org.opensearch.sql.expression.function.PPLFuncImpTable;
67+
import org.opensearch.sql.utils.Utils;
6768

6869
public interface PlanUtils {
6970

@@ -603,11 +604,30 @@ static void replaceTop(RelBuilder relBuilder, RelNode relNode) {
603604
}
604605

605606
static void addIgnoreNullBucketHintToAggregate(RelBuilder relBuilder) {
606-
final RelHint statHits =
607-
RelHint.builder("stats_args").hintOption(Argument.BUCKET_NULLABLE, "false").build();
608607
assert relBuilder.peek() instanceof LogicalAggregate
609608
: "Stats hits should be added to LogicalAggregate";
610-
relBuilder.hints(statHits);
609+
final RelHint statHint =
610+
RelHint.builder("agg_args").hintOption(Argument.BUCKET_NULLABLE, "false").build();
611+
relBuilder.hints(statHint);
612+
relBuilder.getCluster().setHintStrategies(PPLHintStrategyTable.getHintStrategyTable());
613+
}
614+
615+
static void addNestedHintToAggregate(RelBuilder relBuilder, List<Boolean> nestedList) {
616+
assert relBuilder.peek() instanceof LogicalAggregate
617+
: "Stats hits should be added to LogicalAggregate";
618+
LogicalAggregate aggregate = (LogicalAggregate) relBuilder.peek();
619+
List<Integer> indicesWithArgList =
620+
Utils.zipWithIndex(aggregate.getAggCallList()).stream()
621+
.filter(p -> !p.getKey().getArgList().isEmpty())
622+
.map(org.apache.commons.lang3.tuple.Pair::getValue)
623+
.toList();
624+
assert indicesWithArgList.size() == nestedList.size();
625+
RelHint.Builder builder = RelHint.builder("nested_agg");
626+
for (int i = 0; i < indicesWithArgList.size(); i++) {
627+
builder.hintOption(indicesWithArgList.get(i).toString(), nestedList.get(i).toString());
628+
}
629+
final RelHint statHint = builder.build();
630+
relBuilder.hints(statHint);
611631
relBuilder.getCluster().setHintStrategies(PPLHintStrategyTable.getHintStrategyTable());
612632
}
613633

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.utils;
7+
8+
import java.util.Iterator;
9+
import java.util.LinkedList;
10+
import java.util.List;
11+
import org.apache.commons.lang3.tuple.Pair;
12+
13+
public interface Utils {
14+
static <I> List<Pair<I, Integer>> zipWithIndex(List<I> input) {
15+
LinkedList<Pair<I, Integer>> result = new LinkedList<>();
16+
Iterator<I> iter = input.iterator();
17+
int index = 0;
18+
while (iter.hasNext()) {
19+
result.add(Pair.of(iter.next(), index++));
20+
}
21+
return result;
22+
}
23+
}

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,13 @@
1919
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_WORK_INFORMATION;
2020
import static org.opensearch.sql.util.MatcherUtils.assertJsonEqualsIgnoreId;
2121
import static org.opensearch.sql.util.MatcherUtils.assertYamlEqualsIgnoreId;
22+
import static org.opensearch.sql.util.MatcherUtils.verifyErrorMessageContains;
2223

2324
import java.io.IOException;
2425
import java.util.Locale;
2526
import org.junit.Ignore;
2627
import org.junit.Test;
28+
import org.opensearch.client.ResponseException;
2729
import org.opensearch.sql.common.setting.Settings;
2830
import org.opensearch.sql.ppl.ExplainIT;
2931

@@ -2215,4 +2217,33 @@ public void testRexStandardizationForScript() throws IOException {
22152217
TEST_INDEX_BANK),
22162218
true));
22172219
}
2220+
2221+
@Test
2222+
public void testNestedAggPushDownExplain() throws Exception {
2223+
enabledOnlyWhenPushdownIsEnabled();
2224+
String expected = loadExpectedPlan("explain_nested_agg_push.yaml");
2225+
2226+
assertYamlEqualsIgnoreId(
2227+
expected,
2228+
explainQueryYaml(
2229+
"source=opensearch-sql_test_index_nested_simple | stats count(address.area) as"
2230+
+ " count_area, min(address.area) as min_area, max(address.area) as max_area,"
2231+
+ " avg(address.area) as avg_area, avg(age) as avg_age by name"));
2232+
}
2233+
2234+
@Test
2235+
public void testNestedAggExplainWhenPushdownNotApplied() throws Exception {
2236+
enabledOnlyWhenPushdownIsEnabled();
2237+
Exception e =
2238+
assertThrows(
2239+
ResponseException.class,
2240+
() ->
2241+
explainQueryYaml(
2242+
"source=opensearch-sql_test_index_nested_simple | head 10000 | stats"
2243+
+ " count(address.area) as count_area, min(address.area) as min_area,"
2244+
+ " max(address.area) as max_area, avg(address.area) as avg_area, avg(age)"
2245+
+ " as avg_age by name"));
2246+
verifyErrorMessageContains(
2247+
e, "Nested aggregate is unsupported when pushdown cannot be applied");
2248+
}
22182249
}

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStatsCommandIT.java

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,15 @@
77

88
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT;
99
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK;
10+
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_NESTED_SIMPLE;
1011
import static org.opensearch.sql.util.MatcherUtils.rows;
12+
import static org.opensearch.sql.util.MatcherUtils.schema;
1113
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;
14+
import static org.opensearch.sql.util.MatcherUtils.verifySchemaInOrder;
1215

1316
import java.io.IOException;
1417
import org.json.JSONObject;
18+
import org.junit.Ignore;
1519
import org.junit.jupiter.api.Test;
1620
import org.opensearch.sql.ppl.StatsCommandIT;
1721

@@ -21,6 +25,8 @@ public void init() throws Exception {
2125
super.init();
2226
enableCalcite();
2327
setQuerySizeLimit(2000);
28+
29+
loadIndex(Index.NESTED_SIMPLE);
2430
}
2531

2632
@Test
@@ -106,4 +112,60 @@ public void testPaginatingStatsForHeadFrom() throws IOException {
106112
resetQueryBucketSize();
107113
}
108114
}
115+
116+
@Test
117+
public void testNestedAggregation() throws IOException {
118+
JSONObject actual =
119+
executeQuery(
120+
String.format(
121+
"source=%s | stats count(address.area) as count_area, min(address.area) as"
122+
+ " min_area, max(address.area) as max_area, avg(address.area) as avg_area,"
123+
+ " avg(age) as avg_age",
124+
TEST_INDEX_NESTED_SIMPLE));
125+
verifySchemaInOrder(
126+
actual,
127+
isCalciteEnabled() ? schema("count_area", "bigint") : schema("count_area", "int"),
128+
schema("min_area", "double"),
129+
schema("max_area", "double"),
130+
schema("avg_area", "double"),
131+
schema("avg_age", "double"));
132+
verifyDataRows(actual, rows(9, 9.99, 1000.99, 300.11555555555555, 25.2));
133+
}
134+
135+
@Test
136+
public void testNestedAggregationBy() throws IOException {
137+
JSONObject actual =
138+
executeQuery(
139+
String.format(
140+
"source=%s | stats count(address.area) as count_area, min(address.area) as"
141+
+ " min_area, max(address.area) as max_area, avg(address.area) as avg_area,"
142+
+ " avg(age) as avg_age by name",
143+
TEST_INDEX_NESTED_SIMPLE));
144+
verifySchemaInOrder(
145+
actual,
146+
isCalciteEnabled() ? schema("count_area", "bigint") : schema("count_area", "int"),
147+
schema("min_area", "double"),
148+
schema("max_area", "double"),
149+
schema("avg_area", "double"),
150+
schema("avg_age", "double"),
151+
schema("name", "string"));
152+
verifyDataRows(
153+
actual,
154+
rows(4, 10.24, 400.99, 209.69, 24, "abbas"),
155+
rows(0, null, null, null, 19, "andy"),
156+
rows(2, 9.99, 1000.99, 505.49, 32, "chen"),
157+
rows(1, 190.5, 190.5, 190.5, 25, "david"),
158+
rows(2, 231.01, 429.79, 330.4, 26, "peng"));
159+
}
160+
161+
@Ignore("https://github.com/opensearch-project/sql/issues/3384")
162+
public void testNestedAggregationBySpan() throws IOException {
163+
JSONObject actual =
164+
executeQuery(
165+
String.format(
166+
"source=%s | stats count(address.area) as count_area, min(address.area) as"
167+
+ " min_area, max(address.area) as max_area, avg(address.area) as avg_area,"
168+
+ " avg(age) as avg_age by span(age, 10)",
169+
TEST_INDEX_NESTED_SIMPLE));
170+
}
109171
}

0 commit comments

Comments
 (0)