misc: Move TopNRowNumberNode to presto-spi for connector optimizer access#27232
Merged
feilong-liu merged 1 commit intoprestodb:masterfrom Feb 27, 2026
Merged
misc: Move TopNRowNumberNode to presto-spi for connector optimizer access#27232feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu merged 1 commit intoprestodb:masterfrom
Conversation
feilong-liu
requested review from
a team,
elharo and
jaystarshot
as code owners
Contributor
Reviewer's GuideTopNRowNumberNode is moved into presto-spi and made a connector-accessible PlanNode, with corresponding visitor, optimizer, serialization, and native protocol updates to treat it like other SPI plan nodes and a small behavior tweak in replaceChildren/getOutputVariables. Sequence diagram for TopNRowNumberNode JSON protocol between Java and C++sequenceDiagram
participant PlannerJava as PlannerJava
participant JsonSer as JavaJsonSerializer
participant CppProtocol as CppProtocol
participant NativeExec as NativeExecutionEngine
PlannerJava->>JsonSer: serialize PlanNode tree
JsonSer->>JsonSer: detect TopNRowNumberNode
JsonSer->>JsonSer: set _type and @type to .TopNRowNumberNode
JsonSer-->>CppProtocol: JSON plan
CppProtocol->>CppProtocol: read @type field
CppProtocol->>CppProtocol: if type == .TopNRowNumberNode
CppProtocol->>CppProtocol: construct TopNRowNumberNode
CppProtocol->>CppProtocol: from_json populate fields
CppProtocol-->>NativeExec: TopNRowNumberNode instance
NativeExec->>NativeExec: execute plan with TopNRowNumberNode
Class diagram for SPI PlanNode hierarchy and TopNRowNumberNode moveclassDiagram
class PlanNode {
+PlanNodeId id
+SourceLocation sourceLocation
+PlanNode getStatsEquivalentPlanNode()
+List~PlanNode~ getSources()
+List~VariableReferenceExpression~ getOutputVariables()
+PlanNode replaceChildren(List~PlanNode~ newChildren)
+R accept(PlanVisitor~R, C~ visitor, C context)
}
class InternalPlanNode {
+PlanNodeId id
+SourceLocation sourceLocation
+PlanNode getStatsEquivalentPlanNode()
+List~PlanNode~ getSources()
+List~VariableReferenceExpression~ getOutputVariables()
+PlanNode replaceChildren(List~PlanNode~ newChildren)
+R accept(InternalPlanVisitor~R, C~ visitor, C context)
}
class TopNRowNumberNode {
+RankingFunction rankingFunction
+DataOrganizationSpecification specification
+VariableReferenceExpression rowNumberVariable
+Optional~VariableReferenceExpression~ hashVariable
+long maxRowCountPerPartition
+boolean partial
+PlanNode source
+List~PlanNode~ getSources()
+List~VariableReferenceExpression~ getOutputVariables()
+Optional~VariableReferenceExpression~ getHashVariable()
+PlanNode replaceChildren(List~PlanNode~ newChildren)
+R accept(PlanVisitor~R, C~ visitor, C context)
}
class RankingFunction {
<<enum>>
DENSE_RANK
RANK
ROW_NUMBER
}
class PlanVisitor~R, C~ {
+R visitPlan(PlanNode node, C context)
+R visitTopNRowNumber(TopNRowNumberNode node, C context)
+R visitMaterializedViewScan(MaterializedViewScanNode node, C context)
%% other visit methods for connector accessible nodes
}
class InternalPlanVisitor~R, C~ {
+R visitPlan(PlanNode node, C context)
+R visitRowNumber(RowNumberNode node, C context)
+R visitExchange(ExchangeNode node, C context)
%% no visitTopNRowNumber here after the change
}
class ApplyConnectorOptimization {
-Set~Class~ connectorAccessiblePlanNodes
+PlanNode optimize(PlanNode root)
}
PlanNode <|-- InternalPlanNode
PlanNode <|-- TopNRowNumberNode
RankingFunction <.. TopNRowNumberNode
DataOrganizationSpecification <.. TopNRowNumberNode
VariableReferenceExpression <.. TopNRowNumberNode
PlanVisitor <.. TopNRowNumberNode
InternalPlanVisitor <.. InternalPlanNode
PlanVisitor ..> TopNRowNumberNode : visitTopNRowNumber
ApplyConnectorOptimization ..> PlanNode : uses
ApplyConnectorOptimization ..> TopNRowNumberNode : in CONNECTOR_ACCESSIBLE_PLAN_NODES
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.cpp" line_range="755" />
<code_context>
return;
}
- if (type == "com.facebook.presto.sql.planner.plan.TopNRowNumberNode") {
+ if (type == ".TopNRowNumberNode") {
j = *std::static_pointer_cast<TopNRowNumberNode>(p);
return;
</code_context>
<issue_to_address>
**issue (bug_risk):** Changing the TopNRowNumberNode type key may break compatibility with older serialized plans.
Previously we deserialized using the FQCN key, and now we only accept the short ".TopNRowNumberNode" key. That will break deserialization for any existing persisted plans or mixed-version deployments still emitting the FQCN. If backward compatibility matters here, please either accept both keys during a transition period or clearly document this as a breaking change.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return; | ||
| } | ||
| if (type == "com.facebook.presto.sql.planner.plan.TopNRowNumberNode") { | ||
| if (type == ".TopNRowNumberNode") { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Changing the TopNRowNumberNode type key may break compatibility with older serialized plans.
Previously we deserialized using the FQCN key, and now we only accept the short ".TopNRowNumberNode" key. That will break deserialization for any existing persisted plans or mixed-version deployments still emitting the FQCN. If backward compatibility matters here, please either accept both keys during a transition period or clearly document this as a breaking change.
feilong-liu
changed the title
…restodb#27232) Summary: Pull Request resolved: prestodb#27232 Move TopNRowNumberNode from presto-main-base to presto-spi to make it accessible to connector optimizers in ApplyConnectorOptimization.java. Changes: - Move TopNRowNumberNode.java to presto-spi/src/main/java/com/facebook/presto/spi/plan/ - Change extends InternalPlanNode to extends PlanNode - Add visitTopNRowNumber to PlanVisitor.java - Remove visitTopNRowNumber from InternalPlanVisitor.java - Add TopNRowNumberNode to CONNECTOR_ACCESSIBLE_PLAN_NODES - Update all imports across the codebase - Update protocol YAML files to use .TopNRowNumberNode key - Regenerate C++ protocol files Differential Revision: D94627414
feilong-liu
force-pushed
the
export-D94627414
branch
from
97eb549 to
c635cfc
Compare
NikhilCollooru
approved these changes
Feb 27, 2026
feilong-liu
added a commit
that referenced
this pull request
Mar 2, 2026
…cess (#27232) ## Description Similar to changes in #24088 Summary: Move TopNRowNumberNode from presto-main-base to presto-spi to make it accessible to connector optimizers in ApplyConnectorOptimization.java. Changes: - Move TopNRowNumberNode.java to presto-spi/src/main/java/com/facebook/presto/spi/plan/ - Change extends InternalPlanNode to extends PlanNode - Add visitTopNRowNumber to PlanVisitor.java - Remove visitTopNRowNumber from InternalPlanVisitor.java - Add TopNRowNumberNode to CONNECTOR_ACCESSIBLE_PLAN_NODES - Update all imports across the codebase - Update protocol YAML files to use .TopNRowNumberNode key - Regenerate C++ protocol files Differential Revision: D94627414 ## Motivation and Context As in description ## Impact As in description ## Test Plan Existing unit tests ``` == NO RELEASE NOTE == ``` ## Summary by Sourcery Expose TopNRowNumberNode as a connector-accessible SPI plan node and align protocol/serialization support. Enhancements: - Move TopNRowNumberNode from presto-main-base into presto-spi as a PlanNode with a PlanVisitor hook so connector optimizers can see and transform it. - Tighten TopNRowNumberNode child handling and output variable construction using standard collections utilities instead of Guava. - Add a catchableByTry field to ErrorCode in the native protocol to support JSON round-tripping. Build: - Update Java/C++ protocol YAML mappings and generated native protocol code to use the shortened .TopNRowNumberNode type key instead of the old fully qualified class name.
feilong-liu
added a commit
that referenced
this pull request
Mar 2, 2026
…cess (#27232) ## Description Similar to changes in #24088 Summary: Move TopNRowNumberNode from presto-main-base to presto-spi to make it accessible to connector optimizers in ApplyConnectorOptimization.java. Changes: - Move TopNRowNumberNode.java to presto-spi/src/main/java/com/facebook/presto/spi/plan/ - Change extends InternalPlanNode to extends PlanNode - Add visitTopNRowNumber to PlanVisitor.java - Remove visitTopNRowNumber from InternalPlanVisitor.java - Add TopNRowNumberNode to CONNECTOR_ACCESSIBLE_PLAN_NODES - Update all imports across the codebase - Update protocol YAML files to use .TopNRowNumberNode key - Regenerate C++ protocol files Differential Revision: D94627414 ## Motivation and Context As in description ## Impact As in description ## Test Plan Existing unit tests ``` == NO RELEASE NOTE == ``` ## Summary by Sourcery Expose TopNRowNumberNode as a connector-accessible SPI plan node and align protocol/serialization support. Enhancements: - Move TopNRowNumberNode from presto-main-base into presto-spi as a PlanNode with a PlanVisitor hook so connector optimizers can see and transform it. - Tighten TopNRowNumberNode child handling and output variable construction using standard collections utilities instead of Guava. - Add a catchableByTry field to ErrorCode in the native protocol to support JSON round-tripping. Build: - Update Java/C++ protocol YAML mappings and generated native protocol code to use the shortened .TopNRowNumberNode type key instead of the old fully qualified class name.
This was referenced Mar 31, 2026
15 tasks
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 free
to 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
Similar to changes in #24088
Summary:
Move TopNRowNumberNode from presto-main-base to presto-spi to make it
accessible to connector optimizers in ApplyConnectorOptimization.java.
Changes:
Differential Revision: D94627414
Motivation and Context
As in description
Impact
As in description
Test Plan
Existing unit tests
Summary by Sourcery
Expose TopNRowNumberNode as a connector-accessible SPI plan node and align protocol/serialization support.
Enhancements:
Build: