Commit cea1b65
authored
[analytics-engine] Wire PPL eventstats / streamstats commands end-to-end (#21734)
* [analytics-engine] Allow PARTITION BY in window functions; add ROW_NUMBER
Build directly on #21668's framework: cost gate on OpenSearchProject already
forces SINGLETON input on any RexOver-bearing project. With all rows on the
coordinator, WindowAggExec computes per-partition windows correctly regardless
of whether partition keys span shards. HASH-shuffle parallel execution remains
a future strict improvement, not a correctness prerequisite.
Changes:
- OpenSearchProjectRule.collectWindowFunctions: drop the partition-by rejection
branch. ORDER BY was already unchecked (good: streamstats running window uses
it).
- WindowFunction enum: add ROW_NUMBER. PPL streamstats … by … emits a helper
__row_number_for_streamstats__ column via ROW_NUMBER() OVER ROWS UNBOUNDED.
Other ranking functions (RANK / DENSE_RANK) are not yet emitted by the
PPL paths exercised by analytics-engine.
- DataFusionAnalyticsBackendPlugin.windowCapabilities: add ROW_NUMBER to the
declared set so the backend is viable for streamstats … by ….
- WindowPlanShapeTests.testRexOverPartitionBy_rejected → renamed to
testRexOverPartitionBy_2shard, asserting the cost-gate now produces the
same SINGLETON-gather-then-project shape as the empty OVER case.
Unblocks PPL eventstats … by <field> and the simpler streamstats forms (no
reset / no global+window+by — those use a self-join lowering that doesn't
flow through RexOver and aren't covered by this PR).
Signed-off-by: Songkan Tang <songkant@amazon.com>
* [QA] Port EventstatsCommandIT + StreamstatsCommandIT from sql repo
One-to-one ports of CalcitePPLEventstatsIT and CalciteStreamstatsCommandIT
from opensearch-project/sql. Every @test method here corresponds to a method
of the same name in the source IT.
Schema is translated to calcs (the only fixture this QA module provisions):
age → int0 (11 non-null + 6 nulls — gives the null-bubbling coverage
that the sql IT gets from explicit null partition rows)
country → str0 (3 distinct: FURNITURE, OFFICE SUPPLIES, TECHNOLOGY)
state → str3 (e + null)
name → key (unique row id)
Reachable cases assert exact rows. Cases that depend on functionality not yet
wired through the analytics-engine route are expectThrows negative tests
pinning the precise error fragment — failing those is how a future PR observes
that they should be promoted to passing assertions.
EventstatsCommandIT (28 cases mirroring sql IT):
- 12 reachable: testEventstats, *WithNull, *By, *ByWithNull, *ByWithNullBucket,
*MultipleEventstats(WithNull|WithNullBucket|WithEval), *EmptyRows,
*Variance, *VarianceWithNull, *VarianceBy, *VarianceWithNullBy
- 16 expectThrows: 7 span()-blocked cases, 1 percentile rejection, 1 dc, 1
distinct_count, 1 dc-with-null, 1 earliest/latest, 1 variance-by-span,
3 multiple-partition-by-span variants
StreamstatsCommandIT (47 cases mirroring sql IT):
- ~17 reachable: basic streamstats with/without by, current=false, window=N,
bigWindow, currentAndWindow, multiple chained, streamstatsAndEventstats,
streamstatsAndSort, multipleWithEval (1+2), variance + varianceBy
- ~30 expectThrows: span()-blocked cases, dc / distinct_count / earliest /
latest / percentile not in WindowFunction enum, reset_before/after and
global+window+by which use self-join lowering not flowing through RexOver,
leftJoinWithStreamstats / whereInWithStreamstatsSubquery (composed shapes
not exercised on the route).
Streamstats running aggregates are row-order dependent. Every reachable test
inserts | sort key as the first stage to pin output to a deterministic row
order (calcs has 17 rows; without the sort, parallel scan can return them in
any order, making running counts/avgs non-deterministic).
Both files compile clean against the analytics-engine plugin. No IT actually
runs in this commit — local cluster is unavailable due to a pre-existing
dataformat-native cargo/protoc gap. Marc / verifier should run against a
working cluster to confirm the reachable-vs-throws split matches the comment
predictions.
Signed-off-by: Songkan Tang <songkant@amazon.com>
* [analytics-engine] Plan-shape tests for ROW_NUMBER + PARTITION BY ORDER BY
Three additions to WindowPlanShapeTests pin behaviour of the relaxation in
the previous commit:
- testRowNumberOverEmpty_2shard — ROW_NUMBER OVER (), the helper sequence
column PPL streamstats (with hasGroup=true) emits.
- testSumOverPartitionByOrderBy_2shard — SUM with both partition keys and
order keys. Calcite's RexOver expansion yields different rendered text
(no NULLS LAST suffix on default null ordering); the assertion pins the
actual rendered shape.
- testRowNumberOverPartitionByOrderBy_2shard — combined PARTITION BY +
ORDER BY for ROW_NUMBER, the streamstats … by … helper-sequence shape.
Also extends MockDataFusionBackend.windowCapabilities to declare
ROW_NUMBER, mirroring the production DataFusionAnalyticsBackendPlugin.
Without it the planner rejects ROW_NUMBER on the mock backend with
"No backend supports window functions [ROW_NUMBER]".
All 23 WindowPlanShapeTests + analytics-framework tests pass.
Signed-off-by: Songkan Tang <songkant@amazon.com>
* [analytics-engine] Preserve lifted-window Project layer in fragment rewire
OpenSearchProject.liftNestedRexOver hoists nested RexOver expressions into a
2-layer LogicalProject(LogicalProject(input)) — outer rewrites to RexInputRefs
into the lower's appended window column, lower computes the window. After
convertStandalone the wrapper is Project_outer(Project_lower(input)).
DataFusionFragmentConvertor.replaceInput previously swapped the topmost
Project's direct child wholesale, dropping the lower Project. Subsequent
attachFragmentOnTop calls then crashed substrait deserialisation with
"Field reference offset (N) must be less than number of fields in struct (N)"
because the outer's RexInputRefs pointed past the now-narrower input struct.
Surfaced on multi-shard streamstats … by where SINGLETON-gather injects an
exchange cut that recursively rewires through the lift wrapper. Single-shard
plans avoided it because no rewire happens.
Detect the lift signature (Project whose direct child is a Project containing
a WindowFunctionInvocation) and recurse so newInput replaces the lower
Project's input — both lift layers stay above newInput. Adds a unit-level
regression in DataFusionFragmentConvertorTests and upgrades
StreamstatsCommandIT.testStreamstatsBy_3shard from an error-pinning assertion
to a correctness assertion on the per-partition final running counts.
Signed-off-by: Songkan Tang <songkant@amazon.com>
* [QA] testChainedEventstats_3shard — pin lifted-window rewire across multiple stages
Two chained eventstats on a 3-shard index, each producing its own 2-layer
LogicalProject(LogicalProject(input)) lift bundle that flows through a
separate attachFragmentOnTop call on the multi-shard exchange path. Pins
that the lifted-window detection in DataFusionFragmentConvertor.replaceInput
fires correctly per attach call rather than only on the outermost lift bundle.
Signed-off-by: Songkan Tang <songkant@amazon.com>
* [QA] Drop redundant *WithNull tests; tighten *WithNullBucket on str3
testEventstatsWithNull / testEventstatsByWithNull and the streamstats peers
were exact copies of their *non*-null variants — calcs has no nulls in str0,
so they exercised nothing the base tests didn't. Removed.
testEventstatsByWithNullBucket / testStreamstatsByWithNullBucket now partition
by str3 (7 nulls + 10 'e' rows) instead of str0 (no nulls). The default and
bucket_nullable=false outputs only diverge when a null-key partition exists,
so the prior asserts proved nothing about the bucket_nullable=false path.
With str3 the null partition's row carries NULL aggregates under
bucket_nullable=false vs concrete values under default, which is what these
tests are supposed to demonstrate.
Signed-off-by: Songkan Tang <songkant@amazon.com>
* [QA] Pin span()-in-PARTITION-BY assertions to exact substrait error
The 14 eventstats/streamstats tests that exercise span(...) inside a window
function's PARTITION BY were using assertErrorAny — anything-fails — which
hides regressions. After verifying each query against the cluster, all 14
fail with the same precise error:
java.lang.UnsupportedOperationException: Unable to convert the type NULL
at io.substrait.isthmus.TypeConverter.toSubstrait(TypeConverter.java:192)
... at WindowFunctionConverter.generateBinding (...)
PPL's span(int0, 10) lowers to SPAN($10, 10, NULL:NULL) — the third operand
is a NULL-typed literal. span() works fine in scalar contexts (stats … by
span(...)) because non-window paths don't visit literal operands the same
way, but isthmus's WindowFunctionConverter walks every operand and trips on
the NULL type.
Replaced assertErrorAny with assertErrorContains pinning the substring
"Unable to convert the type NULL" in all 14 cases, plus updated the
class-level docs in both files. When isthmus or the PPL frontend drops
the NULL operand, these tests will fail loudly so a follow-up can upgrade
them to row assertions.
Signed-off-by: Songkan Tang <songkant@amazon.com>
* [analytics-engine] Recurse BackendPlanAdapter into RexOver.window keys
PPL eventstats/streamstats … by span(field, n) lowers to
RexOver(... PARTITION BY SPAN(field, n, NULL:NULL)). Substrait conversion
runs through SpanAdapter, which rewrites SPAN(field, n, NULL) into
FLOOR(field/n)*n so the NULL-typed third operand never reaches isthmus.
But BackendPlanAdapter.adaptRex only walked RexCall.getOperands() — RexOver's
window (partitionKeys / orderKeys) lives on a separate field, so the SPAN
calls inside PARTITION BY were untouched and survived into substrait emission,
where isthmus's WindowFunctionConverter visited every operand including the
NULL literal and threw 'Unable to convert the type NULL'.
Teach adaptRex to recognize RexOver and recurse into its window's
partitionKeys and orderKeys, then rebuild the RexOver via
rexBuilder.makeOver with the adapted window. ORDER BY keys are wrapped in
RexFieldCollation; rebuild the collation only when the inner expression
changed. Same SpanAdapter rewrite path now applies to window-context SPAN
exactly as it does for stats-context SPAN.
Flips all 14 span-in-PARTITION-BY IT cases (7 in EventstatsCommandIT, 7 in
StreamstatsCommandIT) from assertErrorContains('Unable to convert the type
NULL') to assertRowsEqual against the actual per-bucket aggregates.
Signed-off-by: Songkan Tang <songkant@amazon.com>
* [QA] Drop stats-collapse, assert per-row data on slim schema
The eventstats / streamstats IT cases were collapsing 17-row outputs through
| stats max(...) by <col> chains so the assertions stayed compact. With only
17 rows in calcs the collapse hides the broadcast-vs-running shape these
commands are supposed to demonstrate.
Reworked every reachable test (no error / no flaky-by-shard) to:
* drop the | stats collapse,
* select a slim schema via | fields key, <by-col?>, int0, <new eventstats / streamstats cols>,
* sort key for deterministic order,
* assert all 17 (or the post-filter row count) per-row outputs explicitly.
Single-shard streamstats becomes deterministic with `| sort key` so per-row
running aggregates are reproducible. testStreamstatsBy_3shard keeps its
`| stats max(cnt) by str0` collapse — 3-shard streaming order is shard-
dependent and per-row would be flaky.
While in there, two `WithNullBy` companions were partitioning by str0 (no nulls
in calcs) which silently negated the by-key-null coverage they claimed:
* testEventstatsVarianceWithNullBy → switched to `by str3` (7 nulls + 10 'e')
* testStreamstatsVarianceWithNullBy → switched to `by str3`
Added testStreamstatsByWithNull as the default-mode companion to
testStreamstatsByWithNullBucket (eventstats already has the equivalent pair).
Verified locally:
* 29/29 EventstatsCommandIT pass
* 47/47 StreamstatsCommandIT pass
* :sandbox:qa:analytics-engine-rest:precommit green
Signed-off-by: Songkan Tang <songkant@amazon.com>
---------
Signed-off-by: Songkan Tang <songkant@amazon.com>1 parent c2d0a6c commit cea1b65
10 files changed
Lines changed: 2724 additions & 31 deletions
File tree
- sandbox
- libs/analytics-framework/src/main/java/org/opensearch/analytics/spi
- plugins
- analytics-backend-datafusion/src
- main/java/org/opensearch/be/datafusion
- test/java/org/opensearch/be/datafusion
- analytics-engine/src
- main/java/org/opensearch/analytics/planner
- dag
- rules
- test/java/org/opensearch/analytics/planner
- qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa
Lines changed: 8 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
15 | | - | |
16 | | - | |
17 | | - | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
18 | 23 | | |
19 | 24 | | |
20 | 25 | | |
| |||
24 | 29 | | |
25 | 30 | | |
26 | 31 | | |
27 | | - | |
28 | 32 | | |
29 | 33 | | |
30 | 34 | | |
| |||
Lines changed: 7 additions & 5 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
430 | 430 | | |
431 | 431 | | |
432 | 432 | | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
433 | 440 | | |
434 | 441 | | |
435 | 442 | | |
| |||
438 | 445 | | |
439 | 446 | | |
440 | 447 | | |
441 | | - | |
442 | | - | |
443 | | - | |
444 | | - | |
445 | | - | |
446 | 448 | | |
447 | 449 | | |
448 | 450 | | |
| |||
Lines changed: 16 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
548 | 548 | | |
549 | 549 | | |
550 | 550 | | |
| 551 | + | |
| 552 | + | |
| 553 | + | |
| 554 | + | |
| 555 | + | |
| 556 | + | |
| 557 | + | |
551 | 558 | | |
552 | 559 | | |
553 | 560 | | |
| |||
561 | 568 | | |
562 | 569 | | |
563 | 570 | | |
| 571 | + | |
| 572 | + | |
| 573 | + | |
| 574 | + | |
| 575 | + | |
| 576 | + | |
| 577 | + | |
| 578 | + | |
| 579 | + | |
564 | 580 | | |
565 | 581 | | |
566 | 582 | | |
| |||
Lines changed: 57 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
641 | 641 | | |
642 | 642 | | |
643 | 643 | | |
| 644 | + | |
| 645 | + | |
| 646 | + | |
| 647 | + | |
| 648 | + | |
| 649 | + | |
| 650 | + | |
| 651 | + | |
| 652 | + | |
| 653 | + | |
| 654 | + | |
| 655 | + | |
| 656 | + | |
| 657 | + | |
| 658 | + | |
| 659 | + | |
| 660 | + | |
| 661 | + | |
| 662 | + | |
| 663 | + | |
| 664 | + | |
| 665 | + | |
| 666 | + | |
| 667 | + | |
| 668 | + | |
| 669 | + | |
| 670 | + | |
| 671 | + | |
| 672 | + | |
| 673 | + | |
| 674 | + | |
| 675 | + | |
| 676 | + | |
| 677 | + | |
| 678 | + | |
| 679 | + | |
| 680 | + | |
| 681 | + | |
| 682 | + | |
| 683 | + | |
| 684 | + | |
| 685 | + | |
| 686 | + | |
| 687 | + | |
| 688 | + | |
| 689 | + | |
| 690 | + | |
| 691 | + | |
| 692 | + | |
| 693 | + | |
| 694 | + | |
| 695 | + | |
| 696 | + | |
| 697 | + | |
| 698 | + | |
| 699 | + | |
| 700 | + | |
644 | 701 | | |
Lines changed: 49 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
18 | 19 | | |
19 | 20 | | |
| 21 | + | |
20 | 22 | | |
| 23 | + | |
21 | 24 | | |
22 | 25 | | |
23 | 26 | | |
| |||
204 | 207 | | |
205 | 208 | | |
206 | 209 | | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
207 | 256 | | |
208 | 257 | | |
209 | 258 | | |
| |||
Lines changed: 9 additions & 12 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
285 | 285 | | |
286 | 286 | | |
287 | 287 | | |
288 | | - | |
289 | | - | |
290 | | - | |
291 | | - | |
292 | | - | |
293 | | - | |
294 | | - | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
295 | 297 | | |
296 | 298 | | |
297 | 299 | | |
| |||
302 | 304 | | |
303 | 305 | | |
304 | 306 | | |
305 | | - | |
306 | | - | |
307 | | - | |
308 | | - | |
309 | | - | |
310 | 307 | | |
311 | 308 | | |
312 | 309 | | |
| |||
Lines changed: 3 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
138 | 138 | | |
139 | 139 | | |
140 | 140 | | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
141 | 144 | | |
142 | 145 | | |
143 | 146 | | |
| |||
146 | 149 | | |
147 | 150 | | |
148 | 151 | | |
149 | | - | |
150 | | - | |
151 | 152 | | |
152 | 153 | | |
153 | 154 | | |
| |||
0 commit comments