Repository navigation
Conversation
### Rationale for this change The `CumulativeOptions` class is not available in GLib/Ruby, and it is used together with the `cumulative_sum`, `cumulative_prod`, etc compute functions. ### What changes are included in this PR? This adds the `CumulativeOptions` to GLib. ### Are these changes tested? Yes, with Ruby unit tests. ### Are there any user-facing changes? Yes, a new class. * GitHub Issue: apache#48364 Authored-by: Sten Larsson <sten@burtcorp.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…48359) ### Rationale for this change It's the first float array. ### What changes are included in this PR? * Add `ArrowFormat::Float32Type` * Add `ArrowFormat::Float32Array` ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#48358 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…ache#48361) ### Rationale for this change It's the 64 bit offset version of binary array. ### What changes are included in this PR? * Add `ArrowFormat::LargeBinaryType` * Add `ArrowFormat::LargeBinaryArray` ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#48360 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
### Rationale for this change The `DictionaryEncodeOptions` class is not available in GLib/Ruby, and it is used together with the `dictionary_encode` compute function. ### What changes are included in this PR? This adds the `DictionaryEncodeOptions` class to GLib. ### Are these changes tested? Yes, with Ruby unit tests. ### Are there any user-facing changes? Yes, a new class. * GitHub Issue: apache#48366 Authored-by: Sten Larsson <sten@burtcorp.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…at spec (apache#48385) ### Rationale for this change: - Broken link for dictionary spec for parquet. - Fixes the same ### What changes are included in this PR? - Same as above ### Are these changes tested? - Yes, CI ### Are there any user-facing changes? - Yes, doc update * GitHub Issue: apache#48384 Authored-by: arnavb <arnavb@uber.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…48381) ### Rationale for this change It's the second floating point array. ### What changes are included in this PR? * Add `ArrowFromat::Float64Type` * Add `ArrowFormat::Float64Array` ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#48380 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…8383) ### Rationale for this change It's a nested array. ### What changes are included in this PR? * Add `ArrowFormat::StructType` * Add `ArrowFormat::StructArray` ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#48382 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…48374) ### Rationale for this change The `ElementWiseAggregateOptions` class is not available in GLib/Ruby, and it is used together with the `min_element_wise` and `max_element_wise` compute functions. ### What changes are included in this PR? This adds the `ElementWiseAggregateOptions` class to GLib. ### Are these changes tested? Yes, with Ruby unit tests. ### Are there any user-facing changes? Yes, a new class. * GitHub Issue: apache#48367 Authored-by: Sten Larsson <sten@burtcorp.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
### Rationale for this change It's a list of struct array. ### What changes are included in this PR? * Add `ArrowFormat::MapType` * Add `ArrowFormat::MapArray` ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#48388 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…ewline cop (apache#48392) ### Rationale for this change This fix introduces new Ruby formatting checks to streamline multi-person development. ### What changes are included in this PR? Enable [`Layout/TrailingEmptyLines`](https://docs.rubocop.org/rubocop/cops_layout.html#layouttrailingemptylines) options ```ruby # `final_newline` looks for one newline at the end of files. # bad class Foo; end # EOF # bad class Foo; end # EOF # good class Foo; end # EOF ``` ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#48386 Authored-by: Hiroyuki Sato <hiroysato@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…48338) ### Rationale for this change While working on PR apache#48336 I quickly got irritated by the clumsiness of the Parquet encryption configuration API. This issue attempts to reduce the verbosity of `ColumnEncryptionProperties` construction. ### What changes are included in this PR? 1. Remove unused `column_path` member in `ColumnEncryptionProperties` 2. Add convenience `ColumnEncryptionProperties` factory functions to avoid going through the clumsy `Builder` API ### Are these changes tested? Yes ### Are there any user-facing changes? Two deprecated constructors and a removed accessor for an useless property. **This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.) * GitHub Issue: apache#48337 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
### Rationale for this change Currently, if given an encrypted file (whether valid or invalid), the Parquet fuzz target will bail out almost immediately since it does not have any decryption key configured. This prevents the fuzzer from covering any significant part of the Parquet encryption codebase. ### What changes are included in this PR? 1. Improve the fuzz target so as to be able to read encrypted files present in the seed corpus (except those that require a external AAD or external key material) 2. Generate encrypted files in the seed corpus that the fuzz target is able to successfully decrypt ### Are these changes tested? In expanded unit test, and locally. ### Are there any user-facing changes? No. * GitHub Issue: apache#48335 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
… it for float16 (apache#48224) ### Rationale for this change Refer to [this comment](apache#48123 (comment)). Additionally, this change enables `arrow::WithinUlp` for `float16`. ### What changes are included in this PR? Re-implement `arrow::WithinUlp` and enable it for `float16`, including relevant tests for corner cases around powers of two and `Float16`. ### Are these changes tested? Yes, I ran the relevant unit tests. ### Are there any user-facing changes? No. * GitHub Issue: apache#48123 Lead-authored-by: arash andishgar <arashandishgar1@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
### Rationale for this change Respect the `MAKEFLAGS` that someone has set. Resolves apache#48340 ### What changes are included in this PR? Respect ### Are these changes tested? No, though we should see speedups in various places. ### Are there any user-facing changes? More respect. * GitHub Issue: apache#48340
apache#48343) ### Rationale for this change Building GCS is fragile in places and we're not sure how many (if anyone) really needs GCS. So let's turn it off by default and get some signal that folks want it back (also hopefully by that time we're on a more modern ABSL) ### What changes are included in this PR? Turn off GCS by default in R builds, also bundle when it is enabled. ### Are these changes tested? ### Are there any user-facing changes? No more GCS by default * GitHub Issue: apache#48342
### Rationale for this change The CSV fuzz target was fuzz-testing only the threaded table reader. This PR adds fuzz testing for more CSV-reading code paths: - the serial table reader - the streaming reader - the row-counting reader, with both possible chunker implementations depending on whether newlines are allowed in CSV values ### Are these changes tested? Manually. ### Are there any user-facing changes? No. * GitHub Issue: apache#47800 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
…ilure (apache#48166) **Rationale for this change** Fix `arrow-ipc-message-internal-test` on big endian systems by passing endianness explicitly. **Are these changes tested?** Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced. **Are there any user-facing changes?** No * GitHub main Issue link: apache#48151 * GitHub Issue: apache#48176 Lead-authored-by: Vishwanatha-HD <Vishwanatha.HD@ibm.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
…pache#48037) ### Rationale for this change Make ODBC return number of affected rows as -1 which to BI tools means number of affected rows is unknown. This is because ODBC only supports `select` statement and doesn't support queries that affect rows. ### What changes are included in this PR? - SQLRowCount & tests ### Are these changes tested? Tested locally on MSVC ### Are there any user-facing changes? N/A * GitHub Issue: apache#47713 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: David Li <li.davidm96@gmail.com>
### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>
…ion (apache#47762) ### Rationale for this change ODBC needs to give BI tools information about the driver itself and the data source it is connected to. ### What changes are included in this PR? - Implementation of `SQLGetinfo` to return driver and data source information - Add default values for SQLGetInfo in `get_info_cache.cc` - Tests ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? No * GitHub Issue: apache#47709
…apache#48036) ### Rationale for this change Implement ODBC to return the number of result columns ### What changes are included in this PR? - SQLNumResultCols implementation & tests ### Are these changes tested? Tested locally on MSVC ### Are there any user-facing changes? N/A * GitHub Issue: apache#47713 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…ache#48021) ### Rationale for this change Addresses apache#47719 ### What changes are included in this PR? SQLTables enabled. Table tests added. ### Are these changes tested? Tested locally on MSVC. ### Are there any user-facing changes? No. * GitHub Issue: apache#47719 Lead-authored-by: Alina (Xi) Li <alina.li@improving.com> Co-authored-by: justing-bq <justin.gossett@improving.com> Co-authored-by: alinalibq <alina.li@improving.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…che#48042) ### Rationale for this change Add support for column binding in ODBC. ### What changes are included in this PR? - Implement SQLBindCol & tests ### Are these changes tested? Tested on local MSVC ### Are there any user-facing changes? N/A * GitHub Issue: apache#47716 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…cation (apache#48053) ### Rationale for this change Add tests for `SQL_HANDLE_DESC` descriptor handle allocation, and diagnostics test. Related test updates ### What changes are included in this PR? - tests for `SQL_HANDLE_DESC` - Use `nullptr` explicitly where applicable - We use error associated returned from Driver manager to ensure users can get errors from descriptor handle ### Are these changes tested? Tested on local MSVC Windows ### Are there any user-facing changes? N/A * GitHub Issue: apache#47707 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: David Li <li.davidm96@gmail.com>
### Rationale for this change We have more features enabled on our fuzzing setup but we haven't updated the corresponding CMake preset. ### Are these changes tested? Manually. ### Are there any user-facing changes? No. * GitHub Issue: apache#48395 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…he#48411) ### Rationale for this change It's a large variant of list array. ### What changes are included in this PR? * Add `ArrowFormat::LargeListType` * Add `ArrowFormat::LargeListArray` * Improve large list support in Red Arrow ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#48410 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…8413) ### Rationale for this change It's a day variant of date array. ### What changes are included in this PR? * Add `ArrowFormat::Date32Type` * Add `ArrowFormat::Date32Array` * Add a missing test for float64 array (This is not related. Sorry.) ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#48412 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…pache#48390) ### Rationale for this change `shields.io` for pyarrow is broken for some reasons for last few days specifically for several packages with failing a request timeout: <img width="425" height="116" alt="Screenshot 2025-12-08 at 7 09 02 AM" src="https://github.com/user-attachments/assets/cca495d5-e34f-491e-95d0-b8595c306e60" /> Instead of relying on the thirdparty project, we should better use the official ones provided by Anaconda in https://anaconda.org/conda-forge/pyarrow/badges ### What changes are included in this PR? Changes the URI for Conda badge to use the official one provided in Anaconda (https://anaconda.org/conda-forge/pyarrow/badges). ### Are these changes tested? Yes, manually tested via my browser. ### Are there any user-facing changes? No. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…apache#48422) ### Rationale for this change https://github.com/apache/arrow/blob/4a8194e5c5b65579e09439121328aa3a448a7bf0/python/pyarrow/tests/test_dataset.py#L3436-L3442 was disabled because of batch size implementation missing in ORC scanner (ARROW-14153). This is resolved now so we can enable the tests. ### What changes are included in this PR? Enable test_orc_scan_options with batch_size ### Are these changes tested? Yes, via ``` python/pyarrow/tests/test_dataset.py::test_orc_dataset_reader ``` ### Are there any user-facing changes? No, test-only. * GitHub Issue: apache#48421 Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
…ARROW-14853) (apache#48407) ### Rationale for this change Seems like it is decided to make the error message better explicitly in ARROW-14853 and apache#11961 Therefore, this test can be converted to the negative test. https://github.com/apache/arrow/blob/2a3c5db7900028fbdf54a4162ce6c5b3f6e9f8e4/python/pyarrow/tests/test_compute.py#L3423 ### What changes are included in this PR? This PR proposes to add a negative test. ### Are these changes tested? Yes, via `python -m pytest pyarrow/tests/test_compute.py::test_struct_fields_options -v` ### Are there any user-facing changes? No, test-only. * GitHub Issue: apache#48406 Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
…river and the Flight Client (apache#48967) ### Rationale for this change The bug breaks a Flight SQL server that refreshens the auth token when cookie authentication is enabled ### What changes are included in this PR? 1. In the ODBC layer, removed the code that adds a 2nd ClientCookieMiddlewareFactory in the client options (the 1st one is registered in `BuildFlightClientOptions`). This fixes the issue of the duplicate header cookie fields. 2. In the flight client layer, uses the case-insensitive equality comparator instead of the case-insensitive less-than comparator for the cookies cache which is an unordered map. This fixes the issue of duplicate cookie keys. ### Are these changes tested? Manually on Windows, and CI ### Are there any user-facing changes? No * GitHub Issue: apache#48966 Authored-by: jianfengmao <jianfengmao@deephaven.io> Signed-off-by: David Li <li.davidm96@gmail.com>
…d and add check to validate LICENSE.txt and NOTICE.txt are part of the wheel contents (apache#48988) Currently the files are missing from the published wheels. - Ensure the license and notice files are part of the wheels - Use build frontend to build wheels - Build wheel from sdist Yes, via archery. I've validated all wheels will fail with the new check if LICENSE.txt or NOTICE.txt are missing: ``` AssertionError: LICENSE.txt is missing from the wheel. ``` No * GitHub Issue: apache#48983 Lead-authored-by: Raúl Cumplido <raulcumplido@gmail.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Co-authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
…che#49060) ### Rationale for this change Fix two issues found by OSS-Fuzz in the IPC reader: * a controlled abort on invalid IPC metadata: https://oss-fuzz.com/testcase-detail/5301064831401984 * a nullptr dereference on invalid IPC metadata: https://oss-fuzz.com/testcase-detail/5091511766417408 None of these two issues is a security issue. ### Are these changes tested? Yes, by new unit tests and new fuzz regression files. ### Are there any user-facing changes? No. **This PR contains a "Critical Fix".** (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.) * GitHub Issue: apache#49059 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
…che#48871) ### Rationale for this change I noticed a reference to a `release_candidate.sh` in the `paths` field in `release_candidate.yml` which is a file that doesn't exist. I think this was just a typo made during refactoring. ### What changes are included in this PR? Corrected `paths` list entry. ### Are these changes tested? No. ### Are there any user-facing changes? No. Authored-by: Bryce Mecum <petridish@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…dding required user-agent on urllib request (apache#49052) ### Rationale for this change See: apache#49044 ### What changes are included in this PR? Urllib now request with `"user-agent": "pyarrow"` ### Are these changes tested? It's a CI fix. ### Are there any user-facing changes? No, just a CI test fix. * GitHub Issue: apache#49044 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
… parsing (apache#49095) ### Rationale for this change This PR restores the behavior previous to version 23 for floating-point parsing on overflow and subnormal. `fast_float` didn't assign an error code on overflow in version `3.10.1` and assigned `±Inf` on overflow and `0.0` on subnormal. With the update to version `8.1`, it started to assign `std::errc::result_out_of_range` in such cases. ### What changes are included in this PR? Ignores `std::errc::result_out_of_range` and produce `±Inf` / `0.0` as appropriate instead of failing the conversion. ### Are these changes tested? Yes. Created tests for overflow with positive and negative signed mantissa, and also created tests for subnormal, all of them for binary{16,32,64}. ### Are there any user-facing changes? It's a user facing change. The CSV reader on version `libarrow==23` was assigning them as strings, while before it was parsing it as `0` or `+- inf`. With this patch, the CSV reader in PyArrow outputs: ```python >>> import pyarrow >>> import pyarrow.csv >>> import io >>> table = pyarrow.csv.read_csv(io.BytesIO(f"data\n10E-617\n10E617\n-10E617".encode())) >>> print(table) pyarrow.Table data: double ---- data: [[0,inf,-inf]] ``` Closes apache#49003 * GitHub Issue: apache#49003 Authored-by: Alvaro-Kothe <kothe65@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
…m manylinux wheel dockerfile (apache#49139) We use nightlies version of Cython for free-threaded PyArrow wheels and they are currently failing, see apache#49138 Nightly Cython install is removed and Cython is installed via [requirements file](https://github.com/apache/arrow/blob/main/python/requirements-wheel-build.txt#L2). Tes. No. * GitHub Issue: apache#49138 Authored-by: AlenkaF <frim.alenka@gmail.com> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
) ### Rationale for this change With Cython 3.3.0.a0 this failed. After some discussion it seems that this should have always had to require the GIL. ### What changes are included in this PR? Moving statement out of the `with nogil` context manager. ### Are these changes tested? Existing CI builds pyarrow. ### Are there any user-facing changes? No * GitHub Issue: apache#49156 Authored-by: Raúl Cumplido <raulcumplido@gmail.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
### Rationale for this change Default Debian version in `.env` now maps to oldstable, we should use stable instead. Also prune entries that are not used anymore. ### Are these changes tested? By existing CI jobs. ### Are there any user-facing changes? No. * GitHub Issue: apache#49024 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…48819) Resolves: apache#48817 ### Rationale for this change Keep R build infrastructure inline with our C++ version ### What changes are included in this PR? Mostly `s/CXX17/CXX20/g` ### Are these changes tested? Yes, lots of CI ### Are there any user-facing changes? **This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.) **This PR contains a "Critical Fix".** (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.) * GitHub Issue: apache#48817 Authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Jonathan Keane <jkeane@gmail.com>
) ### Rationale for this change `repeat()` can only generate `< 2147483647` size output. So output larger than `2147483647` must be rejected. ### What changes are included in this PR? Add overflow check in `repeat()`. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#49159 Lead-authored-by: Sutou Kouhei <kou@cozmixng.org> Co-authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
…m generator (dremio#106) * mac vcpkg * Add cpu attrs to llvm code generation. * unit test * Update test
* Fix Jar build * apacheGH-46708: [C++][Gandiva] Added zero return values for castDECIMAL_utf8 (apache#46709) ### Rationale for this change castDECIMAL_utf8 has undefined behavior if input value can not be parsed, which causes SIGSEGV for some expressions in Projector. ### What changes are included in this PR? Setting 0 to out_high, out_low in function castDECIMAL_utf8 to have those values initialised if input value can not be parsed. Added corresponding test that reproduces SIGSEGV in projector. ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#46708 Authored-by: DenisTarasyuk <denis.tarasyuk@dremio.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> --------- Signed-off-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Tim Hurski <tim.hurski@dremio.com> Co-authored-by: DenisTarasyuk <131180287+DenisTarasyuk@users.noreply.github.com>
…s in `castTIMESTAMP_utf8` and `castTIME_utf8` (apache#48867) ### Rationale for this change Fixes apache#48866. The Gandiva precompiled time functions `castTIMESTAMP_utf8` and `castTIME_utf8` currently reject timestamp and time string literals with more than 3 subsecond digits (beyond millisecond precision), throwing an "Invalid millis" error. This behavior is inconsistent with other implementations. ### What changes are included in this PR? - Fixed `castTIMESTAMP_utf8` and `castTIME_utf8` functions to truncate subseconds beyond 3 digits instead of throwing an error - Updated tests. Replaced error-expecting tests with truncation verification tests and added edge cases ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#48866 Authored-by: Arkadii Kravchuk <arkadii.kravchuk@dremio.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
* apacheGH-48159 [C++][Gandiva] Projector make is significantly slower after move to OrcJIT (apache#49063) ### Rationale for this change Reduces LLVM TargetMachine object creation from 3 to 1. This object is expensive to create and the extra copies weren't needed. ### What changes are included in this PR? Refactor the Engine class to only create one target machine and pass that to the necessary functions. Before the change (3 TargetMachines created): First TargetMachine: In Engine::Make(), MakeTargetMachineBuilder() is called, then BuildJIT() is called. Inside LLJITBuilder::create(), when prepareForConstruction() runs, if no DataLayout was set, it calls JTMB->getDefaultDataLayoutForTarget() which creates a temporary TargetMachine just to get the DataLayout. Second TargetMachine: Inside BuildJIT(), when setCompileFunctionCreator is used with the lambda, that lambda calls JTMB.createTargetMachine() to create a TargetMachine for the TMOwningSimpleCompiler. Third TargetMachine: Back in Engine::Make(), after BuildJIT() returns, there's an explicit call to jtmb.createTargetMachine() to create target_machine_ for the Engine. After the change (1 TargetMachine created): The key changes are: Create TargetMachine first: The code now creates the TargetMachine explicitly at the start of the Engine in Engine::Make. That machine is passed to BuildJIT. In BuildJiIT that machine's DataLayout is sent to LLJITBuilder which prevents prepareForConstruction() from calling getDefaultDataLayoutForTarget() (which would create a temporary TargetMachine). Use SimpleCompiler instead of TMOwningSimpleCompiler: SimpleCompiler takes a reference to an existing TargetMachine rather than owning one, so no new TargetMachine is created. A shared_ptr is used to ensure that TargetMachine stays around for the lifetime of the LLJIT instance. ### Are these changes tested? Yes, unit and integration. ### Are there any user-facing changes? No. * GitHub Issue: apache#48159 Lead-authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com> Co-authored-by: Logan Riggs <logan.riggs@dremio.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> * apacheGH-49034 [C++][Gandiva] Fix binary_string to not trigger error for null strings (apache#49035) ### Rationale for this change The binary_string function will attempt to allocate 0 bytes of memory, which results in a null ptr being returned and the function interprets that as an error. ### What changes are included in this PR? Add kCanReturnErrors to the function definition to match other string functions. Move the check for 0 byte length input earlier in the binary_string function to prevent the 0 allocation. Add a unit test. ### Are these changes tested? Yes, unit and integration testing. ### Are there any user-facing changes? No. * GitHub Issue: apache#49034 Authored-by: Logan Riggs <logan.riggs@dremio.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> --------- Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…sing SYSTEM includes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.