Skip to content

Commit cbf5074

Browse files
committed
Partition partial results on the group key's source fields
Resolve each aggregation group key through the eval Project to the scan fields it reads, so an expression key (e.g. eval g = lower(city) | stats count() by g) gets partial results over the keyword subset just like a bare 'by city'. Previously only a bare group field matched the per-index mapping; a derived key looked up its output alias, found nothing, and bailed to the complete (script) path. A constant group key resolves to no field and cleanly bails. Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent ad3c0a5 commit cbf5074

3 files changed

Lines changed: 82 additions & 7 deletions

File tree

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,30 @@ public void partialResultOnHandlesNestedDottedField() throws IOException {
248248
warning.getString("detail").contains(NESTED_TEXT_INDEX));
249249
}
250250

251+
@Test
252+
public void partialResultOnHandlesEvalDerivedGroupKey() throws IOException {
253+
setPartialResult(true);
254+
setPitContextLimit("1");
255+
// The group key is an expression over the conflicting field (upper(env)), not the bare field.
256+
// Partitioning traces it back to env, so the keyword index is kept and the text index excluded
257+
// just as for a bare group key. Only the keyword index contributes: PROD=2, DEV=1.
258+
JSONObject result =
259+
executeQuery(
260+
String.format(
261+
"source=%s | eval g = upper(env) | stats count() by g | sort g", PATTERN));
262+
verifyDataRows(result, rows(1, "DEV"), rows(2, "PROD"));
263+
264+
assertTrue("response should carry a warnings array", result.has("warnings"));
265+
JSONObject warning = result.getJSONArray("warnings").getJSONObject(0);
266+
assertEquals("PARTIAL_RESULT", warning.getString("type"));
267+
assertTrue(
268+
"warning should name the underlying field the expression reads",
269+
warning.getString("detail").contains("env"));
270+
assertTrue(
271+
"warning should name the excluded text index",
272+
warning.getString("detail").contains(TEXT_INDEX));
273+
}
274+
251275
@Test
252276
public void partialResultKeepsKeywordGroupEvenWhenOutnumbered() throws IOException {
253277
setPartialResult(true);

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,11 @@
77

88
import com.google.common.collect.ImmutableList;
99
import java.util.ArrayList;
10+
import java.util.LinkedHashSet;
1011
import java.util.List;
1112
import java.util.Map;
1213
import java.util.Objects;
14+
import java.util.Set;
1315
import java.util.stream.Collectors;
1416
import javax.annotation.Nullable;
1517
import lombok.Getter;
@@ -34,7 +36,9 @@
3436
import org.apache.calcite.rel.type.RelDataTypeFactory;
3537
import org.apache.calcite.rel.type.RelDataTypeField;
3638
import org.apache.calcite.rex.RexBuilder;
39+
import org.apache.calcite.rex.RexInputRef;
3740
import org.apache.calcite.rex.RexNode;
41+
import org.apache.calcite.rex.RexVisitorImpl;
3842
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
3943
import org.apache.calcite.sql.type.SqlTypeName;
4044
import org.apache.commons.lang3.tuple.Pair;
@@ -414,9 +418,12 @@ private AbstractRelNode pushDownAggregate(
414418
// Try partial mode before analyze: since #5646 a text/keyword conflict pushes down as a slow
415419
// _source script instead of failing, so a post-failure fallback would never fire.
416420
if (allowPartialFallback) {
417-
AbstractRelNode partial = tryPartialResultAggregate(aggregate, project, bucketNames);
418-
if (partial != null) {
419-
return partial;
421+
List<String> partitionFields = resolvePartitionFields(aggregate, project);
422+
if (partitionFields != null) {
423+
AbstractRelNode partial = tryPartialResultAggregate(aggregate, project, partitionFields);
424+
if (partial != null) {
425+
return partial;
426+
}
420427
}
421428
}
422429
int queryBucketSize = osIndex.getQueryBucketSize();
@@ -451,15 +458,58 @@ private AbstractRelNode pushDownAggregate(
451458
return null;
452459
}
453460

461+
/**
462+
* Resolve the aggregation's group keys to the underlying scan fields to partition indices on. A
463+
* key may be a bare field ({@code ... by city}) or an expression over fields ({@code eval g =
464+
* lower(city) | ... by g}); in the latter case we partition on every field the expression reads,
465+
* since a kept index must map all of them aggregatably. The {@code project} (when present) sits
466+
* directly on the scan, so its input refs index into this scan's row type. Returns {@code null}
467+
* if any key is a pure constant with no field to key on.
468+
*/
469+
@Nullable
470+
private List<String> resolvePartitionFields(Aggregate aggregate, @Nullable Project project) {
471+
List<String> scanFields = getRowType().getFieldNames();
472+
List<String> fields = new ArrayList<>();
473+
for (int group : aggregate.getGroupSet()) {
474+
Set<Integer> refs = new LinkedHashSet<>();
475+
if (project == null) {
476+
refs.add(group); // group key indexes directly into the scan
477+
} else {
478+
project
479+
.getProjects()
480+
.get(group)
481+
.accept(
482+
new RexVisitorImpl<Void>(true) {
483+
@Override
484+
public Void visitInputRef(RexInputRef ref) {
485+
refs.add(ref.getIndex());
486+
return null;
487+
}
488+
});
489+
}
490+
if (refs.isEmpty()) {
491+
return null; // constant group key -> nothing to partition on
492+
}
493+
for (int ref : refs) {
494+
String name = scanFields.get(ref);
495+
if (!fields.contains(name)) {
496+
fields.add(name);
497+
}
498+
}
499+
}
500+
return fields;
501+
}
502+
454503
/**
455504
* On a text/keyword mapping conflict, narrow the scan to the index subset where the group field
456505
* is aggregatable, push the aggregation over just that subset, and record a warning naming the
457506
* excluded indices. Only runs behind the opt-in setting and only when the response format can
458507
* carry the warning ({@link QueryContext#isWarningsSupported}); returns {@code null} otherwise.
459-
* Partitioning lives in {@link PartialResultAggregatePushdown}.
508+
* {@code partitionFields} are the scan fields the group keys resolve to (see {@link
509+
* #resolvePartitionFields}). Partitioning lives in {@link PartialResultAggregatePushdown}.
460510
*/
461511
private AbstractRelNode tryPartialResultAggregate(
462-
Aggregate aggregate, @Nullable Project project, List<String> bucketNames) {
512+
Aggregate aggregate, @Nullable Project project, List<String> partitionFields) {
463513
if (!QueryContext.isPartialResultEnabled(osIndex.getSettings())) {
464514
return null;
465515
}
@@ -470,7 +520,7 @@ private AbstractRelNode tryPartialResultAggregate(
470520
try {
471521
Map<String, IndexMapping> mappings = osIndex.getIndexMappings();
472522
PartialResultAggregatePushdown.Plan plan =
473-
PartialResultAggregatePushdown.plan(bucketNames, mappings);
523+
PartialResultAggregatePushdown.plan(partitionFields, mappings);
474524
if (plan == null) {
475525
return null;
476526
}

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ record Plan(List<String> keptIndices, List<String> excludedIndices, Warning warn
5151
/**
5252
* Decide the partial-result plan for a group key over a set of per-index mappings.
5353
*
54-
* @param bucketNames the aggregation's group-by field names (dotted paths)
54+
* @param bucketNames the storage fields the group keys resolve to (dotted paths); an expression
55+
* key like {@code lower(city)} resolves to the field(s) it reads, e.g. {@code city}
5556
* @param mappings per-index field mappings, keyed by concrete index name (from {@code
5657
* getIndexMappings}); the wildcard has already been resolved to concrete indices
5758
* @return a plan naming the kept and excluded indices plus the warning, or {@code null} when

0 commit comments

Comments
 (0)