Pushdown sort by complex expressions to scan #4750

songkant-aws · 2025-11-06T06:26:14Z

Description

Add sort performance enhancement of pushing down sort by complex expressions to scan.

Related Issues

Resolves #3912

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
API changes companion pull request created.
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created.

By 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.

Signed-off-by: Songkan Tang <[email protected]>

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java

Signed-off-by: Songkan Tang <[email protected]>

core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java

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

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

LantaoJin · 2025-11-10T08:44:09Z

.../main/java/org/opensearch/sql/opensearch/planner/rules/ExpandCollationOnProjectExprRule.java

-        project.getInput().getTraitSet().getTrait(RelCollationTraitDef.INSTANCE);
+
+    // Branch 1: Check if complex expressions are already sorted by scan
+    if (handleComplexExpressionsSortedByScan(call, project, toTraits, toCollation)) {


can you add some IT and ExplainIT for this case?

All of sort expression pushdown ITs are for this case.

nit: which one for example?
and can this predicate move to definition of ExpandCollationOnProjectExprRule.Config (can reduce node copy)

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

...rch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/SortExpressionInfo.java

opensearch/src/main/java/org/opensearch/sql/opensearch/util/OpenSearchRelOptUtil.java

Signed-off-by: Songkan Tang <[email protected]>

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java

qianheng-aws · 2025-11-13T08:16:41Z

opensearch/src/main/java/org/opensearch/sql/opensearch/util/OpenSearchRelOptUtil.java

+
+    // Check if the scan has sort expressions pushed down
+    if (scan.getPushDownContext().stream()
+        .noneMatch(operation -> operation.type() == PushDownType.SORT_EXPR)) {


Is it possible that the pushed SORT(i.e. SORT_EXPR with all digests are simple expression) can satisfy sort collation here?

It's possible. The cost computation has coarse results by multiplying field count with 1.1. So VolcanoPlanner will prefer regular field sort pushdown.

Another option is to calculate the accurate complex expression count and multiply this count with 1.1. Put a check in the rule to not pushdown if all of expressions are simple expressions

I mean, in this query ... | sort a | eval b = a + 1 | sort b, when pushing the sort b, this method will always return false. But I expect it return true.

Yes, it returns false in this case but doesn't matter. Now, if all of expressions are simple, it will go through regular pushDownSort. Field sort exists in regular traitset and is propagated well. It will be handled by handleSimpleExpressionFieldSorts method in ExpandCollationOnProjectExprRule.

qianheng-aws · 2025-11-13T08:18:58Z

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java

+      Integer offsetValue = LimitIndexScanRule.extractOffsetValue(sort.offset);
+      newScan = (CalciteLogicalIndexScan) scan.pushDownLimit(sort, limitValue, offsetValue);
+    } else {
+      // Attempt to push down sort expressions


We still push down sort although scanProvidesRequiredCollation is true? I think we could just remove the sort directly.

I was thinking if first sort by [a, b] expressions is already in scan. The second sort by [a] expression could narrow down the sort effort. We could still allow the pushdown.

For topK pushdown, it could be also applied. If sort by [a, b], limit 10 is pushed, say the second sort by [a], limit 2 comes in, we should allow it pushdown as well. This may need replace existing SORT_EXPR and LIMIT pushdown context.

But right now, it has the problem that multiple different sort complex expressions doesn't work well yet.

The second sort by [a] expression could narrow down the sort effort. We could still allow the pushdown.

Shall we do the same thing for the above branch then? It only push down limit when scanProvidesRequiredCollation is true.

Revisit the logic, for already pushed down topK and if the scan collation satisfies the output collation, we can still leave the old sort in the scan(remove the new sort in the meantime). It's kind of keeping semantics. For regular sort pushdown, we can override by the new sort.

opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java

qianheng-aws · 2025-11-13T08:25:55Z

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

+      OSRequestBuilderAction action =
+          requestBuilder -> {
+            for (SortExprDigest info : sortExprDigests) {
+              requestBuilder.pushDownSortExpression(


[question] Will this method throw exception by any chance? We plan to make the request builder transformation process lazy. We should put any logic which may throw exception outside of the lambda function, so it can avoid failure on the final plan.

As discussed offline, move most of logic that could throw exception outside. And use a lazy pushdownSortSuppliers method instead.

qianheng-aws · 2025-11-13T08:29:43Z

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java

+                sortExprDigest.getDirection().name()));
+
+    Script script = scriptExpr.getScript();
+    if (script != null) {


What if script is null? Seems nothing happens. We better add assertion or throw exception if that's never possible.

It should never happen. I neglect this check.

qianheng-aws · 2025-11-13T08:48:43Z

...earch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java

+        case SORT_EXPR -> {
+          @SuppressWarnings("unchecked")
+          List<SortExprDigest> sortKeys = (List<SortExprDigest>) operation.digest();
+          dCpu += NumberUtil.multiply(dRows, 1.1 * sortKeys.size());


I think we should filter the simple expr.

Now, it will only count complex expr. The calculation is fine grained.

Signed-off-by: Songkan Tang <[email protected]>

qianheng-aws · 2025-11-17T07:28:12Z

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

+        RexNode sortExpr = digest.getExpression();
+        assert sortExpr instanceof RexCall : "sort expression should be RexCall";
+        Map<String, Object> directionParams = new LinkedHashMap<>();
+        directionParams.put(PlanUtils.NULL_DIRECTION, digest.getNullDirection().name());


Could these 2 params be combined into 1 like MISSING_MIN or MISSING_MAX?

This is not a blocking comment. We have chance to do this when resolving conflict with #4795. These 2 PR both pass parameter and should have conflict. Let's see which will be merged first.

qianheng-aws · 2025-11-17T07:29:48Z

...src/main/java/org/opensearch/sql/opensearch/storage/script/sort/CalciteNumberSortScript.java

+    if (value == null) {
+      boolean isAscending = direction == Direction.ASCENDING;
+      boolean isNullFirst = nullDirection == NullDirection.FIRST;
+      return isAscending == isNullFirst ? Double.NEGATIVE_INFINITY : Double.NaN;


Shouldn't be Double.POSITIVE_INFINITY for Double.NaN?

According to Double's constant definition,
Double.POSITIVE_INFINITY is equal to the value of Double.longBitsToDouble(0x7ff0000000000000L).
Double.NaN is equal to the value of Double.longBitsToDouble(0x7ff8000000000000L).

compareTo() treats NaN is larger. However operator of comparisons like '>', '=', '<' always return false if comparing with NaN because NaN is not a number.

I searched the sorting order online. It indicates most of SQL engine like PostgreSQL will sort NaN as the largest value for Double in ascending order.

LantaoJin

non-blocking for this PR:
can you test the sort pushdown on join condition such on a.id + 1 = b.id +2?

opensearch-trigger-bot · 2025-11-18T06:04:49Z

The backport to 2.19-dev failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-4750-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e83f7413f5924dc5b9f91ea79bc72aed337a3a33
# Push it to GitHub
git push --set-upstream origin backport/backport-4750-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-dev

Then, create a pull request where the base branch is 2.19-dev and the compare/head branch is backport/backport-4750-to-2.19-dev.

songkant-aws · 2025-11-18T08:23:55Z

@LantaoJin It doesn't support sort expr pushdown for SortMergeJoin yet.

SortMergeJoin is optimized by EnumerableMergeJoinRule, which is a physical plan level rule. It will inserts collation to left and right children of join and then sort is transformed to EnumerableSort.

Our rules mostly optimize logical plans. Considering it's a valid case, I think one more enhancement is to add physical plan optimization support(Enumerable convention optimization) as well. I will create an issue to track this enhancement.

* Pushdown sort expression to scan Signed-off-by: Songkan Tang <[email protected]> * Refactor a bit of SortExprIndexScanRule Signed-off-by: Songkan Tang <[email protected]> * Simplify some code and add more explain tests Signed-off-by: Songkan Tang <[email protected]> * Support null ordering for sort expression pushdown Signed-off-by: Songkan Tang <[email protected]> * Attempt to fix security tests and add more test cases Signed-off-by: Songkan Tang <[email protected]> * Address comments Signed-off-by: Songkan Tang <[email protected]> * Address more comments Signed-off-by: Songkan Tang <[email protected]> --------- Signed-off-by: Songkan Tang <[email protected]>

…) (#4818) * Pushdown sort by complex expressions to scan (#4750) * Pushdown sort expression to scan Signed-off-by: Songkan Tang <[email protected]> * Refactor a bit of SortExprIndexScanRule Signed-off-by: Songkan Tang <[email protected]> * Simplify some code and add more explain tests Signed-off-by: Songkan Tang <[email protected]> * Support null ordering for sort expression pushdown Signed-off-by: Songkan Tang <[email protected]> * Attempt to fix security tests and add more test cases Signed-off-by: Songkan Tang <[email protected]> * Address comments Signed-off-by: Songkan Tang <[email protected]> * Address more comments Signed-off-by: Songkan Tang <[email protected]> --------- Signed-off-by: Songkan Tang <[email protected]> * Fix compilation after merge conflicts Signed-off-by: Songkan Tang <[email protected]> * Fix tests Signed-off-by: Songkan Tang <[email protected]> --------- Signed-off-by: Songkan Tang <[email protected]>

* Pushdown sort expression to scan Signed-off-by: Songkan Tang <[email protected]> * Refactor a bit of SortExprIndexScanRule Signed-off-by: Songkan Tang <[email protected]> * Simplify some code and add more explain tests Signed-off-by: Songkan Tang <[email protected]> * Support null ordering for sort expression pushdown Signed-off-by: Songkan Tang <[email protected]> * Attempt to fix security tests and add more test cases Signed-off-by: Songkan Tang <[email protected]> * Address comments Signed-off-by: Songkan Tang <[email protected]> * Address more comments Signed-off-by: Songkan Tang <[email protected]> --------- Signed-off-by: Songkan Tang <[email protected]>

songkant-aws added 4 commits November 5, 2025 17:49

Pushdown sort expression to scan

c85bb1a

Signed-off-by: Songkan Tang <[email protected]>

Merge branch 'main' into sort-expression-pushdown

cadac00

Refactor a bit of SortExprIndexScanRule

cc61524

Signed-off-by: Songkan Tang <[email protected]>

Simplify some code and add more explain tests

3bc9d62

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws marked this pull request as ready for review November 6, 2025 09:38

songkant-aws commented Nov 6, 2025

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java Outdated Show resolved Hide resolved

songkant-aws commented Nov 6, 2025

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java Outdated Show resolved Hide resolved

yuancu added enhancement New feature or request backport 2.19-dev labels Nov 7, 2025

songkant-aws added 3 commits November 7, 2025 17:46

Support null ordering for sort expression pushdown

d1bd892

Signed-off-by: Songkan Tang <[email protected]>

Attempt to fix security tests and add more test cases

d21ecef

Signed-off-by: Songkan Tang <[email protected]>

Merge branch 'main' into sort-expression-pushdown

0492cdf

LantaoJin reviewed Nov 10, 2025

View reviewed changes

Address comments

99580ff

Signed-off-by: Songkan Tang <[email protected]>

LantaoJin previously approved these changes Nov 12, 2025

View reviewed changes

qianheng-aws reviewed Nov 13, 2025

View reviewed changes

Address more comments

9714ed8

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws dismissed LantaoJin’s stale review via 9714ed8 November 14, 2025 09:40

qianheng-aws reviewed Nov 17, 2025

View reviewed changes

qianheng-aws approved these changes Nov 18, 2025

View reviewed changes

LantaoJin approved these changes Nov 18, 2025

View reviewed changes

LantaoJin merged commit e83f741 into opensearch-project:main Nov 18, 2025
66 of 67 checks passed

opensearch-trigger-bot bot added the backport-failed label Nov 18, 2025

songkant-aws mentioned this pull request Nov 18, 2025

[Backport 2.19-dev] Pushdown sort by complex expressions to scan (#4750) #4818

Merged

8 tasks

qianheng-aws mentioned this pull request Nov 18, 2025

Perform RexNode expression standardization for script push down. #4795

Merged

8 tasks

songkant-aws mentioned this pull request Nov 19, 2025

[FEATURE] Support sort expressions pushdown for SortMergeJoin #4823

Closed

LantaoJin added the backport-manually Filed a PR to backport manually. label Nov 19, 2025

songkant-aws mentioned this pull request Nov 20, 2025

Support sort expression pushdown for SortMergeJoin #4830

Merged

8 tasks

songkant-aws deleted the sort-expression-pushdown branch December 11, 2025 07:57

Pushdown sort by complex expressions to scan #4750

Pushdown sort by complex expressions to scan #4750

Uh oh!

Conversation

songkant-aws commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songkant-aws Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songkant-aws Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qianheng-aws Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songkant-aws Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

opensearch-trigger-bot bot commented Nov 18, 2025

Uh oh!

songkant-aws commented Nov 18, 2025

songkant-aws commented Nov 6, 2025 •

edited

Loading

LantaoJin Nov 12, 2025 •

edited

Loading

songkant-aws Nov 14, 2025 •

edited

Loading

songkant-aws Nov 14, 2025 •

edited

Loading

qianheng-aws Nov 17, 2025 •

edited

Loading

songkant-aws Nov 17, 2025 •

edited

Loading