refactor(hive): Move WriterOptions update to file-formats #11956

majetideepak · 2024-12-25T00:56:41Z

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

netlify · 2024-12-25T00:57:02Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`3a14e73`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6772f9c83371d40008e8f78c

JkSelf · 2024-12-26T06:02:37Z

velox/connectors/hive/HiveConfig.h

@@ -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 =


@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

@JkSelf I prefer to handle the ReaderOptions in a separate PR. The ReaderOptions are not specialized per file format similar to the WriterOptions

pedroerp

Wow, we were on-and-off talking about this refactor, thanks for taking a stab!

Overall it looks good to me, just a few minor nits. Please double check that we are not changing the default values of any these flags, or the precedence in which we apply these different sources of configs. I remember these looked quite messy last I looked.

Thanks for the cleanup!

pedroerp · 2024-12-27T23:04:28Z

velox/dwio/dwrf/common/Config.h

@@ -78,6 +78,48 @@ class Config : public config::ConfigBase {
  static Entry<uint64_t> RAW_DATA_SIZE_PER_BATCH;
  static Entry<bool> MAP_STATISTICS;

+  /// Maximum stripe size in orc writer.
+  static constexpr const char* kOrcWriterMaxStripeSize =


nit: in the future, we should probably make these consistent with the upper case ones above. But nice that we are taking small(er) steps.

pedroerp · 2024-12-27T23:05:51Z

velox/dwio/dwrf/test/ConfigTests.cpp

+  options.processConfigs(base, emptySession);
+  auto config = options.config;
+  ASSERT_EQ(getConfig(config, Config::STRIPE_SIZE), 64L * 1024L * 1024L);
+  ASSERT_EQ(


I suppose here we are copying the exact same tests from the hive connector? My only concern is that we don't change the default values (as these are currently all over the place), since this may have hard to predict effects. But I'm assuming all of these are preserved.

The API changed a bit since we moved from HiveConfig to Dwrf::Config and processConfigs. But the body and default values are the same.
There are 8 configs and I confirmed the defaults are the same as previous.

pedroerp · 2024-12-27T23:06:18Z

velox/dwio/dwrf/test/ConfigTests.cpp

@@ -71,6 +78,79 @@ struct ConfigTestParams {
  std::vector<uint32_t> expectedCols{}; // do we expect the spec to be valid
 };

+TEST(ConfigTests, WriterOptionsDefaultConfig) {


nit: test names are lower camel case

pedroerp · 2024-12-27T23:07:24Z

velox/dwio/dwrf/writer/Writer.h

@@ -44,6 +44,9 @@ struct WriterOptions : public dwio::common::WriterOptions {
      columnWriterFactory;
  const tz::TimeZone* sessionTimezone{nullptr};
  bool adjustTimestampToTimezone{false};
+  void processConfigs(


nit: newline to separate method declaration.

majetideepak · 2024-12-28T00:50:25Z

@pedroerp thanks for the review. I addressed your comments. I limited the refactoring to only that is required to minimize risk.

JkSelf

LGTM. Thanks for your great work.

facebook-github-bot · 2024-12-30T17:47:05Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng

@majetideepak thanks for the refactor % one comment. Thanks!

xiaoxmeng · 2024-12-30T17:51:53Z

velox/dwio/dwrf/common/Config.h

@@ -78,6 +78,48 @@ class Config : public config::ConfigBase {
  static Entry<uint64_t> RAW_DATA_SIZE_PER_BATCH;
  static Entry<bool> MAP_STATISTICS;

+  /// Maximum stripe size in orc writer.
+  static constexpr const char* kOrcWriterMaxStripeSize =
+      "hive.orc.writer.stripe-max-size";


Do we need hive prefix in the config? If it is hive specific, then shall we pass connector id to processConfig API (this might cause the dependency from file format to connector config) or does hive data sink should convert hive specific config to the corresponding format ones?

@xiaoxmeng We have the same dilemma for others like storage adapters (s3, gcs) config as well. eg. We use hive.s3. for S3.
We should discuss this and modify all the configs in a separate PR.

In a separate PR, I think we can remove the hive prefix for all and modify the config key in the HiveConnector before passing it to file-formats, storage-adapters.

xiaoxmeng · 2024-12-30T17:59:44Z

velox/connectors/hive/HiveConfig.h

@@ -136,48 +133,6 @@ class HiveConfig {
  /// meta data together. Optimization to decrease the small IO requests
  static constexpr const char* kFilePreloadThreshold = "file-preload-threshold";

-  /// Maximum stripe size in orc writer.


Can you update configs.rst and maybe have one section for each supported file format? Thanks!

Will do this.

majetideepak · 2024-12-30T19:52:35Z

@xiaoxmeng I updated the configs.rst. Thanks!

xiaoxmeng

@majetideepak thanks for the update!

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

majetideepak requested a review from JkSelf December 25, 2024 00:57

majetideepak changed the title ~~refactor(hive): Restore WriterOptions to File Formats~~ refactor(hive): Restore WriterOptions update to File Formats Dec 25, 2024

majetideepak changed the title ~~refactor(hive): Restore WriterOptions update to File Formats~~ refactor(hive): Move WriterOptions update to file-formats Dec 25, 2024

majetideepak marked this pull request as ready for review December 26, 2024 00:50

majetideepak requested a review from assignUser as a code owner December 26, 2024 00:50

JkSelf reviewed Dec 26, 2024

View reviewed changes

majetideepak force-pushed the fix-writeroptions branch from 85eaa5f to 269a98f Compare December 26, 2024 13:31

majetideepak requested review from kgpai and pedroerp December 27, 2024 18:40

majetideepak added 2 commits December 27, 2024 13:41

build: Cleanup Hive Connector depenencies

3c71207

refactor(hive): Restore WriterOptions to File Formats

bebc015

majetideepak force-pushed the fix-writeroptions branch from 269a98f to bebc015 Compare December 27, 2024 18:41

majetideepak mentioned this pull request Dec 27, 2024

fix(parquet): Add support for weak functions to register parquet readers and writers #11882

Open

pedroerp requested review from Yuhta and xiaoxmeng December 27, 2024 23:00

pedroerp approved these changes Dec 27, 2024

View reviewed changes

address review comments

e58dcdf

JkSelf approved these changes Dec 30, 2024

View reviewed changes

pedroerp added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Dec 30, 2024

xiaoxmeng reviewed Dec 30, 2024

View reviewed changes

Move File Format configuration

3a14e73

xiaoxmeng approved these changes Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(hive): Move WriterOptions update to file-formats #11956

refactor(hive): Move WriterOptions update to file-formats #11956

majetideepak commented Dec 25, 2024 •

edited

Loading

netlify bot commented Dec 25, 2024 •

edited

Loading

JkSelf Dec 26, 2024

majetideepak Dec 26, 2024

pedroerp left a comment

pedroerp Dec 27, 2024

pedroerp Dec 27, 2024

majetideepak Dec 28, 2024

pedroerp Dec 27, 2024

pedroerp Dec 27, 2024

majetideepak commented Dec 28, 2024

JkSelf left a comment

facebook-github-bot commented Dec 30, 2024

xiaoxmeng left a comment

xiaoxmeng Dec 30, 2024

majetideepak Dec 30, 2024

majetideepak Dec 30, 2024 •

edited

Loading

xiaoxmeng Dec 30, 2024

majetideepak Dec 30, 2024

majetideepak Dec 30, 2024

majetideepak commented Dec 30, 2024

xiaoxmeng left a comment

refactor(hive): Move WriterOptions update to file-formats #11956

Are you sure you want to change the base?

refactor(hive): Move WriterOptions update to file-formats #11956

Conversation

majetideepak commented Dec 25, 2024 • edited Loading

netlify bot commented Dec 25, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedroerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Dec 28, 2024

JkSelf left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 30, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Dec 30, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

majetideepak commented Dec 25, 2024 •

edited

Loading

netlify bot commented Dec 25, 2024 •

edited

Loading

majetideepak Dec 30, 2024 •

edited

Loading