Commit 817f7f3
authored
Close gaps from top/rare analytics-engine wiring (opensearch-project#5433)
* Forward PPL_SYNTAX_LEGACY_PREFERRED through UnifiedQueryContext
`RestUnifiedQueryAction.applyClusterOverrides` previously only forwarded
`PPL_REX_MAX_MATCH_LIMIT` into the per-request `UnifiedQueryContext`. As a
result, cluster-side updates to `plugins.ppl.syntax.legacy.preferred` were
ignored on the analytics-engine route: `PPLQueryParser` -> `AstBuilder` ->
`ArgumentFactory` read the legacy-preferred flag from the unified context's
settings map, which never received the override. This caused queries like
`top age` / `rare state` with `usenull` defaulting off to behave as if
`usenull=true` on the analytics route.
Refactor the override builder into a small helper and forward both
`PPL_REX_MAX_MATCH_LIMIT` and `PPL_SYNTAX_LEGACY_PREFERRED`. Future keys can
be added with a one-liner.
Fixes `CalciteTopCommandIT.testTopCommandLegacyFalse` and
`CalciteRareCommandIT.testRareCommandLegacyFalse` against the analytics
route (`-Dtests.analytics.force_routing=true`).
Signed-off-by: Kai Huang <huangkaics@gmail.com>
* Stabilize rare/top tie-break by appending field columns to ROW_NUMBER order
`CalciteRelNodeVisitor.visitRareTopN` lowers `rare`/`top` to a
`ROW_NUMBER() OVER (PARTITION BY ... ORDER BY count [DESC])` window. With
only the count column in the ORDER BY clause, ties at the same count
resolved via the upstream operator's insertion order, which differed
between backends (in-process Calcite vs. analytics-engine vs. Lucene
pushdown). On the analytics-engine route, `testRareWithGroup` failed
because ROW_NUMBER picked NV at count=8 while the test expected AR.
Append the rare/top field columns as secondary ASC keys so ties resolve
alphabetically and deterministically across backends. This matches the
behavior of the existing OpenSearch terms-aggregation pushdown, which
tie-breaks on `_key:asc`.
Update `RareTopPushdownRule` to accept the new shape: 1 or 2 order keys,
where the (optional) second key must be the rare/top target field in ASC
direction. The pushdown's wire payload is unchanged.
Update the matching unit-test expectations in `CalcitePPLRareTopNTest`
(11 RelNode/result/SparkSQL strings) and 5 explain YAML fixtures.
Fixes `CalciteRareCommandIT.testRareWithGroup` against the analytics
route (and removes the same class of tie-break flakiness across other
rare/top tests).
Signed-off-by: Kai Huang <huangkaics@gmail.com>
* Restrict ROW_NUMBER tie-break to comparable columns + sync no-pushdown fixtures
Three follow-ups to the rare/top tie-break commit, surfaced by CI:
1. **Skip non-comparable tie-break columns.** `top address` on a nested-array
field reached the in-process `EnumerableWindow` with `ORDER BY count DESC,
address` and crashed at runtime — `class java.util.ArrayList cannot be cast
to class java.lang.Comparable`. Filter array / multiset / map / struct (row)
columns out of the tie-break list before appending. If all field columns are
non-comparable, no tie-break is added (falling back to the original
count-only ORDER BY).
2. **Sync the parallel `calcite_no_pushdown/` explain fixtures.**
`PPLIntegTestCase.loadExpectedPlan` switches to
`expectedOutput/calcite_no_pushdown/` when pushdown is disabled, which
`CalciteNoPushdownIT` exercises. The previous commit only updated the
`expectedOutput/calcite/` copies, so `CalciteNoPushdownIT > CalciteExplainIT`
read stale (no-tie-break) YAML. Update the four `explain_{rare,top}_usenull_*`
YAMLs under `calcite_no_pushdown/` to include the `, $N` tie-break key in
both the logical plan and the `EnumerableWindow` collation.
3. **Collapse the pushdown-disabled branch in `RareCommandIT.testRareWithGroup`.**
With the stable tie-break, both pushdown-enabled and pushdown-disabled paths
now resolve the count=8 tie deterministically to `AR` (alphabetical). Drop
the `isPushdownDisabled() ? rows("F", "NV", 8) : rows("F", "AR", 8)`
conditional in favor of the single `AR` expectation.
Signed-off-by: Kai Huang <huangkaics@gmail.com>
---------
Signed-off-by: Kai Huang <huangkaics@gmail.com>1 parent 5851bdb commit 817f7f3
14 files changed
Lines changed: 148 additions & 86 deletions
File tree
- core/src/main/java/org/opensearch/sql/calcite
- integ-test/src/test
- java/org/opensearch/sql/ppl
- resources/expectedOutput
- calcite_no_pushdown
- calcite
- opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules
- plugin/src/main/java/org/opensearch/sql/plugin/rest
- ppl/src/test/java/org/opensearch/sql/ppl/calcite
Lines changed: 27 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3071 | 3071 | | |
3072 | 3072 | | |
3073 | 3073 | | |
| 3074 | + | |
| 3075 | + | |
| 3076 | + | |
| 3077 | + | |
| 3078 | + | |
| 3079 | + | |
| 3080 | + | |
| 3081 | + | |
| 3082 | + | |
| 3083 | + | |
| 3084 | + | |
| 3085 | + | |
| 3086 | + | |
| 3087 | + | |
| 3088 | + | |
| 3089 | + | |
| 3090 | + | |
3074 | 3091 | | |
3075 | 3092 | | |
3076 | 3093 | | |
3077 | 3094 | | |
3078 | 3095 | | |
3079 | 3096 | | |
3080 | 3097 | | |
3081 | | - | |
| 3098 | + | |
3082 | 3099 | | |
3083 | 3100 | | |
3084 | 3101 | | |
| |||
3102 | 3119 | | |
3103 | 3120 | | |
3104 | 3121 | | |
| 3122 | + | |
| 3123 | + | |
| 3124 | + | |
| 3125 | + | |
| 3126 | + | |
| 3127 | + | |
| 3128 | + | |
| 3129 | + | |
| 3130 | + | |
3105 | 3131 | | |
3106 | 3132 | | |
3107 | 3133 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
59 | 59 | | |
60 | 60 | | |
61 | 61 | | |
62 | | - | |
| 62 | + | |
63 | 63 | | |
64 | 64 | | |
65 | 65 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| |||
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
13 | | - | |
| 13 | + | |
14 | 14 | | |
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| |||
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
13 | | - | |
| 13 | + | |
14 | 14 | | |
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | | - | |
| 14 | + | |
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
13 | | - | |
| 13 | + | |
14 | 14 | | |
15 | 15 | | |
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | | - | |
| 14 | + | |
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
0 commit comments