[refactor] Move HiveConnectorSplitBuilder to HiveConnectorSplit#484
[refactor] Move HiveConnectorSplitBuilder to HiveConnectorSplit#484ZacBlanco merged 1 commit intobytedance:mainfrom
Conversation
0b60cfd to
c710aba
Compare
|
@ZacBlanco Can you please review this PR? Thanks! |
There was a problem hiding this comment.
Change overall LGTM. One other thing though - I know the CI doesn't build all configurations now, but I think there might be some more tests which use the old HiveSplitBuilder in modules that we don't run in CI - for example, the GCS and S3 storage tests. Can you check for usages there to see if they also need to be updated?
You can build locally with something like
make <target> CONAN_OPTIONS=" -o bolt/*:enable_gcs=True -o bolt/*:enable_s3=True "
I'm going to add an item to my backlog to enable some more of these options in CI, but I'm not sure when I'll get around to it.
@ZacBlanco bolt/exec/tests/utils/HiveConnectorTestBase.cpp is using the moved HiveConnectorSplitBuilder by: Same for bolt/exec/tests/HashJoinTest.cpp. In the next PR I will remove the direct reference to any Hive objects in HiveConnectorTestBase.cpp, but I believe it's ok to do so in this PR. I verified bolt_exec_test builds fine and tests using it run fine too: The CI would fail if the tests weren't able to find HiveConnectorSplitBuilder. It's nice to have more coverage in the CI, but for the concern you had I think the CI already covers it. |
c710aba to
f9a09a7
Compare
Head branch was pushed to by a user without write access
bdb8e62 to
5e2a87e
Compare
In the upcoming refactor to decouple Hive from exec tests, we will need to make HiveConnectorTestBase connector agnostic. However, it contains HiveConnectorSplitBuilder, which should go back to the Hive connector in connectors/hive/HiveConnectorSplit.cpp. This commit makes this move and updates HiveObjectFactory to use it to create Hive splits.
5e2a87e to
c02d602
Compare
What problem does this PR solve?
Type of Change
Description
The HiveConnectorSplitBuilder should go back to the Hive connector
in connectors/hive/HiveConnectorSplit.cpp. This commit makes this move
and updates HiveObjectFactory to use it to create Hive splits.
Performance Impact
No Impact: This change does not affect the critical path (e.g., build system, doc, error handling).
Positive Impact: I have run benchmarks.
Click to view Benchmark Results
Negative Impact: Explained below (e.g., trade-off for correctness).
Release Note
Please describe the changes in this PR
Release Note:
N/A
Checklist (For Author)
Breaking Changes
No
Yes (Description: ...)
Click to view Breaking Changes