feat(optimizer): Native TopNRank optimization #24138

aditi-pandit · 2024-11-25T10:54:29Z

Description

https://docs.google.com/document/d/1WQfNigR9bVrbM-PqY7F0mswcetN_tdNahzD9ENye-Q0/edit?tab=t.0

Motivation and Context

TPC-DS Q67

== RELEASE NOTES ==

General Changes
* Add Window filter pushdown in native engine for rank and dense_rank functions. Use session property `optimizer.optimize-top-n-rank` to enable the rewrite.

BryanCutler

LGTM just a couple questions

BryanCutler · 2024-12-10T19:14:48Z

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/TopNRowNumberNode.java

+            @JsonProperty("rankingType") RankingFunction rankingFunction,
            @JsonProperty("rowNumberVariable") VariableReferenceExpression rowNumberVariable,
-            @JsonProperty("maxRowCountPerPartition") int maxRowCountPerPartition,
+            @JsonProperty("maxRowCountPerPartition") int maxRankPerPartition,


Should the json property be changed too or is that not possible without breaking compat?

BryanCutler · 2024-12-11T17:53:47Z

...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java

                        // Write config file
                        String configProperties = format("discovery.uri=%s%n" +
                                "presto.version=testversion%n" +
+                                "native-execution-enabled=true" +


does this need a %n? I'm guessing that's to give it a newline but not sure

steveburnett · 2025-01-09T15:37:29Z

Thank you for the release note! Some nits of phrasing and formatting suggestions.

== RELEASE NOTES ==

Prestissimo (Native Execution) Changes
* Improve Window filter pushdown in Prestissimo engine for ``rank`` and ``dense_rank`` functions. :pr:`24138`

steveburnett · 2025-03-25T19:45:19Z

One more nit about the release note entry: we've automated the link to the PR so you don't have to add it manually anymore.

== RELEASE NOTES ==

Prestissimo (Native Execution) Changes
* Improve Window filter pushdown in Prestissimo engine for ``rank`` and ``dense_rank`` functions.

Summary: Design doc : https://docs.google.com/document/d/1WQfNigR9bVrbM-PqY7F0mswcetN_tdNahzD9ENye-Q0/edit?usp=sharing https://github.com/facebookincubator/velox/discussions/9404 e2e Presto PR (with changes in the Presto optimizer as well) prestodb/presto#24138 Latency for SF1K TPC-DS Q67 fell from 399s to 146s with this change. Pull Request resolved: #14141 Reviewed By: pratikpugalia Differential Revision: D85763891 Pulled By: kevinwilfong fbshipit-source-id: 122bc6d60a4cfbf90c7921ebb1df28ef6edcc0a0

sourcery-ai · 2025-11-08T20:13:20Z

Reviewer's Guide

This PR extends the existing TopNRowNumber optimization to support rank and dense_rank window functions when native execution is enabled. The window filter pushdown logic is generalized, a new RankingFunction enum is added to TopNRowNumberNode (Java and protocol layers), configuration properties and session flags for optimize-top-n-rank are introduced, and the Velox plan converter and serialization code are updated accordingly. New tests cover both limit- and filter-based pushdowns for rank and dense_rank.

Sequence diagram for TopNRank optimization in WindowFilterPushDown

sequenceDiagram
    participant "WindowFilterPushDown"
    participant "WindowNode"
    participant "TopNRowNumberNode"
    participant "Session"
    "WindowFilterPushDown"->>"WindowNode": Check if canOptimizeRankFunction
    "WindowFilterPushDown"->>"Session": Check isOptimizeTopNRank & isNativeExecutionEnabled
    alt If conditions met
        "WindowFilterPushDown"->>"TopNRowNumberNode": convertToTopNRank(windowNode, limit)
    end

Class diagram for updated TopNRowNumberNode with RankingFunction

classDiagram
    class TopNRowNumberNode {
        +PlanNode source
        +DataOrganizationSpecification specification
        +RankingFunction rankingFunction
        +VariableReferenceExpression rowNumberVariable
        +int maxRowCountPerPartition
        +boolean partial
        +Optional<VariableReferenceExpression> hashVariable
        +RankingFunction getRankingFunction()
    }
    class RankingFunction {
        <<enum>>
        ROW_NUMBER
        RANK
        DENSE_RANK
    }
    TopNRowNumberNode --> RankingFunction

Class diagram for FeaturesConfig and SystemSessionProperties changes

classDiagram
    class FeaturesConfig {
        +boolean optimizeTopNRowNumber
        +boolean optimizeTopNRank
        +boolean isOptimizeTopNRowNumber()
        +boolean isOptimizeTopNRank()
        +FeaturesConfig setOptimizeTopNRowNumber(boolean)
        +FeaturesConfig setOptimizeTopNRank(boolean)
    }
    class SystemSessionProperties {
        +static final String OPTIMIZE_TOP_N_ROW_NUMBER
        +static final String OPTIMIZE_TOP_N_RANK
        +static boolean isOptimizeTopNRowNumber(Session)
        +static boolean isOptimizeTopNRank(Session)
    }
    SystemSessionProperties ..> FeaturesConfig

Class diagram for protocol TopNRowNumberNode and RankingFunction

classDiagram
    class TopNRowNumberNode {
        +shared_ptr<PlanNode> source
        +DataOrganizationSpecification specification
        +RankingFunction rankingType
        +VariableReferenceExpression rowNumberVariable
        +int maxRowCountPerPartition
        +bool partial
    }
    class RankingFunction {
        <<enum>>
        ROW_NUMBER
        RANK
        DENSE_RANK
    }
    TopNRowNumberNode --> RankingFunction

File-Level Changes

Change	Details	Files
Generalize WindowFilterPushDown to handle rank and dense_rank	Added canOptimizeRankFunction check alongside row number Created convertToTopNRank for constructing TopNRowNumberNode with appropriate RankingFunction Updated visitLimit and visitFilter to branch on optimizeTopNRank and native execution	`WindowFilterPushDown.java`
Enhance TopNRowNumberNode to carry RankingFunction	Added RankingFunction enum (ROW_NUMBER, RANK, DENSE_RANK) Injected rankingFunction field into constructors, JSON annotations and getters Updated GraphvizPrinter, replaceChildren, assignStatsEquivalentPlanNode, canonical plan, and various optimizer rules to pass rankingFunction	`TopNRowNumberNode.java` `GraphvizPrinter.java` `CanonicalPlanGenerator.java` `multiple planner rule classes`
Support RankingFunction in protocol and Velox conversion	Defined RankingFunction enum with JSON to_json/from_json handlers Extended presto_protocol_core.cpp/h with enum serialization Mapped rankingType to Velox RankFunction in PrestoToVeloxQueryPlan	`presto_protocol_core.*` `PrestoToVeloxQueryPlan.cpp`
Introduce optimize-top-n-rank configuration	Added optimizeTopNRank flag in FeaturesConfig and SystemSessionProperties Exposed OPTIMIZE_TOP_N_RANK session property and isOptimizeTopNRank helper Updated TestFeaturesConfig mappings	`FeaturesConfig.java` `SystemSessionProperties.java` `TestFeaturesConfig.java`
Add tests for rank/dense_rank pushdowns	Refactored TestWindowFilterPushDown to parameterize limit and filter scenarios Extended AbstractTestNativeWindowQueries and TestPrestoNativeWindowQueries to validate TopN for rank and dense_rank Added session helpers optimizeTopNRank and optimizeTopNRankWithoutNative in tests	`TestWindowFilterPushDown.java` `AbstractTestNativeWindowQueries.java` `TestPrestoNativeWindowQueries.java`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

Refactor WindowFilterPushDown's visitLimit and visitFilter conditionals to extract the common logic for row_number/rank, reducing duplication and improving readability.
Merge convertToTopNRowNumber and convertToTopNRank into a single method parametrized by RankingFunction to avoid near-duplicate code paths.
Use NLOHMANN_JSON_SERIALIZE_ENUM for RankingFunction in C++ to eliminate manual to_json/from_json boilerplate.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Refactor WindowFilterPushDown's visitLimit and visitFilter conditionals to extract the common logic for row_number/rank, reducing duplication and improving readability.
- Merge convertToTopNRowNumber and convertToTopNRank into a single method parametrized by RankingFunction to avoid near-duplicate code paths.
- Use NLOHMANN_JSON_SERIALIZE_ENUM for RankingFunction in C++ to eliminate manual to_json/from_json boilerplate.

## Individual Comments

### Comment 1
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java:158-159` </location>
<code_context>
                 source = rowNumberNode;
             }
-            else if (source instanceof WindowNode && canOptimizeWindowFunction((WindowNode) source, metadata.getFunctionAndTypeManager()) && isOptimizeTopNRowNumber(session)) {
+            else if (source instanceof WindowNode && canOptimizeRowNumberFunction((WindowNode) source, metadata.getFunctionAndTypeManager()) && isOptimizeTopNRowNumber(session) ||
+                    source instanceof WindowNode && canOptimizeRankFunction((WindowNode) source, metadata.getFunctionAndTypeManager()) && isOptimizeTopNRank(session) && isNativeExecutionEnabled(session)) {
                 WindowNode windowNode = (WindowNode) source;
                 // verify that unordered row_number window functions are replaced by RowNumberNode
</code_context>

<issue_to_address>
**issue:** Operator precedence may lead to unexpected logic in conditional statement.

Add parentheses to clarify how '||' and '&&' are grouped in the condition, ensuring the intended logic and correct short-circuit behavior.
</issue_to_address>

### Comment 2
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java:163` </location>
<code_context>
                 // verify that unordered row_number window functions are replaced by RowNumberNode
                 verify(windowNode.getOrderingScheme().isPresent());
-                TopNRowNumberNode topNRowNumberNode = convertToTopNRowNumber(windowNode, limit);
+                TopNRowNumberNode topNRowNumberNode = convertToTopNRank(windowNode, limit);
                 if (windowNode.getPartitionBy().isEmpty()) {
                     return topNRowNumberNode;
</code_context>

<issue_to_address>
**suggestion:** Method name 'convertToTopNRank' may be misleading for DENSE_RANK support.

Since the method handles both RANK and DENSE_RANK, a more general name like 'convertToTopNRanking' would improve clarity.

Suggested implementation:

```java
                TopNRowNumberNode topNRowNumberNode = convertToTopNRanking(windowNode, limit);

```

You will also need to:
1. Rename the actual method definition from `convertToTopNRank` to `convertToTopNRanking` elsewhere in the codebase (likely in this file or a related class).
2. Update all other usages of `convertToTopNRank` to `convertToTopNRanking` to maintain consistency.
</issue_to_address>

### Comment 3
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java:299-304` </location>
<code_context>
+        {
+
+            String windowFunctionName = Iterables.getOnlyElement(windowNode.getWindowFunctions().values()).getFunctionCall().getFunctionHandle().getName();
+            TopNRowNumberNode.RankingFunction rankingFunction =
+                    windowFunctionName.equals("row_number") ?
+                            TopNRowNumberNode.RankingFunction.ROW_NUMBER :
+                            windowFunctionName.equals("rank") ?
+                                    TopNRowNumberNode.RankingFunction.RANK : TopNRowNumberNode.RankingFunction.DENSE_RANK;
+
+            return new TopNRowNumberNode(
</code_context>

<issue_to_address>
**suggestion:** Ranking function selection logic may not handle unexpected function names robustly.

Explicitly handle or validate unexpected function names to prevent incorrect defaulting to DENSE_RANK.

```suggestion
            String windowFunctionName = Iterables.getOnlyElement(windowNode.getWindowFunctions().values()).getFunctionCall().getFunctionHandle().getName();
            TopNRowNumberNode.RankingFunction rankingFunction;
            switch (windowFunctionName) {
                case "row_number":
                    rankingFunction = TopNRowNumberNode.RankingFunction.ROW_NUMBER;
                    break;
                case "rank":
                    rankingFunction = TopNRowNumberNode.RankingFunction.RANK;
                    break;
                case "dense_rank":
                    rankingFunction = TopNRowNumberNode.RankingFunction.DENSE_RANK;
                    break;
                default:
                    throw new IllegalArgumentException("Unsupported window function for TopNRowNumberNode: " + windowFunctionName);
            }
```
</issue_to_address>

### Comment 4
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java:332-338` </location>
<code_context>
             return isRowNumberMetadata(functionAndTypeManager, functionAndTypeManager.getFunctionMetadata(node.getWindowFunctions().get(rowNumberVariable).getFunctionHandle()));
         }

+        private static boolean canOptimizeRankFunction(WindowNode node, FunctionAndTypeManager functionAndTypeManager)
+        {
+            if (node.getWindowFunctions().size() != 1) {
+                return false;
+            }
+            VariableReferenceExpression rowNumberVariable = getOnlyElement(node.getWindowFunctions().keySet());
+            return isRankMetadata(functionAndTypeManager, functionAndTypeManager.getFunctionMetadata(node.getWindowFunctions().get(rowNumberVariable).getFunctionHandle()));
+        }
+
</code_context>

<issue_to_address>
**suggestion:** Method name 'canOptimizeRankFunction' may not reflect DENSE_RANK support.

Consider renaming the method to 'canOptimizeRankingFunction' to accurately reflect its support for both RANK and DENSE_RANK functions.

Suggested implementation:

```java
        private static boolean canOptimizeRankingFunction(WindowNode node, FunctionAndTypeManager functionAndTypeManager)
        {
            if (node.getWindowFunctions().size() != 1) {
                return false;
            }
            VariableReferenceExpression rowNumberVariable = getOnlyElement(node.getWindowFunctions().keySet());
            return isRankMetadata(functionAndTypeManager, functionAndTypeManager.getFunctionMetadata(node.getWindowFunctions().get(rowNumberVariable).getFunctionHandle()));
        }

```

If there are any calls to `canOptimizeRankFunction` elsewhere in this file (or in related files), they should also be renamed to `canOptimizeRankingFunction` to match the new method name.
</issue_to_address>

### Comment 5
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/planner/plan/TopNRowNumberNode.java:38-43` </location>
<code_context>
 public final class TopNRowNumberNode
         extends InternalPlanNode
 {
+    public enum RankingFunction
+    {
+        ROW_NUMBER,
+        RANK,
+        DENSE_RANK
+    }
+
</code_context>

<issue_to_address>
**suggestion:** RankingFunction enum lacks explicit serialization values.

Explicitly assign string values to each enum constant to ensure stable serialization, especially if enum names change during refactoring.

```suggestion
    public enum RankingFunction
    {
        ROW_NUMBER("row_number"),
        RANK("rank"),
        DENSE_RANK("dense_rank");

        private final String serializationValue;

        RankingFunction(String serializationValue)
        {
            this.serializationValue = serializationValue;
        }

        public String getSerializationValue()
        {
            return serializationValue;
        }

        @Override
        public String toString()
        {
            return serializationValue;
        }

        public static RankingFunction fromSerializationValue(String value)
        {
            for (RankingFunction function : RankingFunction.values()) {
                if (function.serializationValue.equals(value)) {
                    return function;
                }
            }
            throw new IllegalArgumentException("Unknown serialization value: " + value);
        }
    }
```
</issue_to_address>

### Comment 6
<location> `presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.cpp:11612` </location>
<code_context>
+// Loosly copied this here from NLOHMANN_JSON_SERIALIZE_ENUM()
</code_context>

<issue_to_address>
**nitpick (typo):** Typo in comment: 'Loosly' should be 'Loosely'.

```suggestion
// Loosely copied this here from NLOHMANN_JSON_SERIALIZE_ENUM()
```
</issue_to_address>

### Comment 7
<location> `presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp:1803` </location>
<code_context>
 }

+namespace {
+core::TopNRowNumberNode::RankFunction prestoToVeloxRankFunction(
+    protocol::RankingFunction rankingFunction) {
+  switch (rankingFunction) {
</code_context>

<issue_to_address>
**issue (review_instructions):** The function 'prestoToVeloxRankFunction' uses PascalCase, but functions should use camelCase naming.

Please rename 'prestoToVeloxRankFunction' to 'prestoToVeloxRankFunction' using camelCase (e.g., 'prestoToVeloxRankFunction' -> 'prestoToVeloxRankFunction'). Functions should use camelCase per the naming conventions.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `presto-native-execution/**/*.hpp,presto-native-execution/**/*.hpp,presto-native-execution/**/*.cpp`

**Instructions:**
Use PascalCase for types (classes, structs, enums, type aliases, type template parameters) and file names.

</details>
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

...n-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java

presto-main-base/src/main/java/com/facebook/presto/sql/planner/plan/TopNRowNumberNode.java

presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.cpp

presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

Consider refactoring the repeated source instanceof WindowNode and session flag checks in WindowFilterPushDown into a shared helper predicate to reduce duplication and improve readability.
Instead of splitting the function name string in convertToTopNRank, leverage the FunctionHandle or metadata API directly to determine the ranking type for better robustness.
Rename the boolean parameter rowOrRank in the test helper methods to a more descriptive name or switch to an enum to make the test logic clearer and self-documenting.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider refactoring the repeated `source instanceof WindowNode` and session flag checks in WindowFilterPushDown into a shared helper predicate to reduce duplication and improve readability.
- Instead of splitting the function name string in convertToTopNRank, leverage the FunctionHandle or metadata API directly to determine the ranking type for better robustness.
- Rename the boolean parameter `rowOrRank` in the test helper methods to a more descriptive name or switch to an enum to make the test logic clearer and self-documenting.

## Individual Comments

### Comment 1
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/planner/plan/TopNRowNumberNode.java:47` </location>
<code_context>
+
     private final PlanNode source;
     private final DataOrganizationSpecification specification;
+    private final RankingFunction rankingFunction;
     private final VariableReferenceExpression rowNumberVariable;
     private final int maxRowCountPerPartition;
</code_context>

<issue_to_address>
**suggestion (bug_risk):** rankingFunction is a required field but not validated.

Add a null check or validation for rankingFunction in the constructor to prevent runtime errors from accidental null assignments.

Suggested implementation:

```java
    private final RankingFunction rankingFunction;

```

```java
            @JsonProperty("rankingType") RankingFunction rankingFunction,

```

```java
            @JsonProperty("rowNumberVariable") VariableReferenceExpression rowNumberVariable,
            @JsonProperty("maxRowCountPerPartition") int maxRowCountPerPartition,
            @JsonProperty("partial") boolean partial,
            @JsonProperty("hashVariable") Optional<VariableReferenceExpression> hashVariable)
    {
        this.rankingFunction = java.util.Objects.requireNonNull(rankingFunction, "rankingFunction is null");

```
</issue_to_address>

### Comment 2
<location> `presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.cpp:11612` </location>
<code_context>
 }
 } // namespace facebook::presto::protocol
 namespace facebook::presto::protocol {
+// Loosly copied this here from NLOHMANN_JSON_SERIALIZE_ENUM()
+
+// NOLINTNEXTLINE: cppcoreguidelines-avoid-c-arrays
</code_context>

<issue_to_address>
**nitpick (typo):** Typo in comment: 'Loosly' should be 'Loosely'.

```suggestion
// Loosely copied this here from NLOHMANN_JSON_SERIALIZE_ENUM()
```
</issue_to_address>

### Comment 3
<location> `presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp:1803` </location>
<code_context>
 }

+namespace {
+core::TopNRowNumberNode::RankFunction prestoToVeloxRankFunction(
+    protocol::RankingFunction rankingFunction) {
+  switch (rankingFunction) {
</code_context>

<issue_to_address>
**issue (review_instructions):** The function 'prestoToVeloxRankFunction' uses PascalCase, but functions should use camelCase naming.

Please rename 'prestoToVeloxRankFunction' to 'prestoToVeloxRankFunction' using camelCase (e.g., 'prestoToVeloxRankFunction' -> 'prestoToVeloxRankFunction'). This will ensure consistency with the function naming convention.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `presto-native-execution/**/*.hpp,presto-native-execution/**/*.hpp,presto-native-execution/**/*.cpp`

**Instructions:**
Use PascalCase for types (classes, structs, enums, type aliases, type template parameters) and file names.

</details>
</issue_to_address>

### Comment 4
<location> `presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.cpp:11615` </location>
<code_context>
+// Loosly copied this here from NLOHMANN_JSON_SERIALIZE_ENUM()
+
+// NOLINTNEXTLINE: cppcoreguidelines-avoid-c-arrays
+static const std::pair<RankingFunction, json> RankingFunction_enum_table[] =
+    { // NOLINT: cert-err58-cpp
+        {RankingFunction::ROW_NUMBER, "ROW_NUMBER"},
</code_context>

<issue_to_address>
**issue (review_instructions):** RankingFunction_enum_table is a static constant and should use kPascalCase naming, not snake_case.

Per the instructions, static constants should use the kPascalCase naming convention, e.g., kRankingFunctionEnumTable.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `presto-native-execution/**/*.hpp,presto-native-execution/**/*.cpp`

**Instructions:**
Use UPPER_SNAKE_CASE for macros.

</details>
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

presto-main-base/src/main/java/com/facebook/presto/sql/planner/plan/TopNRowNumberNode.java

presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.cpp

presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp

steveburnett · 2025-11-12T15:31:53Z

Thanks for the revised release note! Two things, the first is only a nit.

The PR link ( :pr:24138 ) is now automatically added and we don't have to manually include it anymore.
I don't find optimizer.optimize-top-n-rank in the Presto documentation. Could doc be added for it?

aditi-pandit · 2025-11-13T04:04:38Z

Thanks for the revised release note! Two things, the first is only a nit.

The PR link ( :pr:24138 ) is now automatically added and we don't have to manually include it anymore.

I don't find optimizer.optimize-top-n-rank in the Presto documentation. Could doc be added for it?

@steveburnett : Thanks for your review. Yes, we should add documentation for optimizer.optimize-top-n-rank.

Though its an interesting one as its applicable only for native engine. So I was undecided if I should document it with th native engine session properties here or the optimizer ones which are usually applicable to both engines. I'm more inclined to add it with the native engine config. what is your suggestion ?

steveburnett · 2025-11-13T14:56:00Z

@steveburnett : Thanks for your review. Yes, we should add documentation for optimizer.optimize-top-n-rank.

Though its an interesting one as its applicable only for native engine. So I was undecided if I should document it with th native engine session properties here or the optimizer ones which are usually applicable to both engines. I'm more inclined to add it with the native engine config. what is your suggestion ?

I agree with you that I'm more inclined to add it with the native engine config, and I think the best thing would be to add doc for it to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/presto_cpp/properties-session.rst.

aditi-pandit · 2025-11-14T00:17:29Z

I agree with you that I'm more inclined to add it with the native engine config, and I think the best thing would be to add doc for it to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/presto_cpp/properties-session.rst.

@steveburnett : Added documentation. Please review.

steveburnett

Thank you for the documentation! Just a couple of nits of formatting.

presto-docs/src/main/sphinx/presto_cpp/properties-session.rst

hantangwangd

@aditi-pandit thanks for adding the optimizer part. I've left some initial comments for discussion.

hantangwangd · 2025-11-17T08:49:35Z

...n-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java

+            VariableReferenceExpression rowNumberVariable = getOnlyElement(node.getWindowFunctions().keySet());
+            return isRankMetadata(functionAndTypeManager, functionAndTypeManager.getFunctionMetadata(node.getWindowFunctions().get(rowNumberVariable).getFunctionHandle()));


Suggested change

VariableReferenceExpression rowNumberVariable = getOnlyElement(node.getWindowFunctions().keySet());

return isRankMetadata(functionAndTypeManager, functionAndTypeManager.getFunctionMetadata(node.getWindowFunctions().get(rowNumberVariable).getFunctionHandle()));

return isRankMetadata(functionAndTypeManager, functionAndTypeManager.getFunctionMetadata(getOnlyElement(node.getWindowFunctions().values()).getFunctionHandle()));

nit: it seems unnecessary to fetch the key first and then use it to get the value.

Had copied this from canOptimizeRowNumberFunction logic. Have modified this code as well as the above function.

hantangwangd · 2025-11-17T08:55:40Z

...n-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java

-                if (windowNode.getPartitionBy().isEmpty()) {
-                    return topNRowNumberNode;
+                if (canReplaceWithTopNRowNumber(windowNode)) {
+                    TopNRowNumberNode topNRowNumberNode = convertToTopNRowNumber(windowNode, limit);


Suggested change

TopNRowNumberNode topNRowNumberNode = convertToTopNRowNumber(windowNode, limit);

TopNRowNumberNode topNRowNumberNode = convertToTopNRank(windowNode, limit);

Should we call convertToTopNRank here?

This was a good catch... I intended to remove the original convertToTopNRowNumber and change convertToTopNRank function to convertToTopNRowNumber to match the naming everywhere.

Have fixed that.

hantangwangd · 2025-11-17T09:09:14Z

...n-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java

+                    OptionalInt upperBound = extractUpperBound(tupleDomain, rowNumberVariable);
+
+                    if (upperBound.isPresent()) {
+                        source = convertToTopNRowNumber(windowNode, upperBound.getAsInt());


nit: It seems that we can call convertToTopNRank here as well. And completely drop the method convertToTopNRowNumber?

Same comment as above.

hantangwangd · 2025-11-17T09:37:42Z

...n-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java

+                    if (windowNode.getPartitionBy().isEmpty()) {
+                        return topNRowNumberNode;
+                    }


Not very sure, but is it correct to simply remove the LimitNode for rank and dense_rank here? I'm not familiar with the actual implementation of TopNRowNumber for rank and dense_rank in Velox. But as I understand from the design doc, it's possible that they could return more rows than the specified limit. Is my understanding correct? PLMK if there are any errors.

+1, I think we should only return directly if the RankingFunction is ROW_NUMBER

My bad. Yes, this is only applicable for row_number()

aaneja · 2025-11-17T13:32:41Z

...se/src/test/java/com/facebook/presto/sql/planner/optimizations/TestWindowFilterPushDown.java

        assertPlanWithSession(
                sql,
-                optimizeTopNRowNumber(true),
+                rowNumber ? optimizeTopNRowNumber(true) : optimizeTopNRank(true),


Can we also assert on the Ranking function for the TopNRowNumberNode

@aaneja : That's a good idea. Though do you have any suggestions on how to cleanly do so ? An option could be to enhance the matcher classes. wdyt ?

steveburnett

Thanks for the update! Just one nit of formatting, everything else looks good.

steveburnett · 2025-11-17T15:21:56Z

presto-docs/src/main/sphinx/presto_cpp/properties-session.rst

 velox::core::NestedLoopJoinNode.
+
+``optimizer.optimize_top_n_rank``
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


Suggested change

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

aditi-pandit · 2025-11-22T01:53:01Z

@hantangwangd @aaneja : Thanks for your review comments. Have addressed them. PTAL.

hantangwangd

Hi @aditi-pandit, thanks for the fixes. I'm seeing some test failures that appear to be related.

aditi-pandit requested review from a team, ClarenceThreepwood, feilong-liu and jaystarshot as code owners November 25, 2024 10:54

aditi-pandit requested a review from presto-oss November 25, 2024 10:54

prestodb-ci added the from:IBM PR from IBM label Nov 25, 2024

prestodb-ci requested review from a team, BryanCutler and imjalpreet and removed request for a team November 25, 2024 10:54

aditi-pandit force-pushed the topn branch from e9fb577 to 99600e4 Compare November 25, 2024 13:27

aditi-pandit mentioned this pull request Nov 25, 2024

(Do not review) feat: TopNRank optimization facebookincubator/velox#11554

Closed

aditi-pandit force-pushed the topn branch 2 times, most recently from b45c503 to ff8a3b5 Compare November 27, 2024 06:29

BryanCutler reviewed Dec 11, 2024

View reviewed changes

aditi-pandit force-pushed the topn branch from e835e9c to aac1159 Compare January 9, 2025 00:20

aditi-pandit force-pushed the topn branch from 56c6d54 to e9246e6 Compare January 10, 2025 02:59

aaneja self-requested a review January 14, 2025 04:09

aditi-pandit mentioned this pull request Feb 11, 2025

fix(iceberg): Date partition value parse issue facebookincubator/velox#12126

Closed

aditi-pandit force-pushed the topn branch 3 times, most recently from a63a0e9 to 2118f4c Compare March 25, 2025 19:33

aditi-pandit force-pushed the topn branch 2 times, most recently from 35f18b2 to 4b96592 Compare March 25, 2025 20:06

aditi-pandit force-pushed the topn branch from 4b96592 to 5c1610d Compare April 4, 2025 04:41

aditi-pandit mentioned this pull request Sep 24, 2025

feat: Add TopNRank optimization facebookincubator/velox#14141

Closed

sourcery-ai bot reviewed Nov 8, 2025

View reviewed changes

aditi-pandit force-pushed the topn branch 2 times, most recently from 3319161 to 75d0257 Compare November 8, 2025 20:23

aditi-pandit marked this pull request as draft November 8, 2025 21:24

aditi-pandit force-pushed the topn branch 3 times, most recently from 5013a95 to a3e9352 Compare November 8, 2025 23:57

aditi-pandit marked this pull request as ready for review November 10, 2025 06:42

sourcery-ai bot reviewed Nov 10, 2025

View reviewed changes

aditi-pandit force-pushed the topn branch 2 times, most recently from 18a6e17 to 8791f96 Compare November 12, 2025 08:08

feat(native): Add TopNRowNumberNode rank function plumbing to Velox

e6a3d72

aditi-pandit force-pushed the topn branch from 8791f96 to e0a0fb8 Compare November 14, 2025 00:15

aditi-pandit requested review from elharo and steveburnett as code owners November 14, 2025 00:15

aditi-pandit force-pushed the topn branch from e0a0fb8 to 0b1ba66 Compare November 14, 2025 01:02

steveburnett requested changes Nov 14, 2025

View reviewed changes

presto-docs/src/main/sphinx/presto_cpp/properties-session.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/presto_cpp/properties-session.rst Outdated Show resolved Hide resolved

aditi-pandit force-pushed the topn branch from 0b1ba66 to 88d5e0c Compare November 15, 2025 00:06

hantangwangd reviewed Nov 17, 2025

View reviewed changes

aaneja reviewed Nov 17, 2025

View reviewed changes

steveburnett requested changes Nov 17, 2025

View reviewed changes

feat(optimizer): Add WindowFilterPushdown rule changes for rank queries

e58b6c2

aditi-pandit force-pushed the topn branch from 88d5e0c to e58b6c2 Compare November 22, 2025 00:52

hantangwangd reviewed Nov 24, 2025

View reviewed changes

		VariableReferenceExpression rowNumberVariable = getOnlyElement(node.getWindowFunctions().keySet());
		return isRankMetadata(functionAndTypeManager, functionAndTypeManager.getFunctionMetadata(node.getWindowFunctions().get(rowNumberVariable).getFunctionHandle()));

	TopNRowNumberNode topNRowNumberNode = convertToTopNRowNumber(windowNode, limit);
	TopNRowNumberNode topNRowNumberNode = convertToTopNRank(windowNode, limit);

	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

feat(optimizer): Native TopNRank optimization #24138

Are you sure you want to change the base?

feat(optimizer): Native TopNRank optimization #24138

Conversation

aditi-pandit commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Uh oh!

BryanCutler left a comment

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!

steveburnett commented Jan 9, 2025

Uh oh!

steveburnett commented Mar 25, 2025

Uh oh!

sourcery-ai bot commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

Sequence diagram for TopNRank optimization in WindowFilterPushDown

Class diagram for updated TopNRowNumberNode with RankingFunction

Class diagram for FeaturesConfig and SystemSessionProperties changes

Class diagram for protocol TopNRowNumberNode and RankingFunction

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveburnett commented Nov 12, 2025

Uh oh!

aditi-pandit commented Nov 13, 2025

Uh oh!

steveburnett commented Nov 13, 2025

Uh oh!

aditi-pandit commented Nov 14, 2025

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit Nov 22, 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!

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

Uh oh!

aditi-pandit commented Nov 25, 2024 •

edited

Loading

sourcery-ai bot commented Nov 8, 2025 •

edited

Loading

aditi-pandit Nov 22, 2025 •

edited

Loading

hantangwangd Nov 17, 2025 •

edited

Loading

aditi-pandit Nov 22, 2025 •

edited

Loading