Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Dec 25, 2024

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 25, 2024
Copy link

netlify bot commented Dec 25, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 269a98f
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/676d5ad18391330008d707d4

@majetideepak majetideepak requested a review from JkSelf December 25, 2024 00:57
@majetideepak majetideepak changed the title refactor(hive): Restore WriterOptions to File Formats refactor(hive): Restore WriterOptions update to File Formats Dec 25, 2024
@majetideepak majetideepak changed the title refactor(hive): Restore WriterOptions update to File Formats refactor(hive): Move WriterOptions update to file-formats Dec 25, 2024
@majetideepak majetideepak marked this pull request as ready for review December 26, 2024 00:50
@@ -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 =
Copy link
Collaborator

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

Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants