-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(hive): Move WriterOptions update to file-formats #11956
base: main
Are you sure you want to change the base?
refactor(hive): Move WriterOptions update to file-formats #11956
Conversation
✅ Deploy Preview for meta-velox canceled.
|
@@ -44,9 +44,6 @@ class HiveConfig { | |||
|
|||
/// Maximum number of (bucketed) partitions per a single table writer | |||
/// instance. | |||
/// | |||
/// TODO: remove hive_orc_use_column_names since it doesn't exist in presto, | |||
/// right now this is only used for testing. | |||
static constexpr const char* kMaxPartitionsPerWriters = |
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.
@majetideepak How about moving the following file format-related configurations in HiveConfig into the file format's configuration file as well?
https://github.com/facebookincubator/velox/blob/main/velox/connectors/hive/HiveConfig.h#L76-L84
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.
@JkSelf I prefer to handle the ReaderOptions in a separate PR. The ReaderOptions are not specialized per file format similar to the WriterOptions
85eaa5f
to
269a98f
Compare
WriterOptions of the file format should implement how to process
format-specific session and connector configs.
This will decouple the Hive Connector library from the file-format libraries.
This was previously changed here #10915