Refactor HiveConfig #7725

kewang1024 · 2023-11-25T22:15:56Z

To solve issue: #7659

netlify · 2023-11-25T22:16:02Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`0da2fab`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65762512985f830008aac123

velox/common/file/FileSystems.h

velox/connectors/Connector.h

velox/connectors/fuzzer/FuzzerConnector.h

velox/connectors/hive/HiveConnector.h

velox/connectors/tpch/TpchConnector.h

velox/core/QueryCtx.h

velox/connectors/hive/HiveConfig.h

facebook-github-bot · 2023-12-09T00:21:35Z

This pull request was exported from Phabricator. Differential Revision: D51814038

Summary: To solve issue: #7659 Pull Request resolved: #7725 Reviewed By: xiaoxmeng Differential Revision: D51814038 Pulled By: kewang1024 fbshipit-source-id: 9fd8cef21fbb98e594bb0ff05eb7059816cc2b79

majetideepak · 2023-12-09T00:28:25Z

@majetideepak Can you elaborate?

@kewang1024 I meant HdfsFileSystem and AbfsFileSystem must also be modified similarly to GCS and S3 in this PR to be consistent.

hiveConfig_ = std::make_shared<HiveConfig>(
        std::make_shared<core::MemConfig>(config->values()));

facebook-github-bot · 2023-12-09T08:13:33Z

This pull request was exported from Phabricator. Differential Revision: D51814038

Summary: To solve issue: #7659 Pull Request resolved: #7725 Reviewed By: xiaoxmeng Differential Revision: D51814038 Pulled By: kewang1024 fbshipit-source-id: bc72ddeaea644b70705edd7bdb0e194c11e79429

facebook-github-bot · 2023-12-10T19:49:09Z

This pull request was exported from Phabricator. Differential Revision: D51814038

Summary: To solve issue: #7659 Pull Request resolved: #7725 Reviewed By: xiaoxmeng Differential Revision: D51814038 Pulled By: kewang1024 fbshipit-source-id: 30e3ae15ad41bd3162fb4610b0a5ad4c99c64dda

facebook-github-bot · 2023-12-10T20:17:53Z

This pull request was exported from Phabricator. Differential Revision: D51814038

Summary: To solve issue: #7659 Pull Request resolved: #7725 Reviewed By: xiaoxmeng Differential Revision: D51814038 Pulled By: kewang1024 fbshipit-source-id: 713733c762dfa549d77ec5cb90077b2672dd5219

Summary: To solve issue: #7659 Pull Request resolved: #7725 Reviewed By: xiaoxmeng Differential Revision: D51814038 Pulled By: kewang1024 fbshipit-source-id: 7ffd4a84738303aa5264676fa76dd87e752a30f5

facebook-github-bot · 2023-12-10T20:52:31Z

This pull request was exported from Phabricator. Differential Revision: D51814038

kewang1024 · 2023-12-11T02:20:04Z

@majetideepak Can you elaborate?

@kewang1024 I meant HdfsFileSystem and AbfsFileSystem must also be modified similarly to GCS and S3 in this PR to be consistent.
hiveConfig_ = std::make_shared<HiveConfig>(
        std::make_shared<core::MemConfig>(config->values()));

Sure, but looks like the way it's written, it won't break because of this change, but I can enhance those in the following PR

facebook-github-bot · 2023-12-11T02:48:19Z

@kewang1024 merged this pull request in eb75367.

conbench-facebook · 2023-12-11T03:22:52Z

Conbench analyzed the 1 benchmark run on commit eb75367c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

zhztheplayer · 2023-12-13T01:55:06Z

velox/connectors/hive/HiveConfig.h

  static constexpr const char* kFileColumnNamesReadAsLowerCase =
+      "file-column-names-read-as-lower-case";
+  static constexpr const char* kFileColumnNamesReadAsLowerCaseSession =
      "file_column_names_read_as_lower_case";


Hi @kewang1024

The change broke some tests in our project that relies on Velox and then took us some time to debug. Would you like to share the reason why having to rename config key kFileColumnNamesReadAsLowerCase ?

Hi, this is an effort to separate config from session property.
And fix the config name convention to be "-" and session property name convention "_", otherwise people are using them sometime "randomly" and it has caused a lot of bugs for us. Also you can refer to the issue: #7659

JkSelf · 2023-12-13T04:21:20Z

velox/connectors/hive/HiveConfig.cpp

-int32_t HiveConfig::maxCoalescedDistanceBytes(const Config* config) {
-  return config->get<int32_t>(kMaxCoalescedDistanceBytes, 512 << 10);
+bool HiveConfig::isFileColumnNamesReadAsLowerCase(const Config* session) const {
+  if (session->isValueExists(kFileColumnNamesReadAsLowerCaseSession)) {


@kewang1024 @majetideepak @xiaoxmeng @mbasmanova

Apart from the issue mentioned by @zhztheplayer here. There is one another issue here. We have currently set kFileColumnNamesReadAsLowerCase to true in Gluten, but it didn't take effect. We must set kFileColumnNamesReadAsLowerCaseSession to true in order for it to work. Based on the code analysis, it seems that if the kFileColumnNamesReadAsLowerCaseSession parameter is not set, it will read the kFileColumnNamesReadAsLowerCase configuration. However, it appears that it is not taking effect.

@JkSelf Would you create GitHub issue to explain the problem you are facing? We can discuss it there.

kFileColumnNamesReadAsLowerCase should be set when a HiveConnector is created, kFileColumnNamesReadAsLowerCaseSession should be set per query basis in the session property.

can you show me where your code is not working?

@kewang1024 The related code is here. We set kFileColumnNamesReadAsLowerCase config to true without this PR. And after this PR, we need to set kFileColumnNamesReadAsLowerCaseSession to true and then the unit test can pass in Gluten. It seems the kFileColumnNamesReadAsLowerCase is not longer taking effect in our case.

Yohahaha · 2023-12-13T09:17:10Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

+  Impl(const Config* config) {
+    hiveConfig_ = std::make_shared<HiveConfig>(
+        std::make_shared<core::MemConfig>(config->values()));


config is nullptr here.

FileSystem was created by FileHandleGenerator, but FileHandleGenerator's properties was set to nullptr in HiveConnector ctor.

Summary: A recent refactor #7725 missed passing the connector config to the FileHandleGenerator in the HiveConnector. Pull Request resolved: #8038 Reviewed By: xiaoxmeng Differential Revision: D52154732 Pulled By: mbasmanova fbshipit-source-id: df6c0edcc5fb169877f9795f6a17eec2c52c1dd2

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 Nov 25, 2023

kewang1024 force-pushed the config-test branch 4 times, most recently from 1b6fc88 to ff5a163 Compare November 27, 2023 18:52

kewang1024 changed the title ~~Config test~~ Refactor HiveConfig Nov 27, 2023

kewang1024 force-pushed the config-test branch 15 times, most recently from 08aa508 to 8f9fd41 Compare November 28, 2023 08:30

kewang1024 marked this pull request as ready for review November 28, 2023 08:31

kewang1024 requested review from xiaoxmeng and majetideepak November 28, 2023 19:13

kewang1024 force-pushed the config-test branch 4 times, most recently from 6dfe58d to a0e6ffd Compare November 28, 2023 22:01

xiaoxmeng reviewed Nov 29, 2023

View reviewed changes

kewang1024 force-pushed the config-test branch from 4f9141b to 9226f1d Compare December 9, 2023 00:21

kewang1024 force-pushed the config-test branch from 9226f1d to 555cc7d Compare December 9, 2023 08:13

kewang1024 force-pushed the config-test branch from 555cc7d to be4ee21 Compare December 10, 2023 19:49

kewang1024 force-pushed the config-test branch from be4ee21 to 9776e47 Compare December 10, 2023 20:17

Refactor HiveConfig (#7725)

0da2fab

Summary: To solve issue: #7659 Pull Request resolved: #7725 Reviewed By: xiaoxmeng Differential Revision: D51814038 Pulled By: kewang1024 fbshipit-source-id: 7ffd4a84738303aa5264676fa76dd87e752a30f5

kewang1024 force-pushed the config-test branch from 9776e47 to 0da2fab Compare December 10, 2023 20:52

facebook-github-bot closed this in eb75367 Dec 11, 2023

facebook-github-bot added the Merged label Dec 11, 2023

zhztheplayer reviewed Dec 13, 2023

View reviewed changes

JkSelf reviewed Dec 13, 2023

View reviewed changes

Yohahaha reviewed Dec 13, 2023

View reviewed changes

Yohahaha mentioned this pull request Dec 13, 2023

Fix adapters issue caused by null config in FileHandleGenerator #8014

Closed

majetideepak mentioned this pull request Dec 14, 2023

Fix FileHandleGenerator inside HiveConnector #8038

Closed

kewang1024 deleted the config-test branch December 15, 2023 17:23

JkSelf mentioned this pull request Jan 23, 2024

Make the kPartitionPathAsLowerCaseSession parameter from session #8468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor HiveConfig #7725

Refactor HiveConfig #7725

kewang1024 commented Nov 25, 2023 •

edited

Loading

netlify bot commented Nov 25, 2023 •

edited

Loading

facebook-github-bot commented Dec 9, 2023

majetideepak commented Dec 9, 2023 •

edited

Loading

facebook-github-bot commented Dec 9, 2023

facebook-github-bot commented Dec 10, 2023

facebook-github-bot commented Dec 10, 2023

facebook-github-bot commented Dec 10, 2023

kewang1024 commented Dec 11, 2023 •

edited

Loading

facebook-github-bot commented Dec 11, 2023

conbench-facebook bot commented Dec 11, 2023

zhztheplayer Dec 13, 2023

kewang1024 Dec 13, 2023

JkSelf Dec 13, 2023

mbasmanova Dec 13, 2023

kewang1024 Dec 13, 2023

JkSelf Dec 18, 2023

Yohahaha Dec 13, 2023

Refactor HiveConfig #7725

Refactor HiveConfig #7725

Conversation

kewang1024 commented Nov 25, 2023 • edited Loading

netlify bot commented Nov 25, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Dec 9, 2023

majetideepak commented Dec 9, 2023 • edited Loading

facebook-github-bot commented Dec 9, 2023

facebook-github-bot commented Dec 10, 2023

facebook-github-bot commented Dec 10, 2023

facebook-github-bot commented Dec 10, 2023

kewang1024 commented Dec 11, 2023 • edited Loading

facebook-github-bot commented Dec 11, 2023

conbench-facebook bot commented Dec 11, 2023

zhztheplayer Dec 13, 2023

Choose a reason for hiding this comment

kewang1024 Dec 13, 2023

Choose a reason for hiding this comment

JkSelf Dec 13, 2023

Choose a reason for hiding this comment

mbasmanova Dec 13, 2023

Choose a reason for hiding this comment

kewang1024 Dec 13, 2023

Choose a reason for hiding this comment

JkSelf Dec 18, 2023

Choose a reason for hiding this comment

Yohahaha Dec 13, 2023

Choose a reason for hiding this comment

kewang1024 commented Nov 25, 2023 •

edited

Loading

netlify bot commented Nov 25, 2023 •

edited

Loading

majetideepak commented Dec 9, 2023 •

edited

Loading

kewang1024 commented Dec 11, 2023 •

edited

Loading