Implement reverse performance optimization

[selsong]

Description

This PR updates the Calcite planner to handle reverse correctly by flipping sort directions in pushdown.
It also documents and tests edge cases such as double or consecutive reverse usage, even though these are not expected in normal user queries.

1. Flip sort direction

Query:

source = accounts | sort + balance, - firstname | reverse

Logical Plan:

LogicalSystemLimit(sort0=[$3], sort1=[$1], dir0=[DESC-nulls-last], dir1=[ASC-nulls-first], fetch=[10000], type=[QUERY_SIZE_LIMIT])
  LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])
  LogicalSort(sort0=[$3], sort1=[$1], dir0=[DESC-nulls-last], dir1=[ASC-nulls-first])
    LogicalSort(sort0=[$3], sort1=[$1], dir0=[ASC-nulls-first], dir1=[DESC-nulls-last])
      CalciteLogicalIndexScan(table=[[OpenSearch, accounts]])

Physical Plan:

CalciteEnumerableIndexScan(table=[[OpenSearch, accounts]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname], SORT->[
  {
    \"balance\" : {
      \"order\" : \"desc\",
      \"missing\" : \"_last\"
    }
  },
  {
    \"firstname.keyword\" : {
      \"order\" : \"asc\",
      \"missing\" : \"_first\"
    }
  }
], LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={
  \"from\":0,
  \"size\":10000,
  \"timeout\":\"1m\",
  \"_source\":{
    \"includes\":[\"account_number\",\"firstname\",\"address\",\"balance\",\"gender\",\"city\",\"employer\",\"state\",\"age\",\"email\",\"lastname\"],
    \"excludes\":[]
  },
  \"sort\":[
    {\"balance\":{\"order\":\"desc\",\"missing\":\"_last\"}},
    {\"firstname.keyword\":{\"order\":\"asc\",\"missing\":\"_first\"}}
  ]
}, requestedTotalSize=10000, pageSize=null, startFrom=0)])

Explanation:
The single reverse flips the original sort order (balance ASC, firstname DESC) to the reversed order (balance DESC, firstname ASC).

2. Double reverse

Query:

source = accounts | sort + balance, - firstname | reverse | reverse

Logical Plan:

LogicalSystemLimit(sort0=[$3], sort1=[$1], dir0=[ASC-nulls-first], dir1=[DESC-nulls-last], fetch=[10000], type=[QUERY_SIZE_LIMIT])
  LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])
  LogicalSort(sort0=[$3], sort1=[$1], dir0=[ASC-nulls-first], dir1=[DESC-nulls-last])   <-- final sort
    LogicalSort(sort0=[$3], sort1=[$1], dir0=[DESC-nulls-last], dir1=[ASC-nulls-first]) <-- after 1st reverse
      LogicalSort(sort0=[$3], sort1=[$1], dir0=[ASC-nulls-first], dir1=[DESC-nulls-last]) <-- original sort
        CalciteLogicalIndexScan(table=[[OpenSearch, accounts]])

Physical Plan:

CalciteEnumerableIndexScan(table=[[OpenSearch, accounts]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname], SORT->[
  {
    \"balance\" : {
      \"order\" : \"asc\",
      \"missing\" : \"_first\"
    }
  },
  {
    \"firstname.keyword\" : {
      \"order\" : \"desc\",
      \"missing\" : \"_last\"
    }
  }
], LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={
  \"from\":0,
  \"size\":10000,
  \"timeout\":\"1m\",
  \"_source\":{
    \"includes\":[\"account_number\",\"firstname\",\"address\",\"balance\",\"gender\",\"city\",\"employer\",\"state\",\"age\",\"email\",\"lastname\"],
    \"excludes\":[]
  },
  \"sort\":[
    {\"balance\":{\"order\":\"asc\",\"missing\":\"_first\"}},
    {\"firstname.keyword\":{\"order\":\"desc\",\"missing\":\"_last\"}}
  ]
}, requestedTotalSize=10000, pageSize=null, startFrom=0)])

Explanation:
The two reverses cancel each other out. The final sort order is the same as the original query (balance ASC, firstname DESC).

3. Reverse after multiple sorts

Query:

source = accounts | sort + balance | sort - firstname | reverse

Logical Plan:

LogicalSystemLimit(sort0=[$1], dir0=[ASC-nulls-first], fetch=[10000], type=[QUERY_SIZE_LIMIT])
  LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])
  LogicalSort(sort0=[$1], dir0=[ASC-nulls-first])
    LogicalSort(sort0=[$1], dir0=[DESC-nulls-last])
      LogicalSort(sort0=[$3], dir0=[ASC-nulls-first])
        CalciteLogicalIndexScan(table=[[OpenSearch, accounts]])

