Skip to content

DX-116527, DX-105095, DX-116032, DX-122700, DX-63755, DX-59505 - #144

Open
lriggs wants to merge 5 commits into
dremio:dremio_27.0_23_19from
lriggs:cpChanges
Open

DX-116527, DX-105095, DX-116032, DX-122700, DX-63755, DX-59505#144
lriggs wants to merge 5 commits into
dremio:dremio_27.0_23_19from
lriggs:cpChanges

Conversation

@lriggs

@lriggs lriggs commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Bring in upstream Arrow fixes for several dremio issues:

DX Title
DX-116527 CHR only supports ASCII (0-127), returns planning error for Unicode code points
DX-105095 Multiple execs crash with SIGSEGV dump
DX-116032 Gandiva castVARCHAR decimal128 causes SIGSEGV crash (native memory corruption)
DX-122700 Gandiva and Java REPLACE functions support arbitrarily small output strings
DX-63755 REGEXP_EXTRACT behavior differs between Software and Cloud
DX-59505 ln function not found in test1 cluster but running in software

commit 7f1bb41
Author: Logan Riggs logan.riggs@gmail.com
Date: Mon Jun 15 17:43:50 2026 -0700

GH-50111: [C++][Gandiva] Improve function error messages (#50112)

Gandiva's runtime errors are surfaced to SQL users via `ExecutionContext::set_error_msg`. An audit of all ~90 call sites in `cpp/src/gandiva/` turned up two recurring problems:

1. **No SQL function name in the message.** Users see `"divide by zero error"` or `"Output buffer length can't be negative"` with no indication of which SQL function produced it, making errors hard to localize in long queries.
2. **The offending value is not echoed.** Many messages reject a value (invalid weekday, bad boolean string, out-of-range index, …) without telling the user what was supplied.

A good runtime error should answer four questions: *which function*, *what went wrong*, *what value triggered it*, *what's the valid range*. This PR moves the highest-impact messages toward that bar.

All edits follow the same pattern:

```
<SQL_FUNCTION_NAME>: <what failed>; <offending value>; <valid range or hint>
```

Where the old message contained a load-bearing substring (e.g. `divide by zero error`, `Output buffer length can't be negative`), the substring is preserved so existing substring-based tests still match.

[cpp/src/gandiva/regex_functions_holder.cc:239-247](cpp/src/gandiva/regex_functions_holder.cc#L239-L247) — the message `"Index to extract out of range"` now reads `"REGEXP_EXTRACT: invalid group_index '<N>'; must be between 0 and <max> (the number of capture groups in the pattern)"`, matching the level of detail of the Java path.

Eight call sites in `arithmetic_ops.cc`, `decimal_ops.cc`, and `extended_math_ops.cc` (covering `DIVIDE`, `DIV`, `MOD`, `PMOD`, decimal `Divide`/`Mod`, `LOG`) now prefix the SQL function name while preserving the original `divide by zero error` substring. `mod_float64_float64` additionally echoes the dividend.

The following sites now include both the SQL function name and the offending value:

- `CAST_BIT` — echoes the invalid input string, lists expected values
- `CAST_VARCHAR` / `CAST_VARBINARY` length checks — echoes requested length
- `REPEAT` — echoes count (and on overflow: count × input length)
- `CONVERT_REPLACE_INVALID_FROM_UTF8` — echoes byte count
- `LOCATE` — echoes start position
- `FACTORIAL` — echoes input value (both negative and >20 paths)
- `NEGATIVE` (integer overflow on `INT_MIN`, and `negative_daytimeinterval` out-of-bounds)
- `CRC32` — echoes input length
- `BASE64` / `UNBASE64` — echoes input length
- `AES_ENCRYPT` / `AES_DECRYPT` — echoes data length on negative input; `AES_ENCRYPT` OOM message also rewritten to name the function clearly
- `NEXT_DAY` — echoes the unrecognized weekday string, lists expected values
- `CAST_INTERVAL_YEAR` — echoes the overflowing source value
- `CAST_*_FROM_HEX` (3 error paths in the macro) — echoes the input hex string
- `REPLACE` — buffer-overflow message prefixed with `REPLACE:`

- Strict `EXPECT_EQ(error, "exact string")` assertions converted to `HasSubstr` / `.find()` so the test contract is the SQL function name plus a stable phrase, not the exact wording.
- Updated assertions where the new message no longer contains the old free-text fragment (e.g. `"Factorial of negative"` → `HasSubstr("FACTORIAL") + HasSubstr("non-negative")`).
- Files touched: `arithmetic_ops_test.cc`, `decimal_ops_test.cc`, `extended_math_ops_test.cc`, `time_test.cc`, `gdv_function_stubs_test.cc`.

```bash
cd cpp/debug
cmake --build . --target gandiva_shared gandiva-precompiled-test gandiva-internals-test gandiva-projector-test -j4
./debug/gandiva-precompiled-test     # 130/130 pass
./debug/gandiva-internals-test       # 156/156 pass
./debug/gandiva-projector-test       # 218/218 pass
```

All affected error paths are exercised by the existing unit tests — that is how each site was located in the audit.

Yes, improved error messages.

```
-- before
SELECT REGEXP_EXTRACT('100-500', '(\d+)-(\d+)', -1);
-- FUNCTION ERROR: Index to extract out of range

-- after
-- FUNCTION ERROR: REGEXP_EXTRACT: invalid group_index '-1';
--                must be between 0 and 2
--                (the number of capture groups in the pattern)
```

```
-- before
SELECT CAST('maybe' AS BOOLEAN);
-- FUNCTION ERROR: Invalid value for boolean.

-- after
-- FUNCTION ERROR: CAST_BIT: Invalid value for boolean: 'maybe'
--                (expected 0, 1, true, false; case-insensitive)
```

```
-- before
SELECT NEXT_DAY(TIMESTAMP '2025-01-01 00:00:00', 'frusday');
-- FUNCTION ERROR: The weekday in this entry is invalid

-- after
-- FUNCTION ERROR: NEXT_DAY: 'frusday' is not a recognized weekday
--                (expected MON|TUE|WED|THU|FRI|SAT|SUN)
```

* GitHub Issue: #50111

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>

commit 689e9da
Author: Logan Riggs logan.riggs@gmail.com
Date: Thu Aug 13 14:28:50 2026 -0700

GH-50136: [C++][Gandiva] Enhance CHR to work with unicode (#50137)

`CHR(n)` only worked for ASCII (0–127). Values ≥ 128 emitted a single raw byte
(invalid UTF‑8), causing "Error during planning". Goal: emit the proper
multi‑byte **UTF‑8 encoding** of the Unicode code point, consistent with
PostgreSQL/Snowflake.

| File | Change |
|------|--------|
| `cpp/src/gandiva/precompiled/string_ops.cc` | `chr_int64` rewritten to UTF‑8‑encode the code point (1–4 bytes) and error on invalid input (negative, > 0x10FFFF, surrogate range 0xD800–0xDFFF). `chr_int32` now delegates to it. |
| `cpp/src/gandiva/precompiled/string_ops_test.cc` | `TestChrBigInt` rewritten for UTF‑8 semantics: every byte‑length boundary (1/2/3/4‑byte, low+high), í/€/日/😀, and the three invalid‑input error cases. |

Yes, unit tests.

Yes, the CHR gandiva function now supports unicode characters.

* GitHub Issue: #50136

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>

commit 74ae7c6
Author: Logan Riggs logan.riggs@gmail.com
Date: Tue Aug 4 15:07:25 2026 -0700

GH-50140: [C++][Gandiva] Fix castVARCHAR(decimal128) native memory corruption / SIGSEGV on allocation failure (#50141)

### Rationale for this change

The Gandiva `castVARCHAR_decimal128_int64` path could corrupt native memory and
crash the process (SIGSEGV) when the output-string arena allocation failed
(e.g. `CAST(decimal AS VARCHAR)` under memory pressure). Three independent
defects combined to cause this:

1. The `castVARCHAR` decimal128 registry entry was missing
   `NativeFunction::kCanReturnErrors`, so generated code skipped the error check
   and ignored any error the function reported.
2. `gdv_fn_dec_to_string` set the output length to a positive value *before*
   checking whether the allocation succeeded, then returned `nullptr` — leaving
   the caller to copy from an invalid buffer with a positive length.
3. `castVARCHAR_decimal128_int64` did not validate a negative requested output
   length and did not handle an upstream allocation failure.

### What changes are included in this PR?

- **`function_registry_string.cc`**: Add `NativeFunction::kCanReturnErrors` to the
  `castVARCHAR` `decimal128` entry so the generated code checks for and
  propagates errors instead of assuming the function never fails.

- **`gdv_function_stubs.cc`** (`gdv_fn_dec_to_string`): Only write the output
  length *after* a successful allocation. On allocation failure, set
  `*dec_str_len = 0` and return an empty string so callers never copy from an
  invalid buffer using a stale, positive length.

- **`precompiled/decimal_wrapper.cc`** (`castVARCHAR_decimal128_int64`):
  - Reject a negative output length with a graceful error
    (`"Output buffer length can't be negative"`) instead of using it as a copy
    size.
  - Bail out safely (zero length, empty string) if the upstream
    `gdv_fn_dec_to_string` call failed, since the error has already been set.

- **`tests/decimal_test.cc`**: Add `TestCastVarCharDecimalNegativeLength`, a
  regression test that casts a decimal to varchar with a negative output length
  and asserts the query fails gracefully with the expected error message rather
  than crashing. This also exercises the `kCanReturnErrors` flag — without it the
  error would not propagate and the test would fail.

### Behavior change

Queries such as `CAST(decimal AS VARCHAR)` that previously crashed the process
(SIGSEGV) under memory pressure now fail gracefully with an error message about
the allocation failure / invalid length, and the rest of the system is
unaffected.

### Are these changes tested?

Yes, unit tests.

### Are there any user-facing changes?

No.
* GitHub Issue: #50140

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>

commit ea676ee
Author: Logan Riggs logan.riggs@gmail.com
Date: Tue Aug 11 22:48:15 2026 -0700

GH-50186: [C++][Gandiva] REPLACE throws "Buffer overflow for output string" for results larger than 64 KB   (#50187)

Gandiva's REPLACE hardcodes a 65535-byte output buffer, throwing Buffer overflow for output string whenever the result exceeds 64 KB. The cap is arbitrary: Gandiva's variable-length output column already grows dynamically and is only bounded by the int32 offset width (~2 GB). Real queries that replace into large concatenated/aggregated strings fail unnecessarily.

replace_utf8_utf8_utf8 now sizes the output buffer to the exact result instead of using a fixed cap. The output length of a replace is deterministic:

out_len = text_len + num_matches * (to_str_len - from_str_len)
The wrapper does a single counting pass over the input to find the number of non-overlapping matches of from_str (mirroring the match loop already used in the implementation), computes the exact size in gdv_int64 to avoid intermediate overflow, and passes that as max_length.

The internal replace_with_max_len_utf8_utf8_utf8 is unchanged — its bounds checks now act purely as a correctness backstop (they should never fire with an exact bound), and its explicit-max-length signature remains for the existing unit tests.
When to is shorter than from, the result shrinks and max_length <= text_len, so the shrinking path is sized correctly too.
Yes. Added regression cases to TestStringOps.TestReplace in string_ops_test.cc:

A 35000-char 'X' input with X → XY, producing a 70000-byte result (previously overflowed at 65535) — asserts no error and exact length/content.
A 70000-char shrinking case (XX → X) to cover the shrink path on a >64 KB input.
Full precompiled suite passes locally (132/132), including the existing explicit-max_len overflow tests, which call the internal function directly and are unaffected.

REPLACE now succeeds on results larger than 64 KB instead of erroring. No API or signature changes.
* GitHub Issue: #50186

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>

commit a878ce2
Author: Logan Riggs logan.riggs@gmail.com
Date: Thu Aug 13 14:32:41 2026 -0700

GH-49977: [C++][Gandiva] Add regexp_extract optional third parameter function version (#49978)

### Rationale for this change
The existing 3 argument REGEXP_EXTRACT function requires the group index to return. It would be useful to have a 2 arg version of the function where the index defaults to 1. This would align well with other comparable database systems.

### What changes are included in this PR?
A new 2 arg REGEXP_FUNCTION and unit tests.

### Are these changes tested?
Yes, unit test and manual testing.

### Are there any user-facing changes?
Yes, a new 2 argument REGEXP_FUNCTION.
* GitHub Issue: #49977

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>

lriggs added 5 commits August 14, 2026 16:51
…meter function version (apache#49978)

### Rationale for this change
The existing 3 argument REGEXP_EXTRACT function requires the group index to return. It would be useful to have a 2 arg version of the function where the index defaults to 1. This would align well with other comparable database systems.

### What changes are included in this PR?
A new 2 arg REGEXP_FUNCTION and unit tests.

### Are these changes tested?
Yes, unit test and manual testing.

### Are there any user-facing changes?
Yes, a new 2 argument REGEXP_FUNCTION.
* GitHub Issue: apache#49977

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…tput string" for results larger than 64 KB (apache#50187)

Gandiva's REPLACE hardcodes a 65535-byte output buffer, throwing Buffer overflow for output string whenever the result exceeds 64 KB. The cap is arbitrary: Gandiva's variable-length output column already grows dynamically and is only bounded by the int32 offset width (~2 GB). Real queries that replace into large concatenated/aggregated strings fail unnecessarily.

replace_utf8_utf8_utf8 now sizes the output buffer to the exact result instead of using a fixed cap. The output length of a replace is deterministic:

out_len = text_len + num_matches * (to_str_len - from_str_len)
The wrapper does a single counting pass over the input to find the number of non-overlapping matches of from_str (mirroring the match loop already used in the implementation), computes the exact size in gdv_int64 to avoid intermediate overflow, and passes that as max_length.

The internal replace_with_max_len_utf8_utf8_utf8 is unchanged — its bounds checks now act purely as a correctness backstop (they should never fire with an exact bound), and its explicit-max-length signature remains for the existing unit tests.
When to is shorter than from, the result shrinks and max_length <= text_len, so the shrinking path is sized correctly too.
Yes. Added regression cases to TestStringOps.TestReplace in string_ops_test.cc:

A 35000-char 'X' input with X → XY, producing a 70000-byte result (previously overflowed at 65535) — asserts no error and exact length/content.
A 70000-char shrinking case (XX → X) to cover the shrink path on a >64 KB input.
Full precompiled suite passes locally (132/132), including the existing explicit-max_len overflow tests, which call the internal function directly and are unaffected.

REPLACE now succeeds on results larger than 64 KB instead of erroring. No API or signature changes.
* GitHub Issue: apache#50186

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…ory corruption / SIGSEGV on allocation failure (apache#50141)

### Rationale for this change

The Gandiva `castVARCHAR_decimal128_int64` path could corrupt native memory and
crash the process (SIGSEGV) when the output-string arena allocation failed
(e.g. `CAST(decimal AS VARCHAR)` under memory pressure). Three independent
defects combined to cause this:

1. The `castVARCHAR` decimal128 registry entry was missing
   `NativeFunction::kCanReturnErrors`, so generated code skipped the error check
   and ignored any error the function reported.
2. `gdv_fn_dec_to_string` set the output length to a positive value *before*
   checking whether the allocation succeeded, then returned `nullptr` — leaving
   the caller to copy from an invalid buffer with a positive length.
3. `castVARCHAR_decimal128_int64` did not validate a negative requested output
   length and did not handle an upstream allocation failure.

### What changes are included in this PR?

- **`function_registry_string.cc`**: Add `NativeFunction::kCanReturnErrors` to the
  `castVARCHAR` `decimal128` entry so the generated code checks for and
  propagates errors instead of assuming the function never fails.

- **`gdv_function_stubs.cc`** (`gdv_fn_dec_to_string`): Only write the output
  length *after* a successful allocation. On allocation failure, set
  `*dec_str_len = 0` and return an empty string so callers never copy from an
  invalid buffer using a stale, positive length.

- **`precompiled/decimal_wrapper.cc`** (`castVARCHAR_decimal128_int64`):
  - Reject a negative output length with a graceful error
    (`"Output buffer length can't be negative"`) instead of using it as a copy
    size.
  - Bail out safely (zero length, empty string) if the upstream
    `gdv_fn_dec_to_string` call failed, since the error has already been set.

- **`tests/decimal_test.cc`**: Add `TestCastVarCharDecimalNegativeLength`, a
  regression test that casts a decimal to varchar with a negative output length
  and asserts the query fails gracefully with the expected error message rather
  than crashing. This also exercises the `kCanReturnErrors` flag — without it the
  error would not propagate and the test would fail.

### Behavior change

Queries such as `CAST(decimal AS VARCHAR)` that previously crashed the process
(SIGSEGV) under memory pressure now fail gracefully with an error message about
the allocation failure / invalid length, and the rest of the system is
unaffected.

### Are these changes tested?

Yes, unit tests.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#50140

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…he#50137)

`CHR(n)` only worked for ASCII (0–127). Values ≥ 128 emitted a single raw byte
(invalid UTF‑8), causing "Error during planning". Goal: emit the proper
multi‑byte **UTF‑8 encoding** of the Unicode code point, consistent with
PostgreSQL/Snowflake.

| File | Change |
|------|--------|
| `cpp/src/gandiva/precompiled/string_ops.cc` | `chr_int64` rewritten to UTF‑8‑encode the code point (1–4 bytes) and error on invalid input (negative, > 0x10FFFF, surrogate range 0xD800–0xDFFF). `chr_int32` now delegates to it. |
| `cpp/src/gandiva/precompiled/string_ops_test.cc` | `TestChrBigInt` rewritten for UTF‑8 semantics: every byte‑length boundary (1/2/3/4‑byte, low+high), í/€/日/😀, and the three invalid‑input error cases. |

Yes, unit tests.

Yes, the CHR gandiva function now supports unicode characters.

* GitHub Issue: apache#50136

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…e#50112)

Gandiva's runtime errors are surfaced to SQL users via `ExecutionContext::set_error_msg`. An audit of all ~90 call sites in `cpp/src/gandiva/` turned up two recurring problems:

1. **No SQL function name in the message.** Users see `"divide by zero error"` or `"Output buffer length can't be negative"` with no indication of which SQL function produced it, making errors hard to localize in long queries.
2. **The offending value is not echoed.** Many messages reject a value (invalid weekday, bad boolean string, out-of-range index, …) without telling the user what was supplied.

A good runtime error should answer four questions: *which function*, *what went wrong*, *what value triggered it*, *what's the valid range*. This PR moves the highest-impact messages toward that bar.

All edits follow the same pattern:

```
<SQL_FUNCTION_NAME>: <what failed>; <offending value>; <valid range or hint>
```

Where the old message contained a load-bearing substring (e.g. `divide by zero error`, `Output buffer length can't be negative`), the substring is preserved so existing substring-based tests still match.

[cpp/src/gandiva/regex_functions_holder.cc:239-247](cpp/src/gandiva/regex_functions_holder.cc#L239-L247) — the message `"Index to extract out of range"` now reads `"REGEXP_EXTRACT: invalid group_index '<N>'; must be between 0 and <max> (the number of capture groups in the pattern)"`, matching the level of detail of the Java path.

Eight call sites in `arithmetic_ops.cc`, `decimal_ops.cc`, and `extended_math_ops.cc` (covering `DIVIDE`, `DIV`, `MOD`, `PMOD`, decimal `Divide`/`Mod`, `LOG`) now prefix the SQL function name while preserving the original `divide by zero error` substring. `mod_float64_float64` additionally echoes the dividend.

The following sites now include both the SQL function name and the offending value:

- `CAST_BIT` — echoes the invalid input string, lists expected values
- `CAST_VARCHAR` / `CAST_VARBINARY` length checks — echoes requested length
- `REPEAT` — echoes count (and on overflow: count × input length)
- `CONVERT_REPLACE_INVALID_FROM_UTF8` — echoes byte count
- `LOCATE` — echoes start position
- `FACTORIAL` — echoes input value (both negative and >20 paths)
- `NEGATIVE` (integer overflow on `INT_MIN`, and `negative_daytimeinterval` out-of-bounds)
- `CRC32` — echoes input length
- `BASE64` / `UNBASE64` — echoes input length
- `AES_ENCRYPT` / `AES_DECRYPT` — echoes data length on negative input; `AES_ENCRYPT` OOM message also rewritten to name the function clearly
- `NEXT_DAY` — echoes the unrecognized weekday string, lists expected values
- `CAST_INTERVAL_YEAR` — echoes the overflowing source value
- `CAST_*_FROM_HEX` (3 error paths in the macro) — echoes the input hex string
- `REPLACE` — buffer-overflow message prefixed with `REPLACE:`

- Strict `EXPECT_EQ(error, "exact string")` assertions converted to `HasSubstr` / `.find()` so the test contract is the SQL function name plus a stable phrase, not the exact wording.
- Updated assertions where the new message no longer contains the old free-text fragment (e.g. `"Factorial of negative"` → `HasSubstr("FACTORIAL") + HasSubstr("non-negative")`).
- Files touched: `arithmetic_ops_test.cc`, `decimal_ops_test.cc`, `extended_math_ops_test.cc`, `time_test.cc`, `gdv_function_stubs_test.cc`.

```bash
cd cpp/debug
cmake --build . --target gandiva_shared gandiva-precompiled-test gandiva-internals-test gandiva-projector-test -j4
./debug/gandiva-precompiled-test     # 130/130 pass
./debug/gandiva-internals-test       # 156/156 pass
./debug/gandiva-projector-test       # 218/218 pass
```

All affected error paths are exercised by the existing unit tests — that is how each site was located in the audit.

Yes, improved error messages.

```
-- before
SELECT REGEXP_EXTRACT('100-500', '(\d+)-(\d+)', -1);
-- FUNCTION ERROR: Index to extract out of range

-- after
-- FUNCTION ERROR: REGEXP_EXTRACT: invalid group_index '-1';
--                must be between 0 and 2
--                (the number of capture groups in the pattern)
```

```
-- before
SELECT CAST('maybe' AS BOOLEAN);
-- FUNCTION ERROR: Invalid value for boolean.

-- after
-- FUNCTION ERROR: CAST_BIT: Invalid value for boolean: 'maybe'
--                (expected 0, 1, true, false; case-insensitive)
```

```
-- before
SELECT NEXT_DAY(TIMESTAMP '2025-01-01 00:00:00', 'frusday');
-- FUNCTION ERROR: The weekday in this entry is invalid

-- after
-- FUNCTION ERROR: NEXT_DAY: 'frusday' is not a recognized weekday
--                (expected MON|TUE|WED|THU|FRI|SAT|SUN)
```

* GitHub Issue: apache#50111

Authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant