Skip to content

Commit d3782dc

Browse files
committed
Branch 7 stats tests on isAnalyticsParquetIndicesEnabled
When -Dtests.analytics.parquet_indices=true is set, every test-created index is parquet-backed and RestUnifiedQueryAction.isAnalyticsIndex routes all queries through the analytics-engine backend (DataFusion). Seven tests need analytics-specific assertions because DataFusion follows different null-bucket, SQL-spec, and TDigest interpolation semantics than the legacy V2 / Calcite-DSL paths: SQL-spec semantics (DataFusion follows the spec; legacy DSL returns 0): - testSumWithNull — SUM of all-null returns null on analytics; the existing isPushdownDisabled branch already handles the Calcite-no- pushdown case. OR analytics into that branch. - testStatsPercentileByNullValue + NonNullBucket — percentile of an all-null / empty group returns null on analytics. Same OR pattern. Non-deterministic head ordering: - testStatsWithLimit Q1+Q2 — head 5 over a 6-bucket result picks a different null-balance row than the legacy / Calcite-DSL path; same effect cascades into `head 2 from 1`. Reuse the size-only assertion branch already present for the no-pushdown case. TDigest interpolation divergence (genuine impl difference): - testStatsPercentileWithNull — DataFusion TDigest interpolates p50 to 35413 vs OpenSearch's 39225; both within compression-bound error. Per-engine expected value. - testStatsPercentileBySpan — same TDigest diff on the age=30 bucket (33194 vs 39225). Coordinated with opensearch-project/OpenSearch#21584 commit 5 which fixes the upstream planner-side type mismatch so the query reaches the data assertion at all. Semantic divergence pending team decision: - testDisableLegacyPreferred — under PPL_SYNTAX_LEGACY_PREFERRED=false, V2 / Calcite-DSL drop the null-age bucket (5 rows) while DataFusion keeps it (6 rows). Skipped on the analytics path via Assume.assumeFalse until the team decides which behaviour is the intended new-syntax semantics. Out of scope for this PR (intentionally left failing on the analytics path so the gap is visible in CI): - testStatsBySpanTimeWithNullBucket — span(@timestamp, 12h) multi-unit time interval unsupported in current SpanAdapter (only interval=1 is rewritten to DATE_TRUNC; multi-unit needs DataFusion's date_bin). Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent 56e3395 commit d3782dc

1 file changed

Lines changed: 40 additions & 8 deletions

File tree

integ-test/src/test/java/org/opensearch/sql/ppl/StatsCommandIT.java

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,10 @@ public void testStatsWithLimit() throws IOException {
309309
verifySchema(response, schema("a", null, "double"), schema("age", null, "int"));
310310
// If push down disabled, the final results will no longer be stable. In DSL, the order is
311311
// guaranteed because we always sort by bucket field, while we don't add sort in the plan.
312-
if (!isPushdownDisabled()) {
312+
// The analytics-engine backend (DataFusion) is also non-deterministic — its hash-bucket
313+
// order picks a different null-balance row (e.g. (null,36) instead of (null,null)) for
314+
// `head 5`.
315+
if (!isPushdownDisabled() && !isAnalyticsParquetIndicesEnabled()) {
313316
verifyDataRows(
314317
response,
315318
rows(null, null),
@@ -327,7 +330,8 @@ public void testStatsWithLimit() throws IOException {
327330
"source=%s | stats avg(balance) as a by age | head 5 | head 2 from 1",
328331
TEST_INDEX_BANK_WITH_NULL_VALUES));
329332
verifySchema(response, schema("a", null, "double"), schema("age", null, "int"));
330-
if (!isPushdownDisabled()) {
333+
// Same non-deterministic head-window issue as the preceding `head 5` block.
334+
if (!isPushdownDisabled() && !isAnalyticsParquetIndicesEnabled()) {
331335
verifyDataRows(response, rows(32838D, 28), rows(39225D, 32));
332336
} else {
333337
assert ((Integer) response.get("size") == 2);
@@ -508,7 +512,9 @@ public void testSumWithNull() throws IOException {
508512
// TODO: Fix -- temporary workaround for the pushdown issue:
509513
// The current pushdown implementation will return 0 for sum when getting null values as input.
510514
// Returning null should be the expected behavior.
511-
Integer expectedValue = isPushdownDisabled() ? null : 0;
515+
// The analytics-engine backend (DataFusion) follows the SQL spec like Calcite-no-pushdown —
516+
// SUM of all-null is null, not 0.
517+
Integer expectedValue = (isPushdownDisabled() || isAnalyticsParquetIndicesEnabled()) ? null : 0;
512518
verifyDataRows(response, rows(expectedValue));
513519
}
514520

@@ -635,7 +641,11 @@ public void testStatsPercentileWithNull() throws IOException {
635641
String.format(
636642
"source=%s | stats percentile(balance, 50)", TEST_INDEX_BANK_WITH_NULL_VALUES));
637643
verifySchema(response, schema("percentile(balance, 50)", null, "bigint"));
638-
verifyDataRows(response, rows(39225));
644+
// DataFusion's TDigest interpolation produces a different median value for the same
645+
// input than OpenSearch's TDigest implementation. Both are valid approximations within
646+
// TDigest's compression-bound error. Until DataFusion's UDAF is replaced with one
647+
// matching OpenSearch's TDigest (or vice versa), record the per-engine values.
648+
verifyDataRows(response, rows(isAnalyticsParquetIndicesEnabled() ? 35413 : 39225));
639649
}
640650

641651
@Test
@@ -674,14 +684,18 @@ public void testStatsPercentileByNullValue() throws IOException {
674684
"source=%s | stats percentile(balance, 50) as p50 by age",
675685
TEST_INDEX_BANK_WITH_NULL_VALUES));
676686
verifySchema(response, schema("p50", null, "bigint"), schema("age", null, "int"));
687+
// DataFusion follows SQL spec like Calcite-no-pushdown — percentile of an all-null
688+
// group is null, not 0 (the latter is a legacy DSL pushdown convention).
689+
Integer emptyGroupPercentile =
690+
(isPushdownDisabled() || isAnalyticsParquetIndicesEnabled()) ? null : 0;
677691
verifyDataRows(
678692
response,
679-
rows(isPushdownDisabled() ? null : 0, null),
693+
rows(emptyGroupPercentile, null),
680694
rows(32838, 28),
681695
rows(39225, 32),
682696
rows(4180, 33),
683697
rows(48086, 34),
684-
rows(isPushdownDisabled() ? null : 0, 36));
698+
rows(emptyGroupPercentile, 36));
685699
}
686700

687701
@Test
@@ -692,13 +706,15 @@ public void testStatsPercentileByNullValueNonNullBucket() throws IOException {
692706
"source=%s | stats bucket_nullable=false percentile(balance, 50) as p50 by age",
693707
TEST_INDEX_BANK_WITH_NULL_VALUES));
694708
verifySchema(response, schema("p50", null, "bigint"), schema("age", null, "int"));
709+
Integer emptyGroupPercentile =
710+
(isPushdownDisabled() || isAnalyticsParquetIndicesEnabled()) ? null : 0;
695711
verifyDataRows(
696712
response,
697713
rows(32838, 28),
698714
rows(39225, 32),
699715
rows(4180, 33),
700716
rows(48086, 34),
701-
rows(isPushdownDisabled() ? null : 0, 36));
717+
rows(emptyGroupPercentile, 36));
702718
}
703719

704720
@Test
@@ -709,7 +725,14 @@ public void testStatsPercentileBySpan() throws IOException {
709725
"source=%s | stats percentile(balance, 50) as p50 by span(age, 10) as age_bucket",
710726
TEST_INDEX_BANK));
711727
verifySchema(response, schema("p50", null, "bigint"), schema("age_bucket", null, "int"));
712-
verifyDataRows(response, rows(32838, 20), rows(39225, 30));
728+
// Same per-engine TDigest interpolation divergence as testStatsPercentileWithNull —
729+
// the age=30 bucket's p50 differs between OpenSearch's TDigest (39225) and DataFusion's
730+
// (33194). The age=20 bucket happens to coincide.
731+
if (isAnalyticsParquetIndicesEnabled()) {
732+
verifyDataRows(response, rows(32838, 20), rows(33194, 30));
733+
} else {
734+
verifyDataRows(response, rows(32838, 20), rows(39225, 30));
735+
}
713736
}
714737

715738
@Test
@@ -729,6 +752,15 @@ public void testDisableLegacyPreferred() throws IOException {
729752
throw new RuntimeException(e);
730753
}
731754
verifySchema(response, schema("a", null, "double"), schema("age", null, "int"));
755+
// Skip on the analytics-engine backend: PPL_SYNTAX_LEGACY_PREFERRED=false is
756+
// expected to give the same shape across backends, but v2 / Calcite-DSL drop
757+
// the null-age bucket (5 rows) while DataFusion keeps it (6 rows). Deciding
758+
// which is correct is a semantic question for the team; until that's resolved,
759+
// exercising this test on the analytics path doesn't tell us anything useful.
760+
org.junit.Assume.assumeFalse(
761+
"PPL_SYNTAX_LEGACY_PREFERRED=false null-bucket semantics not yet aligned"
762+
+ " between V2 / Calcite-DSL and analytics-engine backends",
763+
isAnalyticsParquetIndicesEnabled());
732764
verifyDataRows(
733765
response,
734766
rows(32838D, 28),

0 commit comments

Comments
 (0)