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

Move fileformat's config setting into HiveConnectorUtils #10915

Closed
wants to merge 1 commit into from

Conversation

JkSelf
Copy link
Collaborator

@JkSelf JkSelf commented Sep 3, 2024

Refactor the config setting logic from fileformat's writer to HiveConnectorUtils.

@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 Sep 3, 2024
Copy link

netlify bot commented Sep 3, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 3e10282
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66d905b5aab86f0008dafb66

@JkSelf
Copy link
Collaborator Author

JkSelf commented Sep 3, 2024

@xiaoxmeng Can you help to review this PR? Thanks.

@JkSelf JkSelf force-pushed the writer-config-refactor branch 2 times, most recently from 6121072 to 868d253 Compare September 3, 2024 05:37
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JkSelf looks good % minors. Thanks for the refactoring!

velox/connectors/hive/HiveDataSink.cpp Outdated Show resolved Hide resolved
velox/connectors/hive/HiveConnectorUtil.h Outdated Show resolved Hide resolved
velox/connectors/hive/HiveConfig.h Outdated Show resolved Hide resolved
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JkSelf Thanks for the update!

velox/connectors/hive/HiveConnectorUtil.h Outdated Show resolved Hide resolved
velox/connectors/hive/HiveConfig.cpp Outdated Show resolved Hide resolved
velox/connectors/hive/HiveConfig.cpp Outdated Show resolved Hide resolved
velox/connectors/hive/HiveConnectorUtil.cpp Outdated Show resolved Hide resolved
velox/connectors/hive/tests/HiveConnectorUtilTest.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JkSelf LGTM. Thanks!

@facebook-github-bot
Copy link
Contributor

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

namespace {

#ifdef VELOX_ENABLE_PARQUET
std::optional<TimestampUnit> getTimestampUnit(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getTimestampUnit and getTimestampTimeZone is not parquet specific

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yuhta Thanks for your catch. It should be VELOX_ENABLE_ARROW. I have updated. Can you help to review again? Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yuhta It seems we still need to use VELOX_ENABLE_PARQUET here. Otherwise the build will failed with following exceptions:

error: ‘getTimestampUnit’ was not declared in this scope; did you mean ‘TimestampUnit’?

@JkSelf JkSelf force-pushed the writer-config-refactor branch 2 times, most recently from a1a9d01 to 3e10282 Compare September 5, 2024 01:13
@facebook-github-bot
Copy link
Contributor

@xiaoxmeng merged this pull request in 1900d60.

Copy link

Conbench analyzed the 1 benchmark run on commit 1900d602.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Yuhta added a commit to Yuhta/velox that referenced this pull request Sep 6, 2024
Summary:
Recent refactor facebookincubator#10915
breaks the capability to write to Nimble table in Hive connector, fix it.

Differential Revision: D62302602
Yuhta added a commit to Yuhta/velox that referenced this pull request Sep 6, 2024
Summary:
Pull Request resolved: facebookincubator#10941

Recent refactor facebookincubator#10915
breaks the capability to write to Nimble table in Hive connector, fix it.

Reviewed By: xiaoxmeng

Differential Revision: D62302602
Yuhta added a commit to Yuhta/velox that referenced this pull request Sep 6, 2024
Summary:
Pull Request resolved: facebookincubator#10941

Recent refactor facebookincubator#10915
breaks the capability to write to Nimble table in Hive connector, fix it.

Reviewed By: xiaoxmeng

Differential Revision: D62302602
Yuhta added a commit to Yuhta/velox that referenced this pull request Sep 9, 2024
Summary:
Pull Request resolved: facebookincubator#10941

Recent refactor facebookincubator#10915
breaks the capability to write to Nimble table in Hive connector, fix it.

Reviewed By: xiaoxmeng

Differential Revision: D62302602
Yuhta added a commit to Yuhta/velox that referenced this pull request Sep 9, 2024
Summary:
Pull Request resolved: facebookincubator#10941

Recent refactor facebookincubator#10915
breaks the capability to write to Nimble table in Hive connector, fix it.

Reviewed By: xiaoxmeng

Differential Revision: D62302602
facebook-github-bot pushed a commit that referenced this pull request Sep 10, 2024
Summary:
Pull Request resolved: #10941

Recent refactor #10915
breaks the capability to write to Nimble table in Hive connector, fix it.

Reviewed By: xiaoxmeng

Differential Revision: D62302602

fbshipit-source-id: b7685d9cbedb8533a9e479a5c7033c717db2030a
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. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants