Uh oh!
There was an error while loading. Please reload this page.
SQL/PPL API migration for OpenSearch - #14
Merged
dai-chen merged 8 commits intoApr 22, 2021
Merged
Conversation
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
dai-chen
marked this pull request as ready for review
April 21, 2021 17:32
chloe-zh
approved these changes
Apr 21, 2021
chloe-zh
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thanks for the changes!
6 tasks
Yury-Fridlyand added a commit
that referenced
this pull request
Aug 17, 2023
* Spotless apply on protocol Signed-off-by: Mitchell Gale <Mitchell.Gale@improving.com> * added ignorefailures Signed-off-by: Mitchell Gale <Mitchell.Gale@improving.com> * Update protocol/src/main/java/org/opensearch/sql/protocol/response/format/RawResponseFormatter.java Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com> Signed-off-by: Mitchell Gale <Mitchell.gale@improving.com> * Apply suggestions from code review Co-authored-by: Guian Gumpac <guian.gumpac@improving.com> Signed-off-by: Mitchell Gale <Mitchell.gale@improving.com> --------- Signed-off-by: Mitchell Gale <Mitchell.Gale@improving.com> Signed-off-by: Mitchell Gale <Mitchell.gale@improving.com> Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com> Co-authored-by: Guian Gumpac <guian.gumpac@improving.com>
songkant-aws pushed a commit
to songkant-aws/sql
that referenced
this pull request
Mar 25, 2025
…leanDateTime Add ITs for date time functions like quarter, second, second_of_minute, convert_tz, get_format
expani pushed a commit
to expani/sql
that referenced
this pull request
Nov 24, 2025
…plan update index to multi shard and update ignored set to ignore fetch queries
songkant-aws added a commit
to songkant-aws/sql
that referenced
this pull request
Apr 22, 2026
…shdown Fixes CI failures on PR opensearch-project#5353 by addressing three issues introduced by the sort-preserved-through-dedup work: 1. DedupPushdownRule passed the *reordered* targetChildProject's field names to addDedupSortHintToAggregate, but the collation's field indices are relative to the dedup's input row type (the original project). This caused the wrong field to be pushed into top_hits' inner sort (e.g. sort on `age` instead of `new_gender`). Pass `project.getRowType().getFieldNames()` instead. 2. The pushed-down top_hits sort inherited OpenSearch's default null ordering (ASC -> NULLS LAST), which disagreed with PPL's ASC-NULLS-FIRST default. This made dedup select a different row between pushdown-on and pushdown-off paths. Apply `.missing("_first")` / `"_last"` on the top_hits sort in the LITERAL_AGG (dedup) branch only, so other top_hits callers (FIRST/LAST/MIN/MAX/ARG_*) keep their existing sort semantics. 3. testSortThenDedup's expected rows assumed NULLS LAST (expected `[Z, B]` for name=B) but PPL default is NULLS FIRST, under which B's first occurrence is the null-category row (opensearch-project#14). Correct the expected rows to match Calcite semantics. Regenerate the expected explain YAML/JSON outputs for the changed plans: - EnumerableWindow now carries `order by [1 ASC-nulls-first]` for PR's window-ORDER-BY change. - top_hits inner sort includes the newly pushed `"sort":[{field:{order,missing}}]` clause for dedup-pushdown tests. - Extended-mode Janino comparator code now calls `compareNullsFirst` because the window has an ORDER BY. Signed-off-by: Songkan Tang <songkant@amazon.com>
qianheng-aws pushed a commit
that referenced
this pull request
Apr 24, 2026
…#3922) (#5353) * [BugFix] Fix sort order not preserved through dedup in Calcite engine (#3922) Signed-off-by: Songkan Tang <songkant@amazon.com> * Update integration test expected outputs for sort-then-dedup plan changes Update CalcitePPLDedupIT.testSortThenDedupKeepEmpty expected rows from 7 to 9 to match the corrected dedup behavior. Update all CalciteExplainIT expected YAML/JSON output files to reflect the new logical/physical plan structure where Sort is restored above dedup and ROW_NUMBER includes ORDER BY. Skip testExplain in CalciteNoPushdownIT since pushdown-disabled produces different physical plans. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): propagate sort collation through LogicalDedup and push to top_hits Add inputCollation field to Dedup/LogicalDedup so sort order from upstream Sort nodes is preserved across rule transformations. The collation flows through the full pipeline: - visitDedupe strips the Sort and captures the remapped collation - PPLSimplifyDedupRule extracts ORDER BY from ROW_NUMBER window - PPLDedupConvertRule uses inputCollation for ORDER BY and restore Sort - DedupPushdownRule passes sort info via hint to AggregateAnalyzer - AggregateAnalyzer sets top_hits sort for correct intra-bucket ordering Also fixes an edge case where sort field is projected away before dedup (e.g. sort DEPTNO | fields ENAME, JOB | dedup 1 JOB) by remapping collation field indices through intermediate Projects in stripInputSort. Signed-off-by: Songkan Tang <songkant@amazon.com> * fix(dedup): correct sort field mapping and null ordering for dedup pushdown Fixes CI failures on PR #5353 by addressing three issues introduced by the sort-preserved-through-dedup work: 1. DedupPushdownRule passed the *reordered* targetChildProject's field names to addDedupSortHintToAggregate, but the collation's field indices are relative to the dedup's input row type (the original project). This caused the wrong field to be pushed into top_hits' inner sort (e.g. sort on `age` instead of `new_gender`). Pass `project.getRowType().getFieldNames()` instead. 2. The pushed-down top_hits sort inherited OpenSearch's default null ordering (ASC -> NULLS LAST), which disagreed with PPL's ASC-NULLS-FIRST default. This made dedup select a different row between pushdown-on and pushdown-off paths. Apply `.missing("_first")` / `"_last"` on the top_hits sort in the LITERAL_AGG (dedup) branch only, so other top_hits callers (FIRST/LAST/MIN/MAX/ARG_*) keep their existing sort semantics. 3. testSortThenDedup's expected rows assumed NULLS LAST (expected `[Z, B]` for name=B) but PPL default is NULLS FIRST, under which B's first occurrence is the null-category row (#14). Correct the expected rows to match Calcite semantics. Regenerate the expected explain YAML/JSON outputs for the changed plans: - EnumerableWindow now carries `order by [1 ASC-nulls-first]` for PR's window-ORDER-BY change. - top_hits inner sort includes the newly pushed `"sort":[{field:{order,missing}}]` clause for dedup-pushdown tests. - Extended-mode Janino comparator code now calls `compareNullsFirst` because the window has an ORDER BY. Signed-off-by: Songkan Tang <songkant@amazon.com> * feat(dedup): push all sort fields to top_hits and capture collation field names Extend the dedup sort hint to carry every field collation instead of only the first one, so a multi-field PPL `sort` preserves its full ordering through the pushed-down `top_hits`: - `PPLHintUtils.addDedupSortHintToAggregate` now encodes the full collation as `field:ORDER|field:ORDER|...` in a single hint option; the getter returns a `List<DedupSortKey>`. `AggregateAnalyzer` emits one sort entry per key, preserving ASC/DESC and the Calcite-aligned NULL ordering. - Collation field names are now captured on `LogicalDedup` itself at creation time (against the row type that produced the collation), rather than being resolved later in `DedupPushdownRule` via the dedup's current input row type. This is necessary because planner rules can narrow the dedup's input between `PPLSimplifyDedupRule` and `DedupPushdownRule`, making the index-based `RelCollation` unsafe to resolve against the (then-narrower) input. - New integration test `testMultiColumnSortThenDedup` verifies that `sort state, age, account_number | dedup 1 gender` returns the exact rows that the full three-field ordering dictates — impossible to achieve if only the first sort field were pushed down. - Update the four existing dedup-expr / dedup-with-expr / complex1 expected plans to reflect the second-field sort entry now present in `top_hits`. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): address PR review — simplify collation handling Two PR review comments: 1. core/.../CalciteRelNodeVisitor.java:735 — the private `backtrackForCollation` method was a one-line wrapper around `PlanUtils.findInputCollation`. Remove the wrapper and call `PlanUtils.findInputCollation` directly; make the util public since it is now used from another package. 2. core/.../PlanUtils.java:660 — `remapCollationThroughProjects` hand-rolled a project-to-top index remapping that was fragile (it only understood Logical Projects with simple `RexInputRef`s). Replace it with Calcite's metadata framework: `RelMetadataQuery.collations(input)` returns the subtree's output collation with project remapping handled by `RelMdCollation`, which knows about more operators than our manual walk did. No behavior change expected — every existing CalciteExplainIT / CalcitePPLDedupIT / CalcitePPLSortIT / CalciteSortCommandIT / CalciteReverseCommandIT integration test still passes, as do the core/opensearch/ppl unit tests. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): permute collation to scan schema instead of name workaround Replace the name-based `inputCollationFieldNames` captured on `LogicalDedup` with Calcite's standard index-based collation propagation. The name approach would break under any downstream rename (`RelCollation` itself stores only indices, never names — a rename changes the name but keeps the index). `DedupPushdownRule` now permutes `dedup.inputCollation` into scan-schema indices via `RexInputRef`s of the immediate child `LogicalProject` — mirroring what `Project.getMapping` + `RelCollations.permute` do for Calcite's own trait-propagation paths (cf. `EnumerableMergeJoin.passThroughTraits`). Two cases are handled: - Collation still addresses the child project's output (no rule inserted a narrower project in between): permute each key through the project's projection list. A non-`RexInputRef` projection (computed column, e.g. `lower(gender)`) cannot be expressed as an OS field sort, so the hint is dropped — Calcite's outer sort still restores order. - Collation indices are out of range for the child project's output but within the scan's schema (a narrower project got pushed below dedup after creation): indices are already scan-schema indices; use as-is. Drop `Dedup.inputCollationFieldNames` and the 7-arg `copy` signature — the name capture was a workaround that couldn't survive renames and is no longer needed. Update the two `explain_dedup_expr_complex1*` YAMLs: in that test the sort keys are computed columns (`lower(gender)` / `lower(state)`), which we now correctly refuse to push as `top_hits` sort entries. The outer Calcite sort still runs and the test's final row order is unaffected. Signed-off-by: Songkan Tang <songkant@amazon.com> * fix(dedup): resolve stale inputCollation after planner narrows dedup's input CI revealed that after `PPLSimplifyDedupRule` captures `inputCollation` against the dedup's original input row type, a later Calcite rule (typically scan absorbing a narrowing project) can swap in a different input row type without going through `Dedup.copy()` — so the collation's indices become stale. Symptom: `IndexOutOfBoundsException` in `PPLDedupConvertRule.collationToOrderKeys` (`field ordinal [7] out of range; input fields are: [...]`) in NoPushdown mode, and silently-wrong top_hits sort in pushdown mode. Fix: - Reintroduce `Dedup.inputCollationFieldNames` captured at `LogicalDedup.create` time, strictly as a name-based *fallback anchor* for the "replacement is not a Project" case (scans don't rename, so names are stable there; rename-through- Project scenarios are already handled by Calcite's own index propagation). - `PPLDedupConvertRule.onMatch` now resolves the collation to the current input's schema: if indices are still valid, use as-is; otherwise look each sort key up by original name in the current row type. - `DedupPushdownRule` uses the same two-case resolution (Project permute → name-based fallback) to produce scan-schema indices for the top_hits sort hint, dropping the hint cleanly if any key still can't be resolved. Verified on the integ-test worktree: full `CalciteExplainIT`, `CalcitePPLDedupIT`, and the entire `CalciteNoPushdownIT` suite all pass, as do core/opensearch/ppl unit tests. Signed-off-by: Songkan Tang <songkant@amazon.com> * test(dedup): extend #3922 fixture to actually reproduce the bug The original 5-row fixture happened to pass on pre-fix because the single-partition EnumerableWindow path preserved input order by accident. Adding 5 more rows across a wider category range forces the collation to get shuffled pre-fix (verified locally: output reorders to [X,X,Z,A,B,C,D]), so the expected-datarows match now fails without the fix. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): address PR review comments - PPLDedupConvertRule / DedupPushdownRule: replace fully-qualified org.apache.calcite.rel.* references with imports. - PPLHintUtils: import java.util.Objects; throw IllegalStateException on out-of-range collation index instead of silently skipping — the index is resolved against scan schema upstream, so mismatch is a bug signal. - CalciteExplainIT: drop the testExplain override that forced pushdown-only. Update the corresponding no-pushdown expected plan (explain_output.yaml under calcite_no_pushdown/) to reflect the post-dedup Sort + ROW_NUMBER ORDER BY introduced by this PR. Signed-off-by: Songkan Tang <songkant@amazon.com> --------- Signed-off-by: Songkan Tang <songkant@amazon.com>
jhansimandava27-lang pushed a commit
to jhansimandava27-lang/sql
that referenced
this pull request
Apr 27, 2026
…opensearch-project#3922) (opensearch-project#5353) * [BugFix] Fix sort order not preserved through dedup in Calcite engine (opensearch-project#3922) Signed-off-by: Songkan Tang <songkant@amazon.com> * Update integration test expected outputs for sort-then-dedup plan changes Update CalcitePPLDedupIT.testSortThenDedupKeepEmpty expected rows from 7 to 9 to match the corrected dedup behavior. Update all CalciteExplainIT expected YAML/JSON output files to reflect the new logical/physical plan structure where Sort is restored above dedup and ROW_NUMBER includes ORDER BY. Skip testExplain in CalciteNoPushdownIT since pushdown-disabled produces different physical plans. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): propagate sort collation through LogicalDedup and push to top_hits Add inputCollation field to Dedup/LogicalDedup so sort order from upstream Sort nodes is preserved across rule transformations. The collation flows through the full pipeline: - visitDedupe strips the Sort and captures the remapped collation - PPLSimplifyDedupRule extracts ORDER BY from ROW_NUMBER window - PPLDedupConvertRule uses inputCollation for ORDER BY and restore Sort - DedupPushdownRule passes sort info via hint to AggregateAnalyzer - AggregateAnalyzer sets top_hits sort for correct intra-bucket ordering Also fixes an edge case where sort field is projected away before dedup (e.g. sort DEPTNO | fields ENAME, JOB | dedup 1 JOB) by remapping collation field indices through intermediate Projects in stripInputSort. Signed-off-by: Songkan Tang <songkant@amazon.com> * fix(dedup): correct sort field mapping and null ordering for dedup pushdown Fixes CI failures on PR opensearch-project#5353 by addressing three issues introduced by the sort-preserved-through-dedup work: 1. DedupPushdownRule passed the *reordered* targetChildProject's field names to addDedupSortHintToAggregate, but the collation's field indices are relative to the dedup's input row type (the original project). This caused the wrong field to be pushed into top_hits' inner sort (e.g. sort on `age` instead of `new_gender`). Pass `project.getRowType().getFieldNames()` instead. 2. The pushed-down top_hits sort inherited OpenSearch's default null ordering (ASC -> NULLS LAST), which disagreed with PPL's ASC-NULLS-FIRST default. This made dedup select a different row between pushdown-on and pushdown-off paths. Apply `.missing("_first")` / `"_last"` on the top_hits sort in the LITERAL_AGG (dedup) branch only, so other top_hits callers (FIRST/LAST/MIN/MAX/ARG_*) keep their existing sort semantics. 3. testSortThenDedup's expected rows assumed NULLS LAST (expected `[Z, B]` for name=B) but PPL default is NULLS FIRST, under which B's first occurrence is the null-category row (opensearch-project#14). Correct the expected rows to match Calcite semantics. Regenerate the expected explain YAML/JSON outputs for the changed plans: - EnumerableWindow now carries `order by [1 ASC-nulls-first]` for PR's window-ORDER-BY change. - top_hits inner sort includes the newly pushed `"sort":[{field:{order,missing}}]` clause for dedup-pushdown tests. - Extended-mode Janino comparator code now calls `compareNullsFirst` because the window has an ORDER BY. Signed-off-by: Songkan Tang <songkant@amazon.com> * feat(dedup): push all sort fields to top_hits and capture collation field names Extend the dedup sort hint to carry every field collation instead of only the first one, so a multi-field PPL `sort` preserves its full ordering through the pushed-down `top_hits`: - `PPLHintUtils.addDedupSortHintToAggregate` now encodes the full collation as `field:ORDER|field:ORDER|...` in a single hint option; the getter returns a `List<DedupSortKey>`. `AggregateAnalyzer` emits one sort entry per key, preserving ASC/DESC and the Calcite-aligned NULL ordering. - Collation field names are now captured on `LogicalDedup` itself at creation time (against the row type that produced the collation), rather than being resolved later in `DedupPushdownRule` via the dedup's current input row type. This is necessary because planner rules can narrow the dedup's input between `PPLSimplifyDedupRule` and `DedupPushdownRule`, making the index-based `RelCollation` unsafe to resolve against the (then-narrower) input. - New integration test `testMultiColumnSortThenDedup` verifies that `sort state, age, account_number | dedup 1 gender` returns the exact rows that the full three-field ordering dictates — impossible to achieve if only the first sort field were pushed down. - Update the four existing dedup-expr / dedup-with-expr / complex1 expected plans to reflect the second-field sort entry now present in `top_hits`. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): address PR review — simplify collation handling Two PR review comments: 1. core/.../CalciteRelNodeVisitor.java:735 — the private `backtrackForCollation` method was a one-line wrapper around `PlanUtils.findInputCollation`. Remove the wrapper and call `PlanUtils.findInputCollation` directly; make the util public since it is now used from another package. 2. core/.../PlanUtils.java:660 — `remapCollationThroughProjects` hand-rolled a project-to-top index remapping that was fragile (it only understood Logical Projects with simple `RexInputRef`s). Replace it with Calcite's metadata framework: `RelMetadataQuery.collations(input)` returns the subtree's output collation with project remapping handled by `RelMdCollation`, which knows about more operators than our manual walk did. No behavior change expected — every existing CalciteExplainIT / CalcitePPLDedupIT / CalcitePPLSortIT / CalciteSortCommandIT / CalciteReverseCommandIT integration test still passes, as do the core/opensearch/ppl unit tests. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): permute collation to scan schema instead of name workaround Replace the name-based `inputCollationFieldNames` captured on `LogicalDedup` with Calcite's standard index-based collation propagation. The name approach would break under any downstream rename (`RelCollation` itself stores only indices, never names — a rename changes the name but keeps the index). `DedupPushdownRule` now permutes `dedup.inputCollation` into scan-schema indices via `RexInputRef`s of the immediate child `LogicalProject` — mirroring what `Project.getMapping` + `RelCollations.permute` do for Calcite's own trait-propagation paths (cf. `EnumerableMergeJoin.passThroughTraits`). Two cases are handled: - Collation still addresses the child project's output (no rule inserted a narrower project in between): permute each key through the project's projection list. A non-`RexInputRef` projection (computed column, e.g. `lower(gender)`) cannot be expressed as an OS field sort, so the hint is dropped — Calcite's outer sort still restores order. - Collation indices are out of range for the child project's output but within the scan's schema (a narrower project got pushed below dedup after creation): indices are already scan-schema indices; use as-is. Drop `Dedup.inputCollationFieldNames` and the 7-arg `copy` signature — the name capture was a workaround that couldn't survive renames and is no longer needed. Update the two `explain_dedup_expr_complex1*` YAMLs: in that test the sort keys are computed columns (`lower(gender)` / `lower(state)`), which we now correctly refuse to push as `top_hits` sort entries. The outer Calcite sort still runs and the test's final row order is unaffected. Signed-off-by: Songkan Tang <songkant@amazon.com> * fix(dedup): resolve stale inputCollation after planner narrows dedup's input CI revealed that after `PPLSimplifyDedupRule` captures `inputCollation` against the dedup's original input row type, a later Calcite rule (typically scan absorbing a narrowing project) can swap in a different input row type without going through `Dedup.copy()` — so the collation's indices become stale. Symptom: `IndexOutOfBoundsException` in `PPLDedupConvertRule.collationToOrderKeys` (`field ordinal [7] out of range; input fields are: [...]`) in NoPushdown mode, and silently-wrong top_hits sort in pushdown mode. Fix: - Reintroduce `Dedup.inputCollationFieldNames` captured at `LogicalDedup.create` time, strictly as a name-based *fallback anchor* for the "replacement is not a Project" case (scans don't rename, so names are stable there; rename-through- Project scenarios are already handled by Calcite's own index propagation). - `PPLDedupConvertRule.onMatch` now resolves the collation to the current input's schema: if indices are still valid, use as-is; otherwise look each sort key up by original name in the current row type. - `DedupPushdownRule` uses the same two-case resolution (Project permute → name-based fallback) to produce scan-schema indices for the top_hits sort hint, dropping the hint cleanly if any key still can't be resolved. Verified on the integ-test worktree: full `CalciteExplainIT`, `CalcitePPLDedupIT`, and the entire `CalciteNoPushdownIT` suite all pass, as do core/opensearch/ppl unit tests. Signed-off-by: Songkan Tang <songkant@amazon.com> * test(dedup): extend opensearch-project#3922 fixture to actually reproduce the bug The original 5-row fixture happened to pass on pre-fix because the single-partition EnumerableWindow path preserved input order by accident. Adding 5 more rows across a wider category range forces the collation to get shuffled pre-fix (verified locally: output reorders to [X,X,Z,A,B,C,D]), so the expected-datarows match now fails without the fix. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): address PR review comments - PPLDedupConvertRule / DedupPushdownRule: replace fully-qualified org.apache.calcite.rel.* references with imports. - PPLHintUtils: import java.util.Objects; throw IllegalStateException on out-of-range collation index instead of silently skipping — the index is resolved against scan schema upstream, so mismatch is a bug signal. - CalciteExplainIT: drop the testExplain override that forced pushdown-only. Update the corresponding no-pushdown expected plan (explain_output.yaml under calcite_no_pushdown/) to reflect the post-dedup Sort + ROW_NUMBER ORDER BY introduced by this PR. Signed-off-by: Songkan Tang <songkant@amazon.com> --------- Signed-off-by: Songkan Tang <songkant@amazon.com>
asifabashar pushed a commit
to asifabashar/sql
that referenced
this pull request
Jul 21, 2026
…opensearch-project#3922) (opensearch-project#5353) * [BugFix] Fix sort order not preserved through dedup in Calcite engine (opensearch-project#3922) Signed-off-by: Songkan Tang <songkant@amazon.com> * Update integration test expected outputs for sort-then-dedup plan changes Update CalcitePPLDedupIT.testSortThenDedupKeepEmpty expected rows from 7 to 9 to match the corrected dedup behavior. Update all CalciteExplainIT expected YAML/JSON output files to reflect the new logical/physical plan structure where Sort is restored above dedup and ROW_NUMBER includes ORDER BY. Skip testExplain in CalciteNoPushdownIT since pushdown-disabled produces different physical plans. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): propagate sort collation through LogicalDedup and push to top_hits Add inputCollation field to Dedup/LogicalDedup so sort order from upstream Sort nodes is preserved across rule transformations. The collation flows through the full pipeline: - visitDedupe strips the Sort and captures the remapped collation - PPLSimplifyDedupRule extracts ORDER BY from ROW_NUMBER window - PPLDedupConvertRule uses inputCollation for ORDER BY and restore Sort - DedupPushdownRule passes sort info via hint to AggregateAnalyzer - AggregateAnalyzer sets top_hits sort for correct intra-bucket ordering Also fixes an edge case where sort field is projected away before dedup (e.g. sort DEPTNO | fields ENAME, JOB | dedup 1 JOB) by remapping collation field indices through intermediate Projects in stripInputSort. Signed-off-by: Songkan Tang <songkant@amazon.com> * fix(dedup): correct sort field mapping and null ordering for dedup pushdown Fixes CI failures on PR opensearch-project#5353 by addressing three issues introduced by the sort-preserved-through-dedup work: 1. DedupPushdownRule passed the *reordered* targetChildProject's field names to addDedupSortHintToAggregate, but the collation's field indices are relative to the dedup's input row type (the original project). This caused the wrong field to be pushed into top_hits' inner sort (e.g. sort on `age` instead of `new_gender`). Pass `project.getRowType().getFieldNames()` instead. 2. The pushed-down top_hits sort inherited OpenSearch's default null ordering (ASC -> NULLS LAST), which disagreed with PPL's ASC-NULLS-FIRST default. This made dedup select a different row between pushdown-on and pushdown-off paths. Apply `.missing("_first")` / `"_last"` on the top_hits sort in the LITERAL_AGG (dedup) branch only, so other top_hits callers (FIRST/LAST/MIN/MAX/ARG_*) keep their existing sort semantics. 3. testSortThenDedup's expected rows assumed NULLS LAST (expected `[Z, B]` for name=B) but PPL default is NULLS FIRST, under which B's first occurrence is the null-category row (opensearch-project#14). Correct the expected rows to match Calcite semantics. Regenerate the expected explain YAML/JSON outputs for the changed plans: - EnumerableWindow now carries `order by [1 ASC-nulls-first]` for PR's window-ORDER-BY change. - top_hits inner sort includes the newly pushed `"sort":[{field:{order,missing}}]` clause for dedup-pushdown tests. - Extended-mode Janino comparator code now calls `compareNullsFirst` because the window has an ORDER BY. Signed-off-by: Songkan Tang <songkant@amazon.com> * feat(dedup): push all sort fields to top_hits and capture collation field names Extend the dedup sort hint to carry every field collation instead of only the first one, so a multi-field PPL `sort` preserves its full ordering through the pushed-down `top_hits`: - `PPLHintUtils.addDedupSortHintToAggregate` now encodes the full collation as `field:ORDER|field:ORDER|...` in a single hint option; the getter returns a `List<DedupSortKey>`. `AggregateAnalyzer` emits one sort entry per key, preserving ASC/DESC and the Calcite-aligned NULL ordering. - Collation field names are now captured on `LogicalDedup` itself at creation time (against the row type that produced the collation), rather than being resolved later in `DedupPushdownRule` via the dedup's current input row type. This is necessary because planner rules can narrow the dedup's input between `PPLSimplifyDedupRule` and `DedupPushdownRule`, making the index-based `RelCollation` unsafe to resolve against the (then-narrower) input. - New integration test `testMultiColumnSortThenDedup` verifies that `sort state, age, account_number | dedup 1 gender` returns the exact rows that the full three-field ordering dictates — impossible to achieve if only the first sort field were pushed down. - Update the four existing dedup-expr / dedup-with-expr / complex1 expected plans to reflect the second-field sort entry now present in `top_hits`. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): address PR review — simplify collation handling Two PR review comments: 1. core/.../CalciteRelNodeVisitor.java:735 — the private `backtrackForCollation` method was a one-line wrapper around `PlanUtils.findInputCollation`. Remove the wrapper and call `PlanUtils.findInputCollation` directly; make the util public since it is now used from another package. 2. core/.../PlanUtils.java:660 — `remapCollationThroughProjects` hand-rolled a project-to-top index remapping that was fragile (it only understood Logical Projects with simple `RexInputRef`s). Replace it with Calcite's metadata framework: `RelMetadataQuery.collations(input)` returns the subtree's output collation with project remapping handled by `RelMdCollation`, which knows about more operators than our manual walk did. No behavior change expected — every existing CalciteExplainIT / CalcitePPLDedupIT / CalcitePPLSortIT / CalciteSortCommandIT / CalciteReverseCommandIT integration test still passes, as do the core/opensearch/ppl unit tests. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): permute collation to scan schema instead of name workaround Replace the name-based `inputCollationFieldNames` captured on `LogicalDedup` with Calcite's standard index-based collation propagation. The name approach would break under any downstream rename (`RelCollation` itself stores only indices, never names — a rename changes the name but keeps the index). `DedupPushdownRule` now permutes `dedup.inputCollation` into scan-schema indices via `RexInputRef`s of the immediate child `LogicalProject` — mirroring what `Project.getMapping` + `RelCollations.permute` do for Calcite's own trait-propagation paths (cf. `EnumerableMergeJoin.passThroughTraits`). Two cases are handled: - Collation still addresses the child project's output (no rule inserted a narrower project in between): permute each key through the project's projection list. A non-`RexInputRef` projection (computed column, e.g. `lower(gender)`) cannot be expressed as an OS field sort, so the hint is dropped — Calcite's outer sort still restores order. - Collation indices are out of range for the child project's output but within the scan's schema (a narrower project got pushed below dedup after creation): indices are already scan-schema indices; use as-is. Drop `Dedup.inputCollationFieldNames` and the 7-arg `copy` signature — the name capture was a workaround that couldn't survive renames and is no longer needed. Update the two `explain_dedup_expr_complex1*` YAMLs: in that test the sort keys are computed columns (`lower(gender)` / `lower(state)`), which we now correctly refuse to push as `top_hits` sort entries. The outer Calcite sort still runs and the test's final row order is unaffected. Signed-off-by: Songkan Tang <songkant@amazon.com> * fix(dedup): resolve stale inputCollation after planner narrows dedup's input CI revealed that after `PPLSimplifyDedupRule` captures `inputCollation` against the dedup's original input row type, a later Calcite rule (typically scan absorbing a narrowing project) can swap in a different input row type without going through `Dedup.copy()` — so the collation's indices become stale. Symptom: `IndexOutOfBoundsException` in `PPLDedupConvertRule.collationToOrderKeys` (`field ordinal [7] out of range; input fields are: [...]`) in NoPushdown mode, and silently-wrong top_hits sort in pushdown mode. Fix: - Reintroduce `Dedup.inputCollationFieldNames` captured at `LogicalDedup.create` time, strictly as a name-based *fallback anchor* for the "replacement is not a Project" case (scans don't rename, so names are stable there; rename-through- Project scenarios are already handled by Calcite's own index propagation). - `PPLDedupConvertRule.onMatch` now resolves the collation to the current input's schema: if indices are still valid, use as-is; otherwise look each sort key up by original name in the current row type. - `DedupPushdownRule` uses the same two-case resolution (Project permute → name-based fallback) to produce scan-schema indices for the top_hits sort hint, dropping the hint cleanly if any key still can't be resolved. Verified on the integ-test worktree: full `CalciteExplainIT`, `CalcitePPLDedupIT`, and the entire `CalciteNoPushdownIT` suite all pass, as do core/opensearch/ppl unit tests. Signed-off-by: Songkan Tang <songkant@amazon.com> * test(dedup): extend opensearch-project#3922 fixture to actually reproduce the bug The original 5-row fixture happened to pass on pre-fix because the single-partition EnumerableWindow path preserved input order by accident. Adding 5 more rows across a wider category range forces the collation to get shuffled pre-fix (verified locally: output reorders to [X,X,Z,A,B,C,D]), so the expected-datarows match now fails without the fix. Signed-off-by: Songkan Tang <songkant@amazon.com> * refactor(dedup): address PR review comments - PPLDedupConvertRule / DedupPushdownRule: replace fully-qualified org.apache.calcite.rel.* references with imports. - PPLHintUtils: import java.util.Objects; throw IllegalStateException on out-of-range collation index instead of silently skipping — the index is resolved against scan schema upstream, so mismatch is a bug signal. - CalciteExplainIT: drop the testExplain override that forced pushdown-only. Update the corresponding no-pushdown expected plan (explain_output.yaml under calcite_no_pushdown/) to reflect the post-dedup Sort + ROW_NUMBER ORDER BY introduced by this PR. Signed-off-by: Songkan Tang <songkant@amazon.com> --------- Signed-off-by: Songkan Tang <songkant@amazon.com>
TomZhang2 added a commit
to TomZhang2/sql
that referenced
this pull request
Jul 21, 2026
…ories Merge overlapping items: - opensearch-project#1+opensearch-project#10 (sample size/p99/p999) → 统计方法 - opensearch-project#2+opensearch-project#3+opensearch-project#8+opensearch-project#16 (network/heap/JDK/H1 pressure) → 环境与硬件 - opensearch-project#4+opensearch-project#5+opensearch-project#6+opensearch-project#12+opensearch-project#15 (concurrency/cache/forcemerge 1M+10M) → 测试设计 - opensearch-project#7+opensearch-project#13+opensearch-project#14 (G0 cross-engine/C1-H→G3/E-group) → 测量方法, cross-ref §5.6 - opensearch-project#9 (data scale) → 数据覆盖 - opensearch-project#11 (H1 timeout) → 生产风险, cross-ref §5.6.4 Items already detailed in new §5.6 subsections are replaced with brief summaries + cross-references, eliminating ~15 lines of repetition. Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Signed-off-by: taozhang314 <42579706+TomZhang2@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
_opensearch/_sqland_opensearch/_pplAPI endpoint._opendistroendpoint is still accessible for backward compatibility.Issues Resolved
N/A
Check List
_opensearchendpoint andLegacyAPICompatibilityITadded for SQL and PPLBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.