Physical Plan:

CalciteEnumerableIndexScan(table=[[OpenSearch, accounts]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname], SORT->[
  {
    \"firstname.keyword\" : {
      \"order\" : \"asc\",
      \"missing\" : \"_first\"
    }
  }
], LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={
  \"from\":0,
  \"size\":10000,
  \"timeout\":\"1m\",
  \"_source\":{
    \"includes\":[\"account_number\",\"firstname\",\"address\",\"balance\",\"gender\",\"city\",\"employer\",\"state\",\"age\",\"email\",\"lastname\"],
    \"excludes\":[]
  },
  \"sort\":[
    {\"firstname.keyword\":{\"order\":\"asc\",\"missing\":\"_first\"}}
  ]
}, requestedTotalSize=10000, pageSize=null, startFrom=0)])

Explanation:
The last sort + reverse overrides the earlier sort + balance. The final pushed-down order is firstname ASC.

Related Issues

Resolves #3924

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• 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.

[Swiddis]

I'm curious about the cases where the double reverse optimization matters in practice, seems unlikely that someone would reverse something twice like that except by accident

[dai-chen]

Is logical optimization rule better way to do this? Or this is just edge case and usually users won't do reverse | reverse?

[penghuo]

+1, calcite opt-rule does not cover this?

[songkant-aws]

I think it's not necessary to check duplicate Reverse nodes. Calcite will ensure the top sort collation by overriding the child's collation with current one.

[LantaoJin]

I think it's not necessary to check duplicate Reverse nodes. Calcite will ensure the top sort collation by overriding the child's collation with current one.

For the previous implementation (adding row_number window function), Calcite can do nothing.

+1 to add a opt-rule, it could cover more cases:

... | reverse | reverse
... | reverse | fields ... | reverse
... | reverse | where ... | reverse

[selsong]

Thank you all for the excellent feedback about using Calcite optimization rules! After implementing and testing the SortDirectionOptRule, I discovered that you were right - Calcite's built-in optimization already handles sort merging effectively. The physical plans were identical with or without the custom rule.

Based on this, I've removed the custom optimization rule and now rely entirely on Calcite's mature optimization framework. The core improvement remains in PlanUtils: reverse detection that flips sort directions instead of using window functions.

[songkant-aws]

Be careful of using getTraitSet() to get fieldCollations because there is an edge case when it returns a different instance than RelCollation.

Sometimes, it returns a RelCompositeTrait instance containing all collation equivalent candidates. For example, this child PPL query source=EMP | sort SAL | eval a = cast(SAL as double) | where a > 2 will return RelCompositeTrait instance like [[5 ASC-nulls-first], [8]]. The eighth projected column is a cast expression but has the equivalent collation with [5 ASC-nulls-first]. The traitSet put equivalences together into a RelCompositeTrait.

A better way is probably List<RelCollation> collations = context.relBuilder.getCluster().getMetadataQuery().collations(context.relBuilder.peek()). You can pick the first valid RelCollation.

[songkant-aws]

Calcite has some issue of carrying NullDirection when deducing collation for expressions. See: https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/rel/metadata/RelMdCollation.java#L358-L360.

If we really care about reversing null direction, another option is we can find the closest sort node and reverse based on that collation.

[selsong]

Thanks! I switched from getTraitSet().getCollation() to getMetadataQuery().collations() to properly handle RelCompositeTrait instances and collation equivalences.
Null direction preservation: The optimization rule now properly handles and reverses null directions when flipping sort collations.

[RyanL1997]

Hi @selsong , thanks for putting this together, and I just left some comments.

[selsong]

I'm curious about the cases where the double reverse optimization matters in practice, seems unlikely that someone would reverse something twice like that except by accident

Thanks for the note! I agree. It’s extremely unlikely that users would intentionally issue a double reverse and there is no usage as of now. This PR is now only focused on flipping the sort direction, and Calcite's default optimization takes care of the double reverse.

[yuancu]

I'm a little about curious what's the behavior without this rule. Won't multiple sorts on the same fields be merged?

[selsong]

You're right. Multiple sorts on the same fields are merged so without this rule physical plans are identical due to Calcite's built-in physical optimization. I think this rule ensures consistent optimization across different query patterns, especially in post-aggregation scenarios where physical rules might not apply.

[songkant-aws]

Could you provide an example where default physical rule might not apply?

There is a SortRemoveRule in Calcite that changes the traits based on the Sort collation requirement. For this two sorts case, I think it will pick one of them anyway. Seems this optimization might be overlapped with existing rule.

In physical plan rules, Calcite will ensure query do not lose sort semantics by adding AbstractConverter node over applicable nodes that require sorting. The AbstractConverter node checks if the child collation satisfies the expected collation to decide whether to add a sort between parent and child. The sort added by AbstractConverter should be winner of the two sorts.

