-
Couldn't load subscription status.
- Fork 5.5k
refactor(native): Decouple Iceberg from HivePrestoToVeloxConnector #26420
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(native): Decouple Iceberg from HivePrestoToVeloxConnector #26420
Conversation
Reviewer's GuideRefactored function organization to decouple IcebergPrestoToVeloxConnector from HivePrestoToVeloxConnector by moving Hive-specific storage format conversions into the Hive connector, extracting common column type mapping into shared utilities, and updating includes accordingly. Class diagram for refactored connector dependenciesclassDiagram
class HivePrestoToVeloxConnector {
+toVeloxFileFormat(format: StorageFormat): FileFormat
}
class IcebergPrestoToVeloxConnector {
// No longer depends on HivePrestoToVeloxConnector
}
class PrestoToVeloxConnectorUtils {
+toHiveColumnType(type: ColumnType): HiveColumnHandle::ColumnType
}
HivePrestoToVeloxConnector --|> PrestoToVeloxConnector
PrestoToVeloxConnectorUtils <.. IcebergPrestoToVeloxConnector : uses
PrestoToVeloxConnectorUtils <.. HivePrestoToVeloxConnector : uses
Class diagram for function relocation and decouplingclassDiagram
class HivePrestoToVeloxConnector {
+toVeloxFileFormat(format: StorageFormat): FileFormat
}
class PrestoToVeloxConnectorUtils {
+toHiveColumnType(type: ColumnType): HiveColumnHandle::ColumnType
}
HivePrestoToVeloxConnector -- PrestoToVeloxConnectorUtils : uses toHiveColumnType
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
|
Hi @aditi-pandit @PingLiuPing, would you like to take a look at this? I think it might be better to further decouple Iceberg from |
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.
Thank you @hantangwangd
a108796 to
0b7a5ec
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 @hantangwangd for the cleanup.
|
|
||
| velox::dwio::common::FileFormat toVeloxFileFormat( | ||
| const presto::protocol::hive::StorageFormat& format); | ||
| velox::connector::hive::HiveColumnHandle::ColumnType toHiveColumnType( |
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.
I would've liked to keep this file independent of all the Hive specific conversions. But seems like there are multiple of them in this file (toHiveTableHandle) etc . We could move them to their own separate file as well. What do you think of adding a HivePrestoToVeloxConnectorUtils as well ?
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.
I would've liked to keep this file independent of all the Hive specific conversions. But seems like there are multiple of them in this file (toHiveTableHandle) etc . We could move them to their own separate file as well.
Sounds great to me! Besides, for method toJsonString, since it is also used by PrestoToVeloxQueryPlan.cpp, perhaps it's worth considering moving it to the util class under package common.
Description
We have separate
HivePrestoToVeloxConnectorto a standalone file in PR #26380. This PR makes some further refactor to letIcebergPrestoToVeloxConnectorget rid of relying onHivePrestoToVeloxConnector.toVeloxFileFormat(const presto::protocol::hive::StorageFormat& format)is a Hive connector specific function, thus move it intoHivePrestoToVeloxConnector; whiletoHiveColumnType(protocol::hive::ColumnType type)is a common function used by Hive and Iceberg, thus move it intoPrestoToVeloxConnectorUtils. This eliminates the need forIcebergPrestoToVeloxConnectorto rely on theHivePrestoToVeloxConnector.Motivation and Context
IcebergPrestoToVeloxConnectorfromHivePrestoToVeloxConnectorImpact
N/A
Test Plan
N/A
Contributor checklist
Release Notes