mirror of
https://github.com/apache/arrow.git
synced 2026-10-28 16:47:19 +08:00
main
7629
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Alina (Xi) Li
|
9eb748dde4 |
GH-55275: [C++][FlightRPC][ODBC] Unset CPACK_WIX_PRODUCT_GUID to allow Windows installer upgrade (#55276)
### Rationale for this change GH-55275 https://cmake.org/cmake/help/v4.5/cpack_gen/wix.html#variable:CPACK_WIX_PRODUCT_GUID > CPACK_WIX_PRODUCT_GUID > Product GUID (`Product/@ Id`) > > Will be automatically generated unless explicitly provided. > > If explicitly provided this will set the Product Id of your installer. > > The installer **will abort** if it detects a pre-existing installation that uses the **same GUID**. Highlights my own. ### What changes are included in this PR? Removed hard-coded `CPACK_WIX_PRODUCT_GUID`. Then CPack will generate the UUID value for Product ID during each build, and it should unblock users from upgrading using ODBC installer. ### Are these changes tested? Regression tested in CI. Tested installer on local Windows machine and was able to upgrade. ### Are there any user-facing changes? no ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed * GitHub Issue: #55275 Authored-by: Alina (Xi) Li <96995091+alinaliBQ@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com> |
||
|
Nic Crane
|
2ecb29d980 |
GH-55267: [C++][R] Arrow 26.0.0 fails to build on CRAN macOS builders: bundled simdjson C++20 concepts don't compile with the macOS 11 SDK (#55273)
### Rationale for this change CRAN failures on 26.0.0 release due to regex not catching incompatible MacOSX11.sdk version ### What changes are included in this PR? Make regex more permissive ### Are these changes tested? No ### Are there any user-facing changes? No **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed * GitHub Issue: #55267 Authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Nic Crane <thisisnic@gmail.com> |
||
|
|
9a18992678 |
GH-47774: [C++] Force-inline Status::~Status() to improve reading performance (#50749)
### Rationale for this change `Status` is returned by nearly every API on hot paths (e.g. IPC/Parquet reading), so its destructor runs extremely frequently. The destructor body is already optimized for the common success case: it only checks `state_ != NULL` (with `ARROW_PREDICT_FALSE`) and returns immediately for an OK status, delegating the rare error cleanup to the out-of-line `DeleteState()`. However, without an explicit inlining hint, some compilers/optimization levels may not inline this destructor, turning the trivial `state_ == NULL` fast path into a real function call at every call site. Forcing it inline removes this call overhead on hot read paths and improves read throughput. In our perf test (reads a parquet file of 6.7GB), `Status::~Status` cost about 20 seconds, as the following flamegraph shows: <img width="1381" height="254" alt="image" src="https://github.com/user-attachments/assets/c62601b8-6d1f-455a-9ee5-9edc4ea57852" /> After inlining `Status::~Status`, the hotspot disappeared. End-to-end latency was also reduced by 20 seconds, as expected. <img width="1367" height="263" alt="image" src="https://github.com/user-attachments/assets/78ec8f84-f9b7-4d74-b767-9046c1d40eaf" /> ### What changes are included in this PR? - Annotate `Status::~Status()` in `cpp/src/arrow/status.h` with `ARROW_FORCE_INLINE` (`__attribute__((always_inline)) inline` on GCC/Clang, `__forceinline` on MSVC), so the cheap null-check fast path is always inlined while the cold error path still goes through the non-inlined `DeleteState()`. ### Are these changes tested? Yes, by existing tests. This change does not alter any behavior of `Status`; the existing `Status` unit tests and the full C++ test suite cover it. The performance improvement can be observed with the existing read-path benchmarks. ### Are there any user-facing changes? No. This is an internal performance optimization with no API or behavior change. * GitHub Issue: #47774 Lead-authored-by: Antoine Pitrou <pitrou@free.fr> Co-authored-by: zhouhongfeng <zhouhongfeng.zhf@alibaba-inc.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Raúl Cumplido
|
4ecdfe046e |
GH-54902: [C++] Move GCS and google-cloud-cpp to its own libarrow_gcs.so (#54905)
### Rationale for this change Trying to reduce the size of libarrow.so and remove google-cloud-cpp SDK on some builds. Allow for users to plug and play based on requirements and divide our functionality into cleaner modules. ### What changes are included in this PR? Unconditionally build GCS and the google-cloud-cpp SDK into a different module libarrow_gcs.so outside of libarrow.so. Update bindings to link against the new libarrow_gcs.so library. Update the Linux Package jobs to have the new module into a different package. ### Are these changes tested? Yes via CI ### Are there any user-facing changes? Yes, users will need to either link against libarrow_s3.so or register using LoadFileSystemFactories ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #54902 Authored-by: Raúl Cumplido <raulcd@apache.org> Signed-off-by: Raúl Cumplido <raulcd@apache.org> |
||
|
Zehua Zou
|
5f75a12e29 |
GH-55124: [C++][Parquet] Allow masked nulls in required fields when writing (#55136)
### Rationale for this change The Parquet writer checks the leaf array's physical null count without accounting for the parent struct's validity bitmap. As a result, it rejects nulls in required child fields even when they are masked by null parent rows. ### What changes are included in this PR? Corrected the logic used by the column writer to validate the null values for required leaf fields. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #55124 Authored-by: Zehua Zou <zehuazou2000@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Alexander Taepper
|
f9eee02fd1 |
GH-54924: [C++] HasVariadicBuffers should respect extension types (#54925)
### Rationale for this change This fixes #54924. An extension array would not round-trip correctly through ArraySpans as a test demonstrates. ### What changes are included in this PR? This fixes the `HasVariadicBuffers` implementation to correctly handle extension types. A test demonstrating the fix is also included. The fix follows the implementation of `ArraySpan::SetMembers`, where the same additional case for extension types is included ### Are these changes tested? Yes. By the existing test suite plus an additional test. ### Are there any user-facing changes? None other than the bug fix. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [X] Human - [ ] AI **Reviewed before submission by:** - [X] Human - [X] AI - [ ] Not reviewed An LLM suggested the second fix for ``` if (type_id == Type::STRING_VIEW || type_id == Type::BINARY_VIEW) { ``` in `ArraySpan::SetMembers`, which does look correct to me. * GitHub Issue: #54924 Authored-by: Alexander Taepper <alexander.taepper@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Advit Arora
|
2cadeab93e |
GH-47417: [C++] Fix JSON parser losing nulls in a null-typed list (#51108)
### Rationale for this change
`{"a": [null, 1]}` silently parses to `[[1]]` in release builds, and aborts in debug.
This was introduced in `b7054c21aab` (0.14.0).
### What changes are included in this PR?
`AppendNull` looks up its child from the parent and returns the updated `BuilderPtr`, instead of taking a copy from the caller. `Null()` writes it back to `builder_`, so `builder_` no longer drifts from the parent's copy.
### Are these changes tested?
Two new tests, `BlockParser.NullsInList` (inferred schema) and `BlockParserWithSchema.NullsInList` (explicit schema), parse lists of nulls, including `{"a": [{"b": null}, null]}`. Both fail in debug without the fix.
### Are there any user-facing changes?
`{"a": [null, 1]}` now parses to `[[null, 1]]`.
**This PR contains a "Critical Fix".** Well-formed JSON silently loses list elements.
* GitHub Issue: #47417
### Was AI used for this PR?
In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR.
**PR code and description written by:**
- [x] Human
- [ ] AI
**Reviewed before submission by:**
- [x] Human
- [ ] AI
- [ ] Not reviewed
Authored-by: Advit Arora <advitarora2@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
|
||
|
Sutou Kouhei
|
b0492db231 |
GH-54638: [C++][FlightRPC] Upgrade bundled gRPC for OpenSSL 4 support (#54927)
### Rationale for this change gRPC 1.76.0 doesn't compile with OpenSSL 4. gRPC 1.85.0 includes OpenSSL 4 support: https://github.com/grpc/grpc/pull/41932 ### What changes are included in this PR? gRPC 1.85.0 requires a newer Abseil. gRPC 1.76.0 doesn't compile with the newer Abseil. So we need to upgrade both of them together. We use Abseil 20260817.0 not 20260526.0 that is used by gRPC 1.85.0 because Abseil 20260526.0 can't be built with MSVC with `/arch:AVX2`: https://github.com/abseil/abseil-cpp/issues/2116 gRPC 1.85.0 moved its .proto files to the third_party/grpc-proto submodule. Source archives don't include submodules and we disable gRPC_DOWNLOAD_ARCHIVES. So we download grpc-proto by ourselves. Abseil 20260817.0 removed some libraries and added some new libraries. So we update the list of bundled Abseil libraries. Abseil 20260817.0 uses `O_NONBLOCK` that isn't available on MinGW. So we define `O_NONBLOCK` as 0 for MinGW like the upstream fix: https://github.com/abseil/abseil-cpp/commit/ddbdebc6c8 Abseil 20260817.0 includes `<cstdint>` by itself. So we remove the `-include stdint.h` workaround for GCC 13 or later. gRPC headers don't use deprecated `std::iterator`. So we remove the `-Wno-error=deprecated-declarations` workaround. We didn't bundle upb static libraries used by gRPC. So static linking with `libarrow_bundled_dependencies.a` failed with undefined upb symbols. gRPC 1.85.0 also requires the new `upb_descriptor_lib`. So we bundle all upb static libraries. Other bundled dependencies also need to be upgraded for the new Abseil: Protobuf 31.1 can't be built with Abseil 20260817.0 with MSVC. So we upgrade bundled Protobuf to 36.2. Protobuf 36.2 headers include `utf8_validity.h` in utf8_range. So we add the utf8_range include directory for ORC built by ExternalProject. google-cloud-cpp headers use deprecated Abseil APIs such as `absl::visit()`. They cause compilation errors with `-Werror` with Abseil 20260817.0. They are fixed in google-cloud-cpp 3.9.0 or later but google-cloud-cpp 3.9.0 or later requires libcurl 8.7.1 or later. We can't require it because some supported platforms such as Ubuntu 24.04 ship older libcurl. So we use `-Wno-error=deprecated-declarations` for bundled google-cloud-cpp instead of upgrading it. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed * GitHub Issue: #54638 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Raúl Cumplido <raulcd@apache.org> |
||
|
Pearu Peterson
|
d228ba5a9d |
GH-54647: [C++] Reject leap seconds in the strptime timestamp parser (#54659)
### Rationale for this change `strptime` accepts a seconds value of 60 (glibc also 61), and the parser added it to the time of day, which wrapped it into the next minute. Arrow cannot represent leap seconds, and the ISO-8601 parser rejects them. ### What changes are included in this PR? The parser rejects a seconds value of 60 or more. The check applies only when the time of day is used, so Gandiva's `to_date`, which ignores the time, still accepts such values. ### Are these changes tested? Yes, in `TimestampParser.StrptimeParser`, and a Gandiva `to_date` test checks that a leap second still gives the date when the time is ignored. ### Are there any user-facing changes? Leap seconds no longer parse with `strptime` formats, except in Gandiva's `to_date`. ### AI usage disclosure Developed with AI assistance (Claude Code): the fix, the tests and this description were AI-generated under my direction and reviewed line by line by me. I own and can debug these changes. No copyrighted material is included. * GitHub Issue: #54647 Authored-by: Pearu Peterson <pearu.peterson@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Logan Riggs
|
2db03408aa |
GH-51663: [C++][Gandiva] Race condition with Gandiva expressions (#51665)
### Rationale for this change I originally identified [this issue in arrow-java](https://github.com/apache/arrow-java/issues/601) and fixed it on the arrow-java side with a coarse grained lock on the Projector and Filter Make methods. That fixed the issue but adds some undesirable overhead in certain multi threaded workflows. This issue captures the deeper problem that is occurring in the C++ side of Gandiva and was not touched by my previous fix though it is prevented from happening. I have a proposed solution for this and believe it is a better overall solution. `Projector::Make()` and `Filter::Make()` read the shared expression cache once to decide the `is_cached` status and then call `SetLLVMObjectCache()`. That performs its own second, unsynchronized read of the same key before pre-loading a cached object into the LLJIT. Those two reads could disagree. If another thread compiling the identical (schema, expressions, selection vector mode, configuration) tuple inserted between them, the first thread would: 1. take the `is_cached == false` path, generating `expr_0_0` into its IR module, and 2. also see a hit on the second read and `addObjectFile()` a cached object that defines `expr_0_0` as well. Both then call `JITDylib::define` for the same symbol in the same `JITDylib`, and ORC's duplicate-symbol detection fires: ```text CodeGenError in Gandiva: Failed to add IR module to LLJIT: In gdv_module_..., duplicate definition of symbol 'expr_0_0' ``` ### What changes are included in this PR? Thread the single cache-lookup result (`prev_cached_obj`) that already determined `is_cached` directly into `SetLLVMObjectCache()`, instead of letting it perform an independent second lookup. `Engine::SetLLVMObjectCache()` and `LLVMGenerator::SetLLVMObjectCache()` now take the resolved `std::shared_ptr<llvm::MemoryBuffer>` rather than a `GandivaObjectCache&`. `Engine::SetLLVMObjectCache()` passes a copy of the buffer to `addObjectFile()`, as the old `GandivaObjectCache::getObject()` did, because the shared cache still owns the original buffer. No new locking — this removes the window rather than serializing around it. ### Are these changes tested? This adds a new deterministic regression test, `cpp/src/gandiva/tests/cache_race_test.cc`, which is part of `gandiva-projector-test`. The test doesn't depend on thread timing. It registers an `ExportedFuncsBase` hook that inserts an entry for the key being built into the shared cache during `Engine::Init()`. That is after `Make()`'s first cache lookup and before the (pre-fix) second lookup in `SetLLVMObjectCache()`, so the race window is reproduced on every run. It covers both `Projector::Make()` and `Filter::Make()`, and checks that the built projector/filter evaluates correctly. Verified the tests fail with the duplicate-symbol error without this fix. Verified internally with our product stress tests after removing the arrow-java side of the fix. Once this change is in C++ I can make the java change to remove the synchronization. ### Are there any user-facing changes? **This PR includes breaking changes to public APIs.** Engine::SetLLVMObjectCache and LLVMGenerator::SetLLVMObjectCache are GANDIVA_EXPORT so this changes an exported signature. In-tree there are no other callers, and the Gandiva Java side JNI only uses Projector::Make/Filter::Make, which are unchanged. ### Was AI used for this PR? AI was used to diagnose the problem, write code and tests over several iterations before being reviewed by two human developers. A human ran several manual and regression tests against the change. One human reviewed and heavily edited the PR description and also wrote some himself. **PR code and description written by:** - [X] Human - [X] AI **Reviewed before submission by:** - [X] Human - [X] AI - [ ] Not reviewed * GitHub Issue: #51663 Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Sutou Kouhei
|
e53d3f202b |
GH-51690: [C++] Don't define __SSE2__ globally with MSVC (#53952)
### Rationale for this change `simdjson >= 5` uses GCC builtins such as `__builtin_ctz` when `__SSE2__` is defined. So we can't build Apache Arrow C++ with `simdjson >= 5` and MSVC because we define `__SSE2__` globally. ### What changes are included in this PR? We don't need to define `__SSE2__` because xsimd detects SSE2 by `_M_X64` with MSVC. We still need to define `__SSE4_1__`/`__SSE4_2__` for xsimd because MSVC doesn't define them even with `/arch:SSE4.2`. But we define them only where SSE4.2 is enabled. So they are no longer defined with `ARROW_SIMD_LEVEL=NONE`. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #51690 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Pearu Peterson
|
b9479757dd |
GH-54646: [C++] Reject years out of range in the strptime timestamp parser (#54658)
### Rationale for this change The parser computed the year as `tm_year + 1900`, which can overflow `int` (e.g. with `%s`), and passed it to `date::year`, which holds 16 bits, so a year out of range gave a wrong timestamp. ### What changes are included in this PR? The year is computed in 64 bits and rejected if `date::year` cannot hold it. ### Are these changes tested? Yes, `TimestampParser.StrptimeYearOutOfRange` covers a year that wraps around and one where `tm_year + 1900` overflows `int`. It needs `%s` and is skipped where `strptime` does not parse `%s` into 64 bits (e.g. on Windows). ### Are there any user-facing changes? Values whose year does not fit in 16 bits no longer parse with `strptime` formats. ### AI usage disclosure Developed with AI assistance (Claude Code): the fix, the test and this description were AI-generated under my direction and reviewed line by line by me. I own and can debug these changes. No copyrighted material is included. * GitHub Issue: #54646 Authored-by: Pearu Peterson <pearu.peterson@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
|
373d64c02c |
GH-50282: [C++][FlightRPC] Refactor gRPC server and transport classes (#50407)
# Rationale for this change
The goal of the pullrequest is to refactor the FligtRPC code to prepare the async implementation.
# What changes are included in this PR
- extracts `server.cc/transport_server.cc` internals into `transport_server_internal.{h,cc}` and the gRPC-specific call context, auth/middleware helper, and endpoint/builder setup into `transport/grpc/grpc_server_internal.{h,cc}`, templated on the gRPC context type so `ServerContext` and `CallbackServerContext` can share one implementation.
- Moves signal-driven shutdown into `internal::ServerSignalState`, fixing teardown ordering so handlers are restored even when `Serve()` fails early, a SIGPIPE race when waking the signal thread, and shutdown of an uninitialized server clearing another serving instance's signal target. (Github Copilot issue report)
- Adds public `Location::uri()` and `Location::port()`, and includes` arrow/util/uri.h` from types.h so the `Result<util::Uri>` return type is instantiable by consumers of the installed header. (Github Copilot issue report in the PR)
- `Wait()` on an uninitialized server now returns `Status::Invalid` instead of dereferencing null. No other public behavior change. `ServerTransport`, `TransportMessageReader`/`Writer`, `WriteDataStream` and friends stay internal. (Github Copilot issue report in the PR)
- Drops the unused gRPC FlightData message registry.
# Are these changes tested?
- Full CI tests
- Adds tests covering signal handling and server middleware headers that depend on handler state.
# Are there any user-facing changes?
- New API: `Location::uri()` → `Result<util::Uri>`, `Location::port()` → `int` (-1 when absent)
- Compilation fix: `types.h` now includes `arrow/util/uri.h`. Before, `Result<util::Uri>` was uninstantiable for anyone including only the installed header.
- Behavior: `Wait()` on an uninitialized server returns `Status::Invalid` (was a null deref). `Serve()` restores prior signal handlers on failure. `Shutdown()` on an uninitialized server no longer disables signal shutdown for another serving instance, signal thread wake no longer risks SIGPIPE-killing the process. (Github Copilot issue report in the PR)
# Was AI used for this PR?
In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR.
PR code and description written by:
- [x] Human
- [x] AI
Reviewed before submission by:
- [x] Human
- [x] AI
- [] Not reviewed
* GitHub Issue: #50282
Lead-authored-by: Alexis Placet <alexis.placet@quantstack.net>
Co-authored-by: Alexis Placet <alexis.placet.dev@pm.me>
Co-authored-by: Alexis Placet <2400067+Alex-PLACET@users.noreply.github.com>
Signed-off-by: Raúl Cumplido <raulcd@apache.org>
|
||
|
Pearu Peterson
|
6e4fa50fb0 |
GH-54643: [C++] Reject impossible dates and a zero day in the strptime timestamp parser (#54657)
### Rationale for this change `strptime` checks the day of month against 31 at most, and some implementations (macOS, the vendored musl used on Windows) accept day `00`, and the vendored musl also month `00`. The parser rolled an impossible date such as February 30 over into the next month, took a zero day as the first of the month, and a zero month as December of the previous year. ### What changes are included in this PR? The parser rejects the parsed date if it is not valid, as the ISO-8601 parser does. To tell a zero day from an omitted one, the day is initialized to the first before parsing, instead of being raised to the first afterwards. The two fixes are in one PR because the zero-day fix relies on the date check. The vendored musl `strptime` also gets back the unsigned range check that upstream musl has, so it rejects day and month `00` itself. ### Are these changes tested? Yes, in `TimestampParser.StrptimeParser` (impossible dates, a zero day, a zero month, and February 29 of a leap year) and `TimestampParser.StrptimeWithoutDay` (a format without a day directive still gives the first of the month). The zero day and zero month cases only fail without the fix on platforms whose `strptime` accepts them. ### Are there any user-facing changes? Impossible dates, a zero day and a zero month no longer parse with `strptime` formats. ### AI usage disclosure Developed with AI assistance (Claude Code): the fix, the tests and this description were AI-generated under my direction and reviewed line by line by me. I own and can debug these changes. No copyrighted material is included. * GitHub Issue: #54643 Authored-by: Pearu Peterson <pearu.peterson@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Vikrant Puppala
|
a4b74e560c |
GH-54645: [C++][FlightRPC][ODBC] Fix descriptor test query (#54695)
### Rationale for this change
The Dremio-backed ODBC test suite fails in StatementTest/1.TestChangingAppRowDescriptorRebindsColumns because Dremio treats VALUE as a reserved keyword. The same query succeeds against the SQLite mock, so the failure was only exposed by the remote suite.
### What changes are included in this PR?
Rename the test query alias from value to val, which is accepted by both SQLite and Dremio.
### Are these changes tested?
Yes. Ran the Ubuntu 24.04 CI-equivalent ODBC job locally:
UBUNTU=24.04 docker compose run --rm ubuntu-cpp-odbc
All 61 C++ test executables passed, including the Dremio-backed arrow-flight-sql-odbc-test.
Maintainer note: please apply the CI: Extra: C++ label so the ODBC CI job runs.
### Are there any user-facing changes?
No. This is a test-only compatibility fix.
### Was AI used for this PR?
In accordance to the AI generation guidelines, AI was used to investigate the failure, prepare the one-line test fix, run validation, and draft this description.
**PR code and description written by:**
- [ ] Human
- [x] AI
**Reviewed before submission by:**
- [x] Human
- [x] AI
- [ ] Not reviewed
Closes #54645.
* GitHub Issue: #54645
Authored-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
|
||
|
Sutou Kouhei
|
14daca685e |
GH-49818: [C++][FlightRPC] Don't call Finish() twice on handshake stream (#54685)
### Rationale for this change `arrow-flight-sql-odbc-test` sometimes aborts with `GRPC_CALL_ERROR_TOO_MANY_OPERATIONS` in `ClientReaderWriter::Finish()` of the handshake stream. This happens when a server finishes a handshake before the client writes or reads. For example, `NoOpAuthHandler` on the server side and Dremio do this. In this case: 1. `GrpcClientAuthSender::Write()` (or `GrpcClientAuthReader::Read()`) fails and calls `Finish()` on the handshake stream. 2. The returned status is OK, so the client auth handler and `AuthenticateInternal()` continue. 3. `AuthenticateInternal()` calls `WritesDone()`, drains the stream and calls `Finish()` again. 4. gRPC rejects the second `RECV_STATUS_ON_CLIENT` with `GRPC_CALL_ERROR_TOO_MANY_OPERATIONS` and aborts the process. `GRPC_TRACE=api` shows this sequence: ```text SEND_MESSAGE -> ERROR: Attempt to send message after stream was closed. RECV_INITIAL_METADATA + RECV_STATUS_ON_CLIENT -> OK (Finish() by GrpcClientAuthSender::Write()) SEND_CLOSE_FROM_CLIENT (WritesDone()) RECV_MESSAGE (drain) RECV_STATUS_ON_CLIENT -> GRPC_CALL_ERROR_TOO_MANY_OPERATIONS (Finish() by AuthenticateInternal()) ``` Whether the server finishes before the client writes is a race. So it's flaky. ### What changes are included in this PR? * Add `GrpcClientHandshakeStream` that wraps the handshake stream: * `Finish()` is called at most once. The second and later calls return the status of the first call. * `Read()`, `Write()` and `WritesDone()` return `false` without calling gRPC after the stream is finished. * Return the status in `AuthenticateInternal()` without `WritesDone()` and draining when the stream is already finished. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. Flight clients don't abort the process when a server finishes a handshake early. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed * GitHub Issue: #49818 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Dricmoy Bhattacharjee
|
1776578067 |
GH-51351: [C++][Parquet] Use default KMS connection values (#53996)
### Rationale for this change `FileKeyWrapper` calls `SetDefaultIfEmpty()` on its stored configuration, but passes the original, unmodified configuration to `GetKmsClient`. This allows the KMS client factory to receive empty instance ID and URL values even though Arrow has already filled in defaults on its internal copy. ### What changes are included in this PR? - Pass the defaulted `kms_connection_config_` member to `GetKmsClient`. - Update C++ and Python key-rotation tests to expect Arrow's existing default values for empty fields. - Separate the writer and reader crypto factories in the dataset encryption tests. The dataset tests give the writer all keys and the reader only some. With a shared factory, the reader can reuse the writer's cached client, which already has all the keys. Separate factories ensure the reader uses its own keys, so the missing-key checks work correctly. The write-path fix exposes this because writing and reading now use the same default instance ID, where empty and `"DEFAULT"` IDs previously selected separate cache entries. ### Are these changes tested? - Local C++ encryption key-management suite: 24 tests passed. - Both local Python encryption test files: 62 passed, 1 skipped. The skipped case requires a build without encryption support. - C++ and Python formatting and lint, and `git diff --check`, passed. - Fork CI for `537e296384`: [Python](https://github.com/Dricmoy/arrow/actions/runs/38923806919), [Dev](https://github.com/Dricmoy/arrow/actions/runs/38923806888), and [Docs](https://github.com/Dricmoy/arrow/actions/runs/38923806885) passed. The [C++ workflow](https://github.com/Dricmoy/arrow/actions/runs/38852310915) passed for the preceding commit; the follow-up commit changes only Python tests. ### Are there any user-facing changes? KMS client factories receive Arrow's `"DEFAULT"` markers instead of empty strings for omitted instance ID and URL values. Explicit non-empty values are preserved. These markers are interpreted by the KMS implementation; they are not automatically resolved server addresses. I reviewed the compatibility tradeoff: custom factories that special-case empty strings may need updating. Defaulting the instance ID can also change client reuse because the cache is indexed by access token and instance ID. Requests with the same token and resolved ID can now share a client where empty and `"DEFAULT"` IDs previously selected separate entries. This makes client creation consistent with Arrow's existing defaulting logic and stored key metadata, but changes behavior for integrations that rely on empty strings. There are no API signature changes. Closes #51351. Follow-up to #53949. ### Was AI used for this PR? AI assisted with the implementation, tests, validation, and this description. I reviewed the configuration flow, the default-value test expectations, the reader/writer cache separation, and the compatibility tradeoff described above. The PR overall is fairly small consistent expectation change, had some tests changed to fit the updated expectations. **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed * GitHub Issue: #51351 Authored-by: Dricmoy <dricbhattacharjee@gmail.com> Signed-off-by: Adam Reeve <adreeve@gmail.com> |
||
|
Pearu Peterson
|
a907fbc646 |
GH-54641: [C++] Support locale directives in the vendored strptime on Windows (#54656)
### Rationale for this change On Windows, the vendored musl `strptime` does not handle `%a`, `%A`, `%b`, `%B`, `%h`, `%c`, `%p`, `%r`, `%x` and `%X`, because they need `nl_langinfo()`, which is not available there. ### What changes are included in this PR? A C-locale replacement for `nl_langinfo()`, restricted to the items the vendored `strptime` uses, with the values of the POSIX locale. ### Are these changes tested? Yes, `TimestampParser.StrptimeLocaleDirectives` covers each of these directives. On Windows it exercises the vendored `strptime`; elsewhere it runs against the system `strptime`. ### Are there any user-facing changes? On Windows, `strptime` formats with these directives now parse. ### AI usage disclosure Developed with AI assistance (Claude Code): the fix, the test and this description were AI-generated under my direction and reviewed line by line by me. I own and can debug these changes. No copyrighted material is included. * GitHub Issue: #54641 Authored-by: Pearu Peterson <pearu.peterson@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Aaditya Srinivasan
|
fb767a7cae |
GH-51008: [C++] Remove vendored whereami (#54024)
### Rationale for this change Remove the unused vendored whereami library and its associated license text from LICENSE.txt. ### What changes are included in this PR? - Remove the vendored whereami source and header files. - Remove the associated license text from LICENSE.txt. ### Are there any user-facing changes? No ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [X] Human - [ ] AI **Reviewed before submission by:** - [X] Human - [ ] AI - [ ] Not reviewed Closes: #51008 * GitHub Issue: #51008 Authored-by: Aaditya Srinivasan <aadityasri03@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Alexander
|
e4405dcd52 |
GH-53730: [C++] Simplify destructor of ArrowLog (#53911)
### Rationale for this change @ elfring correctly identified in https://github.com/apache/arrow/issues/53730 that we perform an unnecessary null check before invoking `delete`. Calling `delete` on a `nullptr` is completely safe. ### What changes are included in this PR? This simplifies the destructor of `ArrowLog`. Additionally: - explicitly setting the member to `nullptr` in the destructor is unnecessary - my static analyzer also noticed that we can replace the `reinterpret_cast` with a safer `static_cast` ### Are these changes tested? Yes ### Are there any user-facing changes? No ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [X] Human - [ ] AI **Reviewed before submission by:** - [X] Human - [X] AI - [ ] Not reviewed * GitHub Issue: #53730 Authored-by: Alexander Taepper <alexander.taepper@gmail.com> Signed-off-by: Zehua Zou <zehuazou2000@gmail.com> |
||
|
Alina (Xi) Li
|
50c2fc2656 |
GH-53818: [C++][FlightRPC][ODBC] Fix Windows ODBC Installer Registering Empty Driver and Setup Paths (#53819)
### Rationale for this change https://github.com/apache/arrow/issues/53818 ### What changes are included in this PR? - Resolve bug where Windows ODBC installer registers empty `Driver` and `Setup` values. Both values should point to the actual installed driver path. - Add CI verification for Windows ODBC registration values ### Are these changes tested? - Yes, tested in CI ### Are there any user-facing changes? Yes. This fixes a packaging bug of the Windows ODBC installer. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed * GitHub Issue: #53818 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Evan Brown
|
740237e342 |
[C++] Remove capacity() check in TDigest::TDigestImpl::Validate() (#52276)
### Rationale for this change `std::vector::reserve(delta_)` only guarantees `capacity() >= delta_`, not `capacity() == delta_`. When the C++ standard library and allocator use size-returning allocations (such as C++23 `allocate_at_least`), `reserve(delta_)` can immediately set `capacity() > delta_`, causing `TDigestImpl::Validate()` to fail with `"oversized tdigest buffer"` even when the number of centroids is at most `delta_`. Fixes issue #53777. ### What changes are included in this PR? Remove the check of `capacity() > delta_` in `TDigest::TDigestImpl::Validate()`. ### Are these changes tested? Yes, covered by existing `TDigestTest` unit tests in `cpp/src/arrow/util/tdigest_test.cc`. ### Are there any user-facing changes? No. ### Was AI used for this PR? **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed |
||
|
Vikrant Puppala
|
402a71cb4f |
GH-51341: [C++][FlightRPC][ODBC] Fix statement descriptor and cursor lifecycle (#51342)
### Rationale for this change The Flight SQL ODBC statement implementation had a cluster of related descriptor and cursor lifecycle defects: - the implicit application row descriptor (ARD) was initialized from the application parameter descriptor (APD), aliasing two descriptor roles; - `SQL_NULL_HANDLE` did not restore built-in application descriptors; - freeing a statement left dangling registrations in explicit descriptors; - `SQLFetch` and `SQLGetData` without an open cursor could dereference a null result or report the wrong diagnostic; and - `SQLMoreResults` did not close the current cursor when returning `SQL_NO_DATA`. ### What changes are included in this PR? - Initialize the active ARD from `built_in_ard_`. - Map null APD/ARD assignments to the corresponding built-in descriptor and detach the outgoing explicit descriptor. - Detach explicit descriptors before releasing a statement. - Return SQLSTATE `24000` from `SQLFetch` and `SQLGetData` when no cursor is open. - Close the current cursor when `SQLMoreResults` returns `SQL_NO_DATA`. ### Are there any user-facing changes? Yes. Spec-valid descriptor release and reset sequences no longer retain dangling state, invalid cursor operations return SQLSTATE `24000`, and `SQLMoreResults` closes the current cursor as required. ### How was this patch tested? - Added focused mock/remote typed ODBC regression tests. Closes #51341. * GitHub Issue: #51341 Authored-by: Vikrant Puppala <vikrant.puppala@databricks.com> Signed-off-by: David Li <li.davidm96@gmail.com> |
||
|
Abdul Rawoof Khan
|
d26582eee4 |
GH-50544: [C++][Gandiva] Fix out-of-bounds read in round with most negative precision (#50545)
### Rationale for this change `round_int32_int32` and `round_int64_int32` negate the caller-supplied precision into `abs_precision` before range-checking it, so the check `abs_precision > 9` (and `> 18` for the int64 overload) never fires for `precision == INT32_MIN`, where the negation leaves the value negative. That value reaches `get_power_of_10`, which indexes a 19-element static table behind `DCHECK_GE`/`DCHECK_LE` only. The DCHECKs compile out in release builds, so `round(int_col, -2147483648)` reads far outside the table and either faults or returns arbitrary memory as the rounding multiplier. Both overloads are registered in `function_registry_arithmetic.cc`, so the precision comes straight from user SQL. `truncate_int64_int32` in the same file already compares the signed scale directly (`out_scale < -38`) and is unaffected; the two `round` overloads are the only sites that negate first. ### What changes are included in this PR? Check the bound against the signed `precision` before negating, at both sites. Every precision that was in range before still takes the same path, and the negation now only runs on values known to be within 9 (or 18) of zero. ### Are these changes tested? Yes. Added two cases to `TestExtendedMathOps.TestRound` covering `std::numeric_limits<int32_t>::min()` for both overloads, matching the existing `truncate_int64_int32` case in the same file. On the unpatched tree the debug build aborts with `extended_math_ops.cc:372: Check failed: (exp) >= (0)`; with the fix the full `gandiva-precompiled-test` suite passes (134 tests). ### Are there any user-facing changes? No. **This PR contains a "Critical Fix".** It fixes an out-of-bounds read reachable from a user-supplied `round` precision argument. * GitHub Issue: #50544 Authored-by: abdul rawoof <abdulr@bugqore.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Hanayoshi-8744
|
292674e64a |
GH-53931: [C++] Update bundled Apache Thrift to 0.25.0 (#53932)
### Rationale for this change Thrift 0.25.0 was released shortly after #51559 (bump to 0.24.0) was opened, so bump the bundled version again to keep up with the latest release for 27.0.0. ### What changes are included in this PR? - Bump the bundled Apache Thrift from 0.24.0 to 0.25.0 in `cpp/thirdparty/versions.txt`. The SHA256 matches the one published at downloads.apache.org. ### Are these changes tested? - Compared the 0.24.0 and 0.25.0 sources of the Thrift headers Arrow uses (`TCompactProtocol`, `TMemoryBuffer` in `TBufferTransports.h`, `TApplicationException`). The differences are const-correctness, explicit casts, and opt-in additions mainly for `TFramedTransport`; I found no change in the default behavior of `TMemoryBuffer`. - Built Arrow C++ locally on macOS (arm64, AppleClang) with `-DARROW_PARQUET=ON -DARROW_BUILD_TESTS=ON -DThrift_SOURCE=BUNDLED` on top of the current `main`, and ran `parquet-internals-test`, `parquet-file-deserialize-test`, `parquet-schema-test`, `parquet-reader-test`, `parquet-writer-test`, `parquet-arrow-reader-writer-test`, `parquet-arrow-metadata-test`, `parquet-arrow-index-test`, and `parquet-arrow-internals-test`. All passed (some tests were skipped, e.g. those requiring Snappy, which was not enabled in my build). - Not tested locally: Linux, Windows, and the R package builds; these rely on CI. ### Are there any user-facing changes? No. * GitHub Issue: #53931 --- Disclosure: this change was prepared with the assistance of an AI coding tool (Claude Code). I reviewed the diff and ran the tests locally before opening this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Authored-by: Hanayoshi-8744 <201203709+Hanayoshi-8744@users.noreply.github.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Vikrant PuppalaandIsaac
|
53416d1d71 |
GH-50852: [C++][FlightRPC][ODBC] Decode wide connection/descriptor string attributes with the wide decoder (#50853)
### Rationale for this change
Two string attributes in the ODBC driver are decoded with the byte-wise
decoder even when the value arrives through a wide (Unicode / `*W`) entry point,
so the wide buffer is misread and corrupted.
`ODBCConnection::SetConnectAttr(SQL_ATTR_CURRENT_CATALOG)` had its two decode
branches swapped:
```cpp
if (is_unicode) {
SetAttributeUTF8(value, string_length, catalog); // byte-wise
} else {
SetAttributeSQLWCHAR(value, string_length, catalog); // wide
}
```
`is_unicode` selects the buffer **width**: a unicode (`*W`) call passes a wide
`SQLWCHAR` buffer that must be decoded with `SetAttributeSQLWCHAR`; a non-unicode
call passes a byte string decoded with `SetAttributeUTF8`. With the branches
swapped, a wide catalog name (e.g. UTF-16 `"my_catalog"`) is reinterpreted
byte-wise and stored with the wide encoding's embedded NULs
(`"m\0y\0_\0c\0a\0t\0a\0l\0o\0g\0"`), so catalog scoping operates on a corrupt
name. This is the inverse of the mapping the getter already uses:
`GetStringAttribute` maps `is_unicode -> GetAttributeSQLWCHAR`.
`ODBCDescriptor::SetField(SQL_DESC_NAME)` is the same class of bug: it
unconditionally used the byte-wise `SetAttributeUTF8`, even though the matching
getter (`GetField` / `SQL_DESC_NAME`) reads the field back with
`GetAttributeSQLWCHAR`.
**How this happened (history):**
- The `SQL_ATTR_CURRENT_CATALOG` branches were swapped from the start, in the
original driver import (GH-46522, #40939). Note the getter side was written
against the shared `GetStringAttribute` helper (which decides width in one
place and got it right), while the setter open-coded the branch inline and
inverted the polarity — there is no matching `SetStringAttribute` helper.
- The descriptor `SQL_DESC_NAME` case was originally *consistent* (getter and
setter both byte-wise). GH-47721 (#48050) later migrated the descriptor
string getters to the wide `GetAttributeSQLWCHAR` for correct Unicode column
attributes, but left the `SQL_DESC_NAME` setter on the byte-wise decoder,
creating the asymmetry.
### What changes are included in this PR?
- `odbc_connection.cc`: swap the `SQL_ATTR_CURRENT_CATALOG` decode branches so a
unicode call uses `SetAttributeSQLWCHAR` and a non-unicode call uses
`SetAttributeUTF8`, matching `GetStringAttribute`.
- `odbc_descriptor.cc`: decode `SQL_DESC_NAME` with `SetAttributeSQLWCHAR` to
match its getter.
- `connection_attr_test.cc`: add `TestSQLSetGetConnectAttrCurrentCatalogWide`, a
`TYPED_TEST` (mock + remote fixtures) that sets a multi-character catalog
through the wide entry point and asserts the full name round-trips back.
- `odbc_descriptor_test.cc` (new): add `ODBCDescriptorTest.SetGetNameWideRoundTrips`,
a `SetField`/`GetField` round-trip on `SQL_DESC_NAME` with a multi-character
wide name. There is no `SQLSetDescField` entry point in this tree, so this
covers the descriptor change with a direct unit test on `ODBCDescriptor`.
### Are these changes tested?
Yes. Each change has a round-trip test that distinguishes the fixed and unfixed
code:
| check | result |
|---|---|
| catalog test, **with** fix (mock fixture) | PASS — `out_catalog == "my_catalog"` |
| catalog test, **without** fix (mock fixture) | FAIL — `out_catalog == "m\0y\0_\0c\0a\0t\0a\0l\0o\0g\0"` |
| descriptor test, **with** fix | PASS — name reads back as `"my_column"` |
| descriptor test, **without** fix | FAIL — byte-wise decode corrupts the stored UTF-16 |
| `arrow-odbc-spi-impl-test` (unit suite) | 85/85 pass |
| `ConnectionAttributeTest/0.*` (mock suite) | 25/25 pass |
| clang-format | clean on all changed files |
The connection catalog test uses the mock fixture (`FlightSQLODBCMockTestBase`,
an in-process SQLite Flight SQL server); its remote variant runs when
`ARROW_FLIGHT_SQL_ODBC_CONN` is set. The descriptor test is a standalone unit
test. Verified locally against the mock fixture and the unit suite.
### Are there any user-facing changes?
Yes. A catalog name (and descriptor `SQL_DESC_NAME`) set through the wide entry
point is now decoded correctly instead of being corrupted. Applications that set
a multi-character catalog through `SQLSetConnectAttrW` now scope to the intended
catalog.
### AI usage
Per the [AI-generated code guidance](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code):
the diagnosis, the fix, and the test were produced with the assistance of an AI
coding agent and reviewed and verified by me. Correctness was checked by
(1) confirming the getter path (`GetStringAttribute`) maps `is_unicode` to the
wide decoder, establishing the intended mapping the setters violated, and
(2) running the new round-trip test against both the fixed and unfixed driver to
confirm it distinguishes them (clean name vs. embedded-NUL corruption).
---
_🤖 Drafted by Claude Code (an AI agent) and reviewed & verified by vikrantpuppala._
* GitHub Issue: #50852
Lead-authored-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Co-authored-by: Isaac
Signed-off-by: David Li <li.davidm96@gmail.com>
|
||
|
Eric Wang
|
ca5564e007 |
GH-50828: [C++][FlightRPC][ODBC] Add SQLGetInfo remote tests (#50838)
### Rationale for this change The SQLGetInfo tests for `SQL_CREATE_SCHEMA` and `SQL_CREATE_TABLE` only covered the mock server. The remote fixture can report different DDL capabilities, so the missing remote coverage allowed regressions to go unnoticed. ### What changes are included in this PR? - Add a named remote connection-info fixture. - Add remote tests for `SQL_CREATE_SCHEMA` and `SQL_CREATE_TABLE`. - Keep the mock expectations (`1`) and assert the remote fixture's unsupported DDL values (`0`) separately. Fixes #50828. ### Are these changes tested? Yes ### Are there any user-facing changes? No. * GitHub Issue: #50828 Authored-by: erichanwang <ericwang559@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com> |
||
|
Gang Wu
|
910292405b |
GH-53615: [C++][Parquet] Add ALP to file-level fuzz corpus (#53671)
### Rationale for this change Follow up on #48345 by including ALP in the generated Parquet file-level fuzz corpus. ### What changes are included in this PR? Add `Encoding::ALP` to `kFloatEncodings`. ### Are these changes tested? N/A. ### Are there any user-facing changes? No. This only changes fuzzing seed generation, not production reader or writer behavior. ### Was AI used for this PR? AI prepared and reviewed the one-line change and the PR description. **PR code and description written by:** - [ ] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed Closes #53615. * GitHub Issue: #53615 Authored-by: Gang Wu <ustcwg@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Alexander Taepper
|
28d9769101 |
GH-51834: [C++] Reduce ARROW_ASSIGN_OR_RAISE stack consumption with gcc/clang (#53314)
### Rationale for this change @ pitrou found in [investigations](https://github.com/apache/arrow/issues/51834) that repeated uses of `ARROW_ASSIGN_OR_RAISE` can lead to very large stack consumptions. ### What changes are included in this PR? This PR adds a new definition of the `ARROW_ASSIGN_OR_RAISE` macro, which avoids intermediate results using a gcc/clang compiler extension. ### Are these changes tested? No new feature, all tests still pass. ### Are there any user-facing changes? Yes. The `ARROW_ASSIGN_OR_RAISE` macro is a public interface and now has a breaking change: If `lhs` is a declaration, it will already be in scope for the body of the right-hand-side. This lead to one breakage in the arrow code-base: ``` ARROW_ASSIGN_OR_RAISE(auto options, GetReadOptions(*reader->schema(), *self, *options)); ``` which needed manual fixing. This could also break users of the C++ library ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [X] Human - [ ] AI **Reviewed before submission by:** - [X] Human - [X] AI - [ ] Not reviewed * GitHub Issue: #51834 Authored-by: Alexander Taepper <alexander.taepper@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Divjot Arora
|
7ee15283bc |
GH-53518: [Parquet] Reject unsupported compression codecs (#53522)
### Rationale for this change See https://github.com/apache/arrow/issues/53518 for rationale. Treating unknown compression codecs as `UNCOMPRESSED` is wrong and at best causes the read to fail late during decoding and at worst yields incorrect results if the compressed stream is actually a valid decodable set of bytes (very unlikely). ### What changes are included in this PR? This PR causes unsupported compression codecs to fail a read eagerly during Thrift parsing. ### Are these changes tested? Yes, new test case added to `metadata_test.cc` ### Are there any user-facing changes? No ### Was AI used for this PR? **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed Closes #53518 * GitHub Issue: #53518 Authored-by: Divjot Arora <div.arora@databricks.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Raúl Cumplido
|
e304e13b68 | MINOR: [Release] Update versions for 27.0.0-SNAPSHOT | ||
|
water
|
c4a0850ebf |
GH-51018: [C++] Fix indices_nonzero segfault on chunkless ChunkedArray (#51021)
### Rationale for this change `pyarrow.compute.indices_nonzero()` segfaults (SIGSEGV) on a `ChunkedArray` with zero chunks. A zero-length `Array` and a `ChunkedArray` holding one empty chunk both work; only the chunkless case crashes. ### Are these changes tested? Yes, by new test case. ### Are there any user-facing changes? Only a bugfix. * GitHub Issue: #51018 Authored-by: waterWang <waterWang@users.noreply.github.com> Signed-off-by: Rok Mihevc <rok@mihevc.org> |
||
|
|
875cd2ed9b |
GH-48701: [C++][Parquet] Add ALPpd encoding (#48345)
Co-authored-by: Dhirhan Kanesalingam <[dhirhan17@ gmail.com](mailto:dhirhan17@ gmail.com)> With help from : @ emkornfield, @ wgtmac ### Rationale for this change [Adaptive Lossless Floating-Point (ALP)](https://docs.google.com/document/d/1PlyUSfqCqPVwNt8XA-CfRqsbc0NKRG0Kk1FigEm3JOg/edit?tab=t.0#heading=h.5xf60mx6q7xk) is designed for floating-point data that commonly represents decimal values. For these workloads, ALP can provide better compression and faster decoding than general-purpose compression or existing Parquet encodings. This PR adds ALP support for Parquet `FLOAT` and `DOUBLE` columns in the Arrow C++ implementation. ### Specification - [Original ALP specification](https://docs.google.com/document/d/1xz2cudDpN2Y1ImFcTXh15s-3fPtD_aWt/edit) - [Parquet `AlpEncoding.md`](https://github.com/apache/parquet-format/blob/main/AlpEncoding.md) (merged through apache/parquet-format#539; currently in Preview) ### What changes are included in this PR? This PR adds: - The core ALP compression and decompression implementation. - Sampling logic for selecting encoding parameters. - Page metadata serialization, validation, and decoding. - Parquet encoders and decoders for `FLOAT` and `DOUBLE`. - Incremental decoding of ALP pages one vector at a time. - CMake and Meson build integration. - C++ documentation describing how to use the encoding. ALP is opt-in on the write path. It is used only when the writer explicitly selects `Encoding::ALP` and dictionary encoding is disabled. Arrow does not currently select ALP automatically based on the input data. ### Are these changes tested? Yes. Test coverage includes: - Interoperability with the ALP conformance file in `parquet-testing`. - `FLOAT` and `DOUBLE` round trips. - Decimal-like, constant, random, and exception-heavy inputs. - Empty and all-null pages. - Configurable vector sizes. - Truncated or malformed page metadata. - Invalid offsets, element counts, bit widths, and exception positions. - Batched and incremental decoding. ### Are there any user-facing changes? Yes. Arrow C++ can read Parquet pages encoded with ALP. Writers can explicitly select ALP for supported floating-point columns. ALP requires reader support and is not selected automatically, so existing writer behavior remains unchanged unless users opt in. * GitHub Issue: #48701 Lead-authored-by: Prateek Gaur <prateek.gaur@snowflake.com> Co-authored-by: Gang Wu <ustcwg@gmail.com> Signed-off-by: Gang Wu <ustcwg@gmail.com> |
||
|
Krisztián Szűcs
|
9b34c4a6a8 |
GH-51684: [C++][Parquet] Keep the CDC chunking state across row groups (#51685)
### Rationale for this change The content-defined chunker was owned by the column writer, which is recreated for every row group, so the chunking state was reset at each row group boundary. See #51684. ### What changes are included in this PR? The file writer creates one chunker per leaf column and passes it to the column writers of every row group through a private `ColumnWriter::Make()` overload. ### Are these changes tested? Yes. `IndependentOfRowGroupBoundaries` writes the same table into one and into six row groups and expects the same page boundaries apart from the row group ends. The first commit adds it alone, failing without the fix. ### Are there any user-facing changes? - Files with multiple row groups get different page boundaries, since the chunking continues across row groups. - `ColumnWriter::Make()` is unchanged, but column writers created through it no longer chunk, so content-defined chunking requires `ParquetFileWriter`. - Multi-threaded writes with content-defined chunking are about 1.7× slower until #51692 (9.9 ms vs 5.8 ms writing 12 int32 columns of 1M rows on an M4 Max) ### Deduplication results A 1 GiB file of 128Mi random int64 values (PLAIN, snappy, default CDC options) compared with a copy that has 10 rows inserted at 20%. The ratio is the share of bytes left after deduplicating both files with the Xet chunker; 50% means the edited copy adds almost nothing. | Row groups | pyarrow 25.0.1 | This PR | arrow-rs 58.4 | |---|---|---|---| | 8 × 16Mi rows | 50.2% | 50.1% | 50.1% | | 32 × 4Mi rows | 50.6% | 50.4% | 50.5% | | 128 × 1Mi rows | 52.4% | 51.6% | 51.6% | | 512 × 256Ki rows | 59.8% | 56.4% | 56.5% | | 2048 × 64Ki rows | 84.8% | 74.2% | 74.2% | | Without CDC | 86.3% | 86.3% | 86.3% | The page boundaries now match arrow-rs, which already keeps the chunker state across row groups. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. Claude Code wrote the code, tests and description under human direction. **PR code and description written by:** - [ ] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed 🤖 Generated with [Claude Code](https://claude.com/claude-code) * GitHub Issue: #51684 |
||
|
Robert Kruszewski
|
84472e46c9 |
GH-52161: [C++][Parquet] Add support for string/binary view to content defined chunking writer (#52162)
### Rationale for this change View types are supported in parquet writer but they error when using content defined chunking ### What changes are included in this PR? Teach the internal chunker how to calculate the size for view types ### Are these changes tested? Added tests ### Are there any user-facing changes? No ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [ ] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed * GitHub Issue: #52161 Authored-by: Robert Kruszewski <github@robertk.io> Signed-off-by: Zehua Zou <zehuazou2000@gmail.com> |
||
|
FadyYosry77
|
5ef3c52003 |
GH-51231: [C++][Acero] Keep input nullability information for direct field projections (#51691)
### Rationale for this change ProjectNode creates output column definitions with the requested output name and the type of the bound expression for that column. Nullability information is available from the input schema but is not copied during output column creation; thus, output columns are nullable by default. As a result, projecting a nonnullable column from the input schema produces a nullable column in the output schema, even when only that column is projected. An identity projection may therefore produce a schema incompatible with its input. GH-51231 restricts this change to top-level input fields. ### What changes does this PR contain? Nullability is now exposed through `Expression::nullable()`. Binding captures the input field's nullability for direct top-level field references, and ProjectNode consumes that accessor to determine the nullability of the output field. The captured nullability is recomputed when rebinding and preserved when converting named references to field paths. The projected field name and type of the bound expression remain unchanged. Calls, literals, nested field references, and other expressions whose nullability has not been inferred remain conservatively nullable. This pull request does not include full function-aware nullability inference. A regression test covers: - direct nonnullable-nullable field correspondence - reordered projections - renaming a required field - a calculated control with nonnullable input and nullable output as before - already-bound indexed field projections Expression-level regression tests cover direct fields, the conservative fallback, rebinding, and nullability preservation when converting named references to field paths. ### Are these changes tested? Yes. The regression test checks the schemas created. It failed with the old implementation and passed with the new implementation. Local validation included: - targeted GH-51231 regression: 1/1 passed - expression-level nullability regressions: 5/5 passed - full arrow-compute-expression-test: 54/54 passed - full arrow-acero-plan-test: 63/63 passed - registered expression and plan CTests: 2/2 passed - clang-format v18.1.8 passed - cpplint 1.6.1 passed - output of git diff --check and git diff --cached --check was clean The targeted regression analysis was conducted with: ```bash ./cpp/build-gh51231/debug/arrow-acero-plan-test \ --gtest_filter=ExecPlanExecution.ProjectPreservesDirectFieldNullability ``` The expression and Acero plan test suites were evaluated with: ```bash PYTHON=python3 ctest --test-dir cpp/build-gh51231 \ --output-on-failure \ -j2 \ -R '^(arrow-compute-expression-test|arrow-acero-plan-test)$' ``` Substrait integration was not built locally. The regression test is defined at the native Acero ProjectNode level. ### Are there any visible user impacts? Yes. Top-level field projections copy the nullable attribute from input fields to output fields. This change only alters the projected schema. Projected values remain the same. The experimental Expression API gains the conservative `nullable()` accessor, and bound parameters store nullability. ### Was AI involved in this PR? Yes. AI tools were used to assist with the investigation, implementation and test drafting, review, and preparation of this description. I reproduced the issue locally and reviewed the patch, verifying that the test case failed without the patch and passed with the patch, and ran the listed C++ tests and style checks. **PR code and description by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed * GitHub Issue: #51231 Authored-by: Fady Youssery <fadyyosry2005@gmail.com> Signed-off-by: Rossi Sun <zanmato1984@gmail.com> |
||
|
Hanayoshi-8744
|
c7944b35f3 |
GH-51354: [C++][Parquet] Update bundled Apache Thrift to 0.24.0 (#51559)
### Rationale for this change The bundled Apache Thrift dependency (0.22.0) is affected by CVE-2026-55969, an integer overflow vulnerability in `TTransport::checkReadBytesAvailable()` that could bypass message size checks when reading maliciously crafted Thrift-encoded data. Arrow's Parquet module relies on Thrift's compact protocol to deserialize Parquet file metadata, so this is relevant to Arrow. ### What changes are included in this PR? - Bump the bundled Apache Thrift version from 0.22.0 to 0.24.0 in `cpp/thirdparty/versions.txt` (including the SHA256 checksum). - Remove the Clang-only `thrift-3187.patch` and its application logic in `ThirdpartyToolchain.cmake`. This patch pre-applied Thrift's upstream fix for THRIFT-3268 (a compiler warning), which has since been merged into Thrift itself and is already included in 0.24.0. Keeping the patch would make `git apply`/`patch` fail during the bundled build with Clang. ### Are these changes tested? - Verified that `thrift-0.24.0.tar.gz` downloads correctly and its SHA256 checksum matches the value published at downloads.apache.org. - Confirmed the CVE fix is present in Thrift 0.24.0 by diffing `TProtocol.h`, `TCompactProtocol.h`, `TBinaryProtocol.h`, and `TTransport.h` against 0.22.0. - Confirmed `thrift-3187.patch` no longer applies cleanly against 0.24.0 sources (already fixed upstream), which is why it was removed rather than kept as a no-op. - Built Arrow C++ locally on macOS (arm64, AppleClang) with `-DARROW_PARQUET=ON -DARROW_BUILD_TESTS=ON -DThrift_SOURCE=BUNDLED`, on top of the latest `main` (after rebasing), and ran the following Parquet tests. All of them passed with no failures (some tests were skipped, e.g. those requiring Snappy, which was not enabled in my build): `parquet-internals-test`, `parquet-file-deserialize-test`, `parquet-schema-test`, `parquet-reader-test`, `parquet-writer-test`, `parquet-arrow-reader-writer-test`, `parquet-arrow-metadata-test`, `parquet-arrow-index-test`, and `parquet-arrow-internals-test`. - Not tested locally: the removed patch was only applied for `CMAKE_CXX_COMPILER_ID STREQUAL "Clang"`, which my AppleClang build does not hit, so building the bundled Thrift with Clang on Linux relies on CI. I also did not test encryption, Windows/MSVC, or the Python/R packaging builds locally. ### Are there any user-facing changes? No. * GitHub Issue: #51354 --- Disclosure: this change was prepared with the assistance of an AI coding tool (Claude Code). I reviewed the diff, verified the CVE fix and checksum myself, and ran the test suite locally before opening this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Authored-by: Hanayoshi-8744 <201203709+Hanayoshi-8744@users.noreply.github.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Animesh Kumar
|
6c82e246e2 |
GH-51780: [C++][IPC] Remove the mislabeled null check on schema fields in GetSchema (#51811)
### Rationale for this change In `GetSchema`, the loop over the schema's fields checked each `Field` for null with the label `"DictionaryEncoding.indexType"`. That label belongs to a different check, so if this one ever failed, the error would point at the wrong field. As the `XXX` comment next to it said, the check also can't fail: for a vector of tables, `fields()->Get(i)` returns the element's address plus its stored offset, never null. `Schema.fields` itself is already checked for null just above. ### What changes are included in this PR? The check and its comment are removed, which is the first option in the issue. Child fields in `FieldFromFlatbuffer` are already read through `children->Get(i)` without a null check, so top-level fields now match. ### Are these changes tested? No new test, since the removed check couldn't fail. The `arrow-ipc-*` tests pass locally. ### Are there any user-facing changes? No. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [ ] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #51780 Authored-by: Animesh Kumar <kanimesh187@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
赵家贝
|
bffbbe7428 |
GH-51673: [C++][Parquet] Skip missing min/max in metadata printer (#51705)
### Rationale for this change Parquet column statistics can contain a null count without min/max values, for example when every value in a column chunk is null. `ParquetFilePrinter::DebugPrint` currently formats min and max whenever any statistics are present. For a DECIMAL stored as `FIXED_LEN_BYTE_ARRAY`, formatting the missing max passes an empty byte array to `Decimal256::FromBigEndian` and makes `parquet-reader --only-metadata` fail. Closes #51673. ### What changes are included in this PR? - Print min and max only when their respective encoded statistics are present. Keep the existing output for columns with both values. - Add a self-contained reader test that writes an all-null DECIMAL column, verifies that null-count statistics exist without min/max, and checks that the text printer omits both values. ### Are these changes tested? - `parquet-reader-test --gtest_filter=TestDebugPrintWithMemoryFile.AllNullDecimalStatistics:TestAllTypesPlain.DebugPrintWorks` - `parquet-reader --only-metadata` against the Open Prices `prices.parquet` file referenced in the issue: all 313 row groups print successfully. ### Are there any user-facing changes? `parquet-reader --only-metadata` now prints valid files with all-null DECIMAL column chunks without an error. Missing min/max values are omitted instead of being formatted. ### Was AI used for this PR? In accordance with the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [ ] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed AI assisted with the code, test, and PR description. The issue was reproduced on the public Parquet file and investigated at the throw site with LLDB. * GitHub Issue: #51673 Lead-authored-by: 赵家贝 <970058803@qq.com> Co-authored-by: 赵家贝 <41840745+Sunny-Island@users.noreply.github.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Antoine Pitrou
|
c74c9576f7 |
GH-51757: [C++] Limit max JSON nesting depth in JSON parser (#51758)
### Rationale for this change Our JSON parser is inherently recursive, and a crafted JSON document with huge nesting can send it into a stack overflow. Given that real-world JSON files will not have thousands levels of nesting, we limit the JSON nesting to an arbitrary value of 300. ### Are these changes tested? Yes, by new tests. ### Are there any user-facing changes? **This PR contains a "Critical Fix".** A crafted JSON document with a large nesting depth could trigger a denial of service through stack overflow. ### Was AI used for this PR? **PR code and description written by:** - [x] Human - [ ] AI **Reviewed before submission by:** - [x] Human (self-reviewed) - [ ] AI - [ ] Not reviewed * GitHub Issue: #51757 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Aaditya Srinivasan
|
bbbc06f8a8 |
GH-51037: [C++] Replace RapidJSON with simdjson in JSON parser (#51038)
### Rationale for this change This PR continues the simdjson migration by replacing the RapidJSON-based parsing implementation used by the JSON parser. The existing parser uses RapidJSON's SAX/handler interface to parse JSON values and populate Arrow builders. This change replaces that implementation with simdjson's ondemand API while retaining the existing builder and type-inference logic. ### Changes * Replace the RapidJSON parser and handler interface with simdjson's ondemand API. * Parse JSON documents using `simdjson::ondemand::parser::iterate_many`. * Use `ResolveSimdjsonResult()` consistently when handling simdjson results. * Preserve support for nested objects and arrays. * Preserve explicit-schema and inferred-field behavior. * Preserve unexpected-field handling for `Error`, `Ignore`, and `InferType`. * Continue storing numeric values as raw JSON tokens. * Trim trailing whitespace from numeric raw tokens to preserve existing behavior. * Preserve JSON parse error propagation through `Status::Invalid`. * Remove the parser's RapidJSON-specific dependencies. ### Are there any user-facing changes? No ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [X] Human - [X] AI **Reviewed before submission by:** - [X] Human - [X] AI - [ ] Not reviewed Fixes: #51037 * GitHub Issue: #51037 Authored-by: Aaditya Srinivasan <aadityasri03@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Dominique Belhachemi
|
36f655f44c |
GH-51476: [C++][Parquet] Vectorise FindMinMax explicitly (#51478)
### Rationale for this change FindMinMaxAvx2 in parquet/level_comparison_avx2.cc was not vectorised. ### What changes are included in this PR? A single xsimd kernel `FindMinMaxSimd<Arch>` in `level_comparison_simd_kernel_internal.h`, plus support for multiple architectures. ### Are these changes tested? Yes, with a nice speedup. | dispatch level | time | |---|---| | NONE | 513 ns | | SSE4_2 | 54.9 ns (9.3x) | | AVX2 | 28.7 ns (17.9x) | ### Are there any user-facing changes? No ### Was AI used for this PR? **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed * GitHub Issue: #51476 Authored-by: Dominique Belhachemi <db@domibel.de> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
fenfeng9
|
6b1ad60ab5 |
GH-50658: [C++][Dev] Add RunEndEncoded support to gdb_arrow.py (#50768)
### Rationale for this change The Arrow GDB pretty-printers do not support run-end encoded data types. ### What changes are included in this PR? Add GDB pretty-printer support and tests for `run-end encoded` types, scalars, and arrays. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #50658 Authored-by: fenfeng9 <fenfeng9@qq.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Lorenz Hübschle
|
bb1aba270e |
GH-51308: [C++] Initialize gzip codec state lazily (#51483)
### Rationale for this change `GZipCodec::Init()` initializes compressor state and then immediately initializes decompressor state, which tears the compressor state down again. Compressor state is fairly expensive to initialize (the `deflateInit2` call allocates ~256KiB), so this is rather wasteful. A caller that wants to compress has to do it twice (because decompressor initialization throws it away), and a caller that only wants to decompress still has to do the compressor setup. Whatever the caller wants to do with the GZipCodec, this is pure waste. ### What changes are included in this PR? `GZipCodec::Init()` now validates the compression level and window size without allocating codec state. Since all the compression and decompression methods have to check the mode anyway before doing any work, this is safe. ### Are these changes tested? Largely relying on existing test coverage; the interesting part is that `InitCompressor()` validated the compression level internally, so this check has to be extracted. Split the unit test that covered this into a few separate cases to make it clearer. ### Are there any user-facing changes? No. ### Was AI used for this PR? Yes, both to find the issue and to fix it (Codex). **PR code and description written by:** - [X] Human (PR desc) - [X] AI (code) **Reviewed before submission by:** - [X] Human - [ ] AI - [ ] Not reviewed # * GitHub Issue: #51308 Authored-by: Lorenz Hübschle <lorenz@firebolt.io> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Antoine Pitrou
|
ab7a6af3ee |
GH-51649: [C++][Parquet] Tell encoding fuzzer to skip inputs with invalid parameters (#51650)
### Rationale for this change `LLVMFuzzerTestOneInput` allows two return values: 0 and -1. While 0 allows for the input to be (potentially) saved in the corpus, -1 tells the fuzzer not to save it. In the words of the [fine documentation](https://llvm.org/docs/LibFuzzer.html#rejecting-unwanted-inputs): > It may be desirable to reject some inputs, i.e. to not add them to the corpus. > For example, when fuzzing an API consisting of parsing and other logic, one may want > to allow only those inputs into the corpus that parse successfully. > If the fuzz target returns -1 on a given input, libFuzzer will not add that > input to the corpus, regardless of what coverage it triggers. ### What changes are included in this PR? 1. Add a `FuzzStatus` type that allows telling the fuzzing engine whether an input should be skipped (i.e. not saved in the corpus). 2. Skip inputs where the parameter header contains invalid values in the Parquet encoding fuzzer ### Are these changes tested? Manually, by running the fuzzer from the command line and watching logs with `ARROW_FUZZING_VERBOSITY` enabled. Without a seed corpus and trying to generate 100k inputs (`./build/out/arrow/parquet-encoding-fuzz -seed=42 -runs=100000`), I get: * before: ``` #100000 DONE cov: 1377 ft: 2265 corp: 46/3357b lim: 778 exec/s: 7142 rss: 201Mb Done 100000 runs in 14 second(s) ``` * after: ``` #100000 DONE cov: 2051 ft: 3723 corp: 41/5725b lim: 832 exec/s: 50000 rss: 259Mb Done 100000 runs in 2 second(s) ``` It seems that this PR allows the fuzzer to proceed much faster when mutation generates such inputs. Despite running faster, it also reaches more coverage (see [doc](https://llvm.org/docs/LibFuzzer.html#output) about the `cov:` and `ft:` values). (*caveat:* these measurements is with a fixed random seed, and without a seed corpus) (of course, ideally the fuzzer would not produce such inputs at all, but ensuring that is [more involved](https://llvm.org/docs/LibFuzzer.html#user-supplied-mutators)) ### Are there any user-facing changes? No. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [ ] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #51649 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org> |
||
|
Hiroyuki Sato
|
e85e181188 |
GH-51670: [C++][Dev] Fix shellcheck errors in the cpp/examples/minimal_build directory (#51671)
### Rationale for this change This is the sub issue #44748. * SC2046: Quote this to prevent word splitting * SC2086: Double quote to prevent globbing and word splitting * SC2223: This default assignment may cause DoS due to globbing. Quote it ``` In build_arrow.sh line 23: mkdir -p $ARROW_BUILD_DIR ^--------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: mkdir -p "$ARROW_BUILD_DIR" In build_arrow.sh line 24: pushd $ARROW_BUILD_DIR ^--------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: pushd "$ARROW_BUILD_DIR" In build_arrow.sh line 30: $ARROW_CMAKE_OPTIONS ^------------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: "$ARROW_CMAKE_OPTIONS" In build_arrow.sh line 32: make -j$NPROC ^----^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: make -j"$NPROC" In build_example.sh line 21: mkdir -p $EXAMPLE_BUILD_DIR ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: mkdir -p "$EXAMPLE_BUILD_DIR" In build_example.sh line 22: pushd $EXAMPLE_BUILD_DIR ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: pushd "$EXAMPLE_BUILD_DIR" In run_static.sh line 21: : ${ARROW_DIR:=/arrow} ^------------------^ SC2223 (info): This default assignment may cause DoS due to globbing. Quote it. In run_static.sh line 22: : ${EXAMPLE_DIR:=/io} ^-----------------^ SC2223 (info): This default assignment may cause DoS due to globbing. Quote it. In run_static.sh line 23: : ${ARROW_BUILD_DIR:=/build/arrow} ^-- SC2223 (info): This default assignment may cause DoS due to globbing. Quote it. In run_static.sh line 24: : ${EXAMPLE_BUILD_DIR:=/build/example} ^-- SC2223 (info): This default assignment may cause DoS due to globbing. Quote it. In run_static.sh line 26: : ${ARROW_DEPENDENCY_SOURCE:=BUNDLED} ^-- SC2223 (info): This default assignment may cause DoS due to globbing. Quote it. In run_static.sh line 34: mkdir -p $ARROW_BUILD_DIR ^--------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: mkdir -p "$ARROW_BUILD_DIR" In run_static.sh line 35: pushd $ARROW_BUILD_DIR ^--------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: pushd "$ARROW_BUILD_DIR" In run_static.sh line 39: cmake $ARROW_DIR/cpp \ ^--------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: cmake "$ARROW_DIR"/cpp \ In run_static.sh line 43: -DARROW_DEPENDENCY_SOURCE=${ARROW_DEPENDENCY_SOURCE} \ ^------------------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: -DARROW_DEPENDENCY_SOURCE="${ARROW_DEPENDENCY_SOURCE}" \ In run_static.sh line 46: $ARROW_CMAKE_OPTIONS ^------------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: "$ARROW_CMAKE_OPTIONS" In run_static.sh line 48: make -j$NPROC ^----^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: make -j"$NPROC" In run_static.sh line 60: rm -rf $EXAMPLE_BUILD_DIR ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: rm -rf "$EXAMPLE_BUILD_DIR" In run_static.sh line 61: mkdir -p $EXAMPLE_BUILD_DIR ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: mkdir -p "$EXAMPLE_BUILD_DIR" In run_static.sh line 62: pushd $EXAMPLE_BUILD_DIR ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: pushd "$EXAMPLE_BUILD_DIR" In run_static.sh line 64: cmake $EXAMPLE_DIR -DARROW_LINK_SHARED=OFF ^----------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: cmake "$EXAMPLE_DIR" -DARROW_LINK_SHARED=OFF In run_static.sh line 76: pushd $EXAMPLE_DIR ^----------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: pushd "$EXAMPLE_DIR" In run_static.sh line 78: $EXAMPLE_BUILD_DIR/arrow-example ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: "$EXAMPLE_BUILD_DIR"/arrow-example In run_static.sh line 87: rm -rf $EXAMPLE_BUILD_DIR ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: rm -rf "$EXAMPLE_BUILD_DIR" In run_static.sh line 88: mkdir -p $EXAMPLE_BUILD_DIR ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: mkdir -p "$EXAMPLE_BUILD_DIR" In run_static.sh line 90: -o $EXAMPLE_BUILD_DIR/arrow-example \ ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: -o "$EXAMPLE_BUILD_DIR"/arrow-example \ In run_static.sh line 91: $EXAMPLE_DIR/example.cc \ ^----------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: "$EXAMPLE_DIR"/example.cc \ In run_static.sh line 92: $(PKG_CONFIG_PATH=$ARROW_BUILD_DIR/lib/pkgconfig \ ^-- SC2046 (warning): Quote this to prevent word splitting. In run_static.sh line 104: pushd $EXAMPLE_DIR ^----------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: pushd "$EXAMPLE_DIR" In run_static.sh line 106: $EXAMPLE_BUILD_DIR/arrow-example ^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: "$EXAMPLE_BUILD_DIR"/arrow-example For more information: https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt... https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ... https://www.shellcheck.net/wiki/SC2223 -- This default assignment may cause... ``` ### What changes are included in this PR? * SC2046: Quote command substitutions * SC2086: Quote variables * SC2223: Quote default assignments ### Are these changes tested? Yes. ### Are there any user-facing changes? No. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [ ] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #51670 Authored-by: Hiroyuki Sato <hiroysato@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
Raúl Cumplido
|
676dd43c5f |
GH-51687: [C++] Skipt tests that require threads if ARROW_ENABLE_THREADING=OFF (#51688)
### Rationale for this change The following PR introduced some tests that require threeading but those are not skipped when ARROW_ENABLE_THREADING=OFF failing on jobs like emscripten. - https://github.com/apache/arrow/pull/51498 ### What changes are included in this PR? Skip the tests if ARROW_ENABLE_THREADING=OFF ### Are these changes tested? Yes via archer with the failing job. ### Are there any user-facing changes? No ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [ ] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #51687 Authored-by: Raúl Cumplido <raulcumplido@gmail.com> Signed-off-by: Rossi Sun <zanmato1984@gmail.com> |
||
|
Hiroyuki Sato
|
142829f4fe |
GH-51682: [CI][C++] Add liburiparser-dev to system dependency image (#51683)
### Rationale for this change Changes around UriParser, including #51007, caused example-cpp-minimal-build-static-system-dependency to fail to build. ### What changes are included in this PR? This change adds liburiparser-dev to the system dependency image. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [ ] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #51682 Authored-by: Hiroyuki Sato <hiroysato@gmail.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com> |
||
|
tadeja
|
ba6c5fecdd |
GH-51638: [CI][C++][Parquet] Update expected JSON output in parquet-reader-test for simdjson 5.0 (#51661)
### Rationale for this change See #51638 - `fractured_json` formatting changes with the new simdjson version 5.0 (whitespace, line breaks, key order in tables), cause `parquet-reader-test` to fail on four of its tests `TestJSONWithLocalFile.JSONOutput...` ### What changes are included in this PR? Check `simdjson::SIMDJSON_VERSION_MAJOR`, and for `>= 5` use the new expected formatting (JSON content hasn't changed). Keep existing expected output formatting for simdjson 4.x ### Are these changes tested? Yes, CI runs pass. ### Are there any user-facing changes? No, only test changes. ### Was AI used for this PR? **PR code and description written by:** - [x] Human - [x] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #51638 Authored-by: Tadeja Kadunc <tadeja.kadunc@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> |
||
|
khan
|
ccfcdd24fc |
GH-31076: [C++] Reject overflowing STL allocation sizes (#51477)
### Rationale for this change The STL allocator computes `n * sizeof(T)` without checking for overflow. On a 64-bit platform, requesting `2^61` uint64_t elements wraps the byte count to zero. Requesting one more element wraps it to eight. Both allocations incorrectly succeed on upstream commit `5448aa7a916e73116ccc009454e3c19572dd5aad`, returning less storage than requested. ### What changes are included in this PR? Check the element count before multiplication against both the size_t limit and the int64_t limit used by MemoryPool. Reject oversized requests with the existing BadAlloc exception, which derives from std::bad_alloc. Add one regression test covering wraparound to zero and eight bytes. The test frees any unexpectedly successful allocation before reporting the missing exception. ### Are these changes tested? Yes, by additional unit test. ### Are there any user-facing changes? Just a bugfix. ### Was AI used for this PR? **PR code and description written by** - [ ] Human - [x] AI **Reviewed before submission by** - [x] Human - [x] AI - [ ] Not reviewed OpenAI Codex researched the issue and related PRs, wrote the reproducer, regression test, fix, and this description, ran the listed checks, and reviewed the diff with another AI agent. Grok was used for preliminary X research. No external implementation was copied into the patch. The human author reviewed the complete patch, discussed the overflow checks and regression test, and takes responsibility for the change. * GitHub Issue: #31076 Authored-by: Aikhan Jumashukurov <117706338+aikhanj@users.noreply.github.com> Signed-off-by: Antoine Pitrou <antoine@python.org> |