[selsong]

Thank you both for the insightful discussion! You were absolutely correct, and I was initially wrong in my assessment. After further testing and analysis, I've confirmed that:
Calcite's built-in optimization is sufficient - SortRemoveRule and AbstractConverter nodes handle sort merging effectively
Physical plans are identical with or without the custom rule, so the custom rule was redundant
Based on your feedback, I've removed the SortDirectionOptRule entirely. The core improvement remains: reverse detection that flips existing sort directions. Calcite's built-in optimization then handles the sort merging at the physical level.

[yuancu]

Are there specific reasons on overriding the method instead of using a config?

[selsong]

I think in this case override provides more control for complex matching logic like checking fetch limits and intermediate nodes.

[yuancu]

In my understanding, it falls back to the original implementation with window functions when there is no preceding sort. In this test, if there is not reverse, are the fields sort by $0 (EMPNO) by default?

If not, this test case should fallback.

[selsong]

Yes, EMP table has implicit ordering by EMPNO ($0) in test data. This is detected as an existing sort and converts reverse to a simple DESC sort instead of using window functions.

[yuancu]

Why is EMP table implicitly ordered by EMPNO? How is the order inferred / detected?

Update: I tried debugging, and found that the table does contain a collation trait with EMPNO sorted even with source=EMP. I don't know where is this collation from though.

[dai-chen]

interesting, that's why there is no test case for using ROW_NUMBER in this test right?

[songkant-aws]

I think this pattern needs careful consideration. There are lots of cases to be considered.

If intermediate is a Project node without WINDOW function, the pattern is supposed to be optimized by SortProjectTransposeRule. And it will be changed to Project - outerSort - innerSort. Other bunch of rules may optimize the changed pattern to keep outerSort.

If the Project contains WINDOW function, we should not change outerSort under the intermediate node. Because it may change its semantics.

If the intermediate node is aggregation, suppose the sort is on the group by column, I think the outerSort over aggregated result will be more efficient because the row count will be reduced a lot after aggregation. Check if Calcite can make it like outerSort(groupByColumn) - Aggregate(count) by groupByColumn

[selsong]

Thanks, you're right about these cases. Given that Calcite's built-in optimization already handles these cases correctly and efficiently, I've removed the custom optimization rule entirely. This eliminates the risk of interfering with Calcite's sophisticated rule interactions while preserving the core performance improvement.

[dai-chen]

interesting, that's why there is no test case for using ROW_NUMBER in this test right?

[dai-chen]

So we assume the head of this collation list must come from the sort command right before reverse command right? Is it always true or the reverse logic below works even though it's not true?

[selsong]

1. Yes, this test uses the EMP table which has a natural ordering by EMPNO. When reverse is called, Calcite's metadata query detects this existing collation and reverses it directly, so we see LogicalSort(sort0=[$0], dir0=[DESC]) instead of ROW_NUMBER. The ROW_NUMBER fallback only occurs when there's no existing collation.
2. No, the assumption isn't always true, but I think the logic works correctly when its not true. getMetadataQuery().collations() returns all collations Calcite detects (natural orderings, index orderings, sort operations), not just the immediate sort command. We take the first available RelCollation (which may contain multiple sort fields) and reverse all its field collations at once.

[dai-chen]

For Q2: Take query ... | sort | X | Y | Z | reverse for example, we should be good as long as commands in-between (X, Y, Z) doesn't rely on the output order of sort command, right? Just thinking any counterexample, such as ... | sort | head | reverse?

I'm thinking is it too early to do this in visitReverse? Essentially, our CalciteRelNodeVisitor converts AST to its logical representation by RelNode. Just wondering what does it look like if we simply translate reverse to logical sort operator here?

[selsong]

Great question about edge cases! I tested both | sort | head 10 | reverse and | sort | eval | head | reverse patterns and they work correctly. In the first part of visitReverse the SORT ASC and LIMIT for head are combined into LogicalSort(sort0=[$0], dir0=[ASC-nulls-first], fetch=[10]) and then the reverseCollation is added with LogicalSort(sort0=[$0], dir0=[DESC-nulls-last]). The physical plan pushes SORT ASC + LIMIT N to OpenSearch, then applies DESC sort on the limited results. Let me know if there are any other edge cases, Thanks!

[yuancu]

Is this explain result and explain_double_reverse_sort_no_op used somewhere? I guess they were used for the elimination rule of double reverse. Then you delete the rule and relevant tests

[ahkcs]

@selsong Can you follow up on this? If not, we can take it over

[opensearch-trigger-bot]

This PR is stalled because it has been open for 2 weeks with no activity.