-
Couldn't load subscription status.
- Fork 5.5k
refactor: Separate HivePrestoToVeloxConnector to standalone file #26380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Separate HivePrestoToVeloxConnector to standalone file #26380
Conversation
Reviewer's GuideThe PR refactors the Hive connector by moving all Hive-specific conversion logic and class implementation out of the generic PrestoToVeloxConnector into a dedicated HivePrestoToVeloxConnector module and consolidates shared helper functions into a utils file, updating build and includes accordingly. Class diagram for refactored HivePrestoToVeloxConnectorclassDiagram
class PrestoToVeloxConnector {
<<abstract>>
+connectorName_: string
+~PrestoToVeloxConnector()
+toVeloxSplit(...)
+toVeloxColumnHandle(...)
+toVeloxTableHandle(...)
+toVeloxInsertTableHandle(...)
+createVeloxPartitionFunctionSpec(...)
+createConnectorProtocol()
}
class HivePrestoToVeloxConnector {
+HivePrestoToVeloxConnector(connectorName: string)
+toVeloxSplit(...)
+toVeloxColumnHandle(...)
+toVeloxTableHandle(...)
+toVeloxInsertTableHandle(...)
+createVeloxPartitionFunctionSpec(...)
+createConnectorProtocol()
-toHiveColumns(...)
}
PrestoToVeloxConnector <|-- HivePrestoToVeloxConnector
class TpchPrestoToVeloxConnector {
+TpchPrestoToVeloxConnector(connectorName: string)
+toVeloxSplit(...)
+toVeloxColumnHandle(...)
+toVeloxTableHandle(...)
+toVeloxInsertTableHandle(...)
+createVeloxPartitionFunctionSpec(...)
+createConnectorProtocol()
}
PrestoToVeloxConnector <|-- TpchPrestoToVeloxConnector
Class diagram for helper function relocationclassDiagram
class PrestoToVeloxConnectorUtils {
+toRequiredSubfields(...)
+toFileCompressionKind(...)
+toVeloxFileFormat(...)
+toJsonString(...)
+toFilter(...)
}
class HivePrestoToVeloxConnector {
+toVeloxSplit(...)
+toVeloxColumnHandle(...)
+toVeloxTableHandle(...)
+toVeloxInsertTableHandle(...)
+createVeloxPartitionFunctionSpec(...)
+createConnectorProtocol()
-toHiveColumns(...)
}
HivePrestoToVeloxConnector ..> PrestoToVeloxConnectorUtils : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
e1edc89 to
ea11d81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@aditi-pandit This is a followup PR based on your review comments. Can you help to have a look? Thanks. |
ea11d81 to
926a424
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @PingLiuPing. Code looks good minus nit.
presto-native-execution/presto_cpp/main/connectors/HivePrestoToVeloxConnector.h
Outdated
Show resolved
Hide resolved
926a424 to
db17fc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @PingLiuPing
| velox::connector::hive::HiveColumnHandle::ColumnType toHiveColumnType( | ||
| protocol::hive::ColumnType type); | ||
|
|
||
| std::unique_ptr<velox::connector::ConnectorTableHandle> toHiveTableHandle( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PingLiuPing We have a dependency on this function interanlly at Meta but moving it to annonymous namespace broke the internal build :(. Probably better to keep it in header, otherwise we need to copy paste the function. Since the function is in a header, its an open dependency. Probably we keep this in header so that downstream does not need to copy it.
…26420) ## Description We have separate `HivePrestoToVeloxConnector` to a standalone file in PR #26380. This PR makes some further refactor to let `IcebergPrestoToVeloxConnector` get rid of relying on `HivePrestoToVeloxConnector`. `toVeloxFileFormat(const presto::protocol::hive::StorageFormat& format)` is a Hive connector specific function, thus move it into `HivePrestoToVeloxConnector`; while `toHiveColumnType(protocol::hive::ColumnType type)` is a common function used by Hive and Iceberg, thus move it into `PrestoToVeloxConnectorUtils`. This eliminates the need for `IcebergPrestoToVeloxConnector` to rely on the `HivePrestoToVeloxConnector`. ## Motivation and Context - Decouple `IcebergPrestoToVeloxConnector` from `HivePrestoToVeloxConnector` ## Impact N/A ## Test Plan N/A ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. - [ ] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes ``` == NO RELEASE NOTE == ```
Description
This is a follow up PR of #26237 (comment)
This is a straightforward refactor.
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.