[Enhancement] Upgrade Apache Arrow thirdparty to 24.0.0#75868
Conversation
|
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b984a4c954
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6a5b057 to
1bc4f77
Compare
|
@codex review |
Bump the C++ thirdparty Arrow from 19.0.1 to 24.0.0. Several dependency adjustments are required for Arrow 24 to build against StarRocks thirdparty: - Arrow 24 requires xsimd >= 14.0.0; bump xsimd 13.2.0 -> 14.0.0. - Arrow links each dependency as shared by default; StarRocks builds thrift static-only (libthrift.a), so FindThriftAlt could not locate the library and fell back to bundled thrift (0.22.0), whose pinned checksum does not match StarRocks thrift 0.23.0. Add -DARROW_THRIFT_USE_SHARED=OFF (and use the correctly-cased -DThrift_SOURCE=SYSTEM) to use system thrift. - Arrow 24 pins zstd 1.5.7 to the official release tarball; StarRocks fed its GitHub archive tarball of the same version (different bytes), failing Arrow's bundled-zstd checksum. Switch ZSTD_DOWNLOAD to the release tarball. - Arrow 24 defaults to (and requires >=) C++20, and its public headers use std::span. Under C++20 GCC enables sized deallocation by default, which routes frees around StarRocks' allocator hooks (BE builds with -fno-sized-deallocation). Add -fno-sized-deallocation to the arrow build, and bump benchgen (an Arrow-header consumer) from C++17 to C++20 with the same flag. - Regenerate nix/thirdparty-archives.nix for the arrow/xsimd/zstd bumps. Verified: arrow 24.0.0 configures + compiles (libarrow.a, libparquet.a) and benchgen builds against it (thrift 0.23.0, xsimd 14.0.0, bundled zstd 1.5.7).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bc4f771b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Arrow 24 moved ArrayFromJSON out of the main library into libarrow_testing and removed arrow/ipc/json_simple.h. Adapt StarRocks: - build_arrow: enable ARROW_TESTING (depends only on ARROW_JSON, already on) with system GTest, so libarrow_testing.a is built and installed. - be/CMakeLists.txt: link arrow_testing (static; only the BE test binary that references ArrayFromJSON pulls it in, no production bloat). - column_to_arrow_converter_test.cpp: replace <arrow/ipc/json_simple.h> with <arrow/testing/gtest_util.h> and arrow::ipc::internal::json::ArrayFromJSON with arrow::ArrayFromJSON. Production BE compiles cleanly against Arrow 24 (verified to 57%+ incl. all column/arrow converters); this only touches a test + the thirdparty testing lib.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f8aea5a31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The Linux build-thirdparty.sh Arrow 24 adaptations were not applied to the macOS builder, but vars.sh (Arrow 24) and be/CMakeLists.txt (which now links arrow_testing for all platforms) are shared. Without this, macOS installs no libarrow_testing.a and BE/test fails to link with -larrow_testing not found; the Arrow build itself would also fail earlier on thrift/C++20. Mirror the fixes in build-thirdparty-darwin.sh: - arrow: -DARROW_TESTING=ON (system GTest) so libarrow_testing.a is installed. - arrow: -DARROW_THRIFT_USE_SHARED=OFF and correctly-cased -DThrift_SOURCE=SYSTEM. - arrow: -fno-sized-deallocation (Arrow 24 forces C++20). - benchgen: bump CMAKE_CXX_STANDARD 17 -> 20 and add -fno-sized-deallocation.
parquet::Int96::value changed from a C array (uint32_t[3]) to std::array<uint32_t, 3> in the Arrow 24 bundled Parquet headers, so pointer arithmetic and reinterpret_cast on it no longer compile. Use .data() to get the underlying pointer in level_builder's INT96 timestamp encoding path.
Arrow 24's arrow::ArrayFromJSON (arrow/testing/gtest_util.h) returns std::shared_ptr<Array> directly (it CHECKs internally), unlike the old arrow::ipc::internal::json::ArrayFromJSON which returned a Result. Drop the .ok()/.ValueUnsafe() Result handling and compare via s->Equals(array).
The -DCMAKE_CXX_FLAGS="-fno-sized-deallocation" override replaced the
inherited global CXXFLAGS, dropping -fPIC (and -O3). libbenchgen.a then failed
to link into StarRocks shared libraries (ConnectorBenchmark.so) on Linux with
R_X86_64_* relocation errors. Preserve ${CXXFLAGS} and force
CMAKE_POSITION_INDEPENDENT_CODE=ON. Mirror the flag-preserving change in the
Darwin builder.
…patches The Arrow patches were gated on ARROW_SOURCE=arrow-apache-arrow-19.0.1, so none applied after the 24.0.0 bump. This dropped the Hive optional-MAP-key tolerance, making Arrow 24 reject such files (Bad StatusOr access in ParquetScannerTest.optional_map_key: 'Map keys must be annotated as required'). - Add arrow-24.0.0-parquet-map-key.patch (rebased; comments out the strict key-is-required check in parquet/arrow/schema.cc) and apply it for arrow-apache-arrow-24.0.0 in download-thirdparty.sh. - Drop the other four arrow-19.0.1 patches, now obsolete on 24.0.0: use-zstd-1.5.7 (arrow 24 already pins 1.5.7), thrift (system thrift via -DThrift_SOURCE=SYSTEM), flight-types-clang and libtool-version-check (both fixed upstream in arrow 24).
[BE Incremental Coverage Report]✅ pass : 2 / 2 (100.00%) file detail
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Why I'm doing:
Bump the C++ thirdparty Arrow from 19.0.1 to 24.0.0. Two dependency adjustments are required for Arrow 24 to configure against StarRocks thirdparty:
Verified: arrow 24.0.0 cmake configure completes against the system deps (thrift 0.23.0, xsimd 14.0.0, bundled zstd 1.5.7).
What I'm doing:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: