chore(ci): Advance velox & Migrate presto-trunk from VectorSerde::Kind to string API#27262
Merged
han-yan01 merged 2 commits intoprestodb:masterfrom Mar 8, 2026
Merged
Conversation
Contributor
Reviewer's GuideMigrates presto-native execution code from the legacy velox::VectorSerde::Kind enum to the new string-based named serde API, updating server registration, operators, plan conversion, and tests to use serde names like "Presto" and "CompactRow" instead of enum values. Class diagram for migration to string-based VectorSerde APIclassDiagram
class PrestoServer {
+registerVectorSerdes() void
}
class BroadcastFileWriter {
+BroadcastFileWriter(...) void
}
class BroadcastWriteOperator {
+BroadcastWriteOperator(...) void
}
class ShuffleRead {
+ShuffleRead(...) void
}
class VeloxQueryPlanConverterBase {
+toVeloxQueryPlan(node, tableWriteInfo, taskId) core::PlanNodePtr
}
class VeloxBatchQueryPlanConverter {
+toVeloxQueryPlan(node, tableWriteInfo, taskId) core::PlanFragment
}
class PrestoToVeloxQueryPlan {
+toVeloxSerdeKind(encoding protocol::ExchangeEncoding) std::string
}
class VectorSerdeAPI {
+isRegisteredVectorSerde() bool
+isRegisteredNamedVectorSerde(name std::string) bool
+getNamedVectorSerde(name std::string) VectorSerdeAPI
+getVectorSerdeOptions(compressionKind, serdeName std::string) VectorSerdeOptions
}
class CoreExchangeNodes {
+ExchangeNode(id, rowType, serdeName std::string)
+PartitionedOutputNode(id, rowType, serdeName std::string)
}
PrestoServer ..> VectorSerdeAPI : registers_named_serdes
BroadcastFileWriter ..> VectorSerdeAPI : uses_getNamedVectorSerde
BroadcastWriteOperator ..> VectorSerdeAPI : uses_getVectorSerdeOptions
ShuffleRead ..> CoreExchangeNodes : creates_ExchangeNode_with_serdeName
VeloxQueryPlanConverterBase ..> CoreExchangeNodes : creates_PartitionedOutputNode_with_serdeName
VeloxBatchQueryPlanConverter ..> CoreExchangeNodes : creates_ExchangeNode_with_serdeName
PrestoToVeloxQueryPlan ..> CoreExchangeNodes : provides_serdeName
VeloxQueryPlanConverterBase ..> PrestoToVeloxQueryPlan : uses_toVeloxSerdeKind
VeloxBatchQueryPlanConverter ..> PrestoToVeloxQueryPlan : uses_toVeloxSerdeKind
PrestoServer ..> PrestoToVeloxQueryPlan : relies_on_serdeName_convention
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Now that serde kinds are plain strings in multiple places (e.g., "Presto", "CompactRow", "UnsafeRow" across tests, plan conversion, and server registration), consider centralizing these as named constants or an enum-like helper to avoid typos and keep the set of valid serde names consistent.
- The function
toVeloxSerdeKindinPrestoToVeloxQueryPlan.cppnow returnsstd::string; renaming it (e.g., totoVeloxSerdeNameor similar) would better reflect its behavior and reduce confusion with the legacyVectorSerde::Kind.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that serde kinds are plain strings in multiple places (e.g., "Presto", "CompactRow", "UnsafeRow" across tests, plan conversion, and server registration), consider centralizing these as named constants or an enum-like helper to avoid typos and keep the set of valid serde names consistent.
- The function `toVeloxSerdeKind` in `PrestoToVeloxQueryPlan.cpp` now returns `std::string`; renaming it (e.g., to `toVeloxSerdeName` or similar) would better reflect its behavior and reduce confusion with the legacy `VectorSerde::Kind`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary:
Migrate all presto-trunk files from legacy VectorSerde::Kind enum
to the new string-based named serde API. This is stacked on D94569860
which adds the string API and legacy Kind overloads for backward compat.
Changes:
- PrestoServer.cpp: use string names for serde registration checks
- BroadcastFile.cpp: getNamedVectorSerde("Presto")
- BroadcastWrite.cpp: getVectorSerdeOptions(..., "Presto")
- ShuffleRead.cpp: ExchangeNode with "CompactRow"
- BroadcastTest.cpp: exchange with "Presto"
- TaskManagerTest.cpp: parameterize on std::string instead of Kind
- PrestoToVeloxQueryPlan.cpp: toVeloxSerdeKind returns std::string
- PlanConverterTest.cpp: compare serdeKind() with "CompactRow"
Differential Revision: D95243859
627438f to
8e1eb7c
Compare
Contributor
|
@han-yan01 You need to advance velox in this PR |
amitkdutta
approved these changes
Mar 8, 2026
Contributor
amitkdutta
left a comment
There was a problem hiding this comment.
Looks good. Thanks @han-yan01
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
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.
Summary:
Migrate all presto-trunk files from legacy VectorSerde::Kind enum
to the new string-based named serde API. This is stacked on D94569860
which adds the string API and legacy Kind overloads for backward compat.
Changes:
Differential Revision: D95243859
Releas Notes
Summary by Sourcery
Migrate Presto native execution code from the legacy VectorSerde::Kind enum to the new string-based named serde API.
Enhancements: