fix(expr): decode BYTE_ARRAY page-index bounds by field type

[anoopj]

Which issue does this PR close?

• Closes Page index evaluation panics on binary column predicates #3246.

What changes are included in this PR?

The page index evaluator decoded every BYTE_ARRAY min/max bound as a UTF-8 string via String::from_utf8(..).unwrap(), panicking during scan pruning whenever a binary (or byte-encoded decimal) column carried a predicate. Convert the bound according to the field's primitive type instead: string, binary, or decimal.

Followup

Fully support decimal page-index pruning across INT32, INT64, BYTE_ARRAY, and FIXED_LEN_BYTE_ARRAY encodings, aligned with row-group statistics conversion.

Are these changes tested?

Added/existing tests.

[laskoviymishka]

Thanks for this — routing BYTE_ARRAY bounds through a type-aware decoder instead of an unconditional from_utf8().unwrap() kills a real panic on binary/non-UTF-8 page bounds, and the string and binary paths read correctly.

My one hold is the decimal branch. Parquet BYTE_ARRAY column-index bounds can be truncated by the writer and are only valid in lexicographic byte order, so decoding them as sign-extended i128 can reorder them and prune a page that actually matches — a silent dropped-row result, which is worse than the panic we're removing. And the row-group stats path in arrow/schema.rs still decodes BYTE_ARRAY decimal with a strict 16-byte try_into(), so for the minimum-length encoding a standard writer emits, the scan errors out before page pruning is ever reached — so the new decimal handling mostly can't run today. Since BYTE_ARRAY decimal isn't spec-conformant anyway (Java writes INT32/INT64/FIXED_LEN_BYTE_ARRAY), I'd lean toward returning unsupported here for now and landing decimal properly in the follow-up.

Once that's resolved, happy to take another pass and approve.

[laskoviymishka]

This decimal branch is the one piece I'd hold on, and I'd lean toward pulling it out of this PR.

Parquet column-index bounds for BYTE_ARRAY can be truncated by the writer, and truncated bounds are only valid in lexicographic byte order — decoding them as sign-extended i128 changes the ordering, so a truncated max can decode below the real max and we'd prune a page that actually matches. That's a silent dropped-row result, which is a worse failure mode than the panic this PR removes.

Separately, the row-group stats path in arrow/schema.rs still decodes BYTE_ARRAY decimal with a strict 16-byte try_into(), so for the minimum-length encoding a standard writer emits, the scan errors out at row-group level before page pruning is ever reached — so this branch mostly can't be exercised as-is.

Given BYTE_ARRAY decimal is non-spec anyway (Java writes INT32/INT64/FIXED_LEN_BYTE_ARRAY), I'd return unsupported here for now and land decimal properly in the follow-up, once the row-group path and truncation are settled. If we do want it in this PR, we'd need to guard truncated bounds and fix schema.rs in lockstep — and the test would need multi-byte sign-extension coverage, since it only exercises single-byte values today. wdyt?

[anoopj]

Good catch on the truncation scenario. Decoding truncated values is pretty bad. :) You are also right that the row group path is currently not consistent.

Now, instead of erroring, a BYTE_ARRAY column whose Iceberg type isn't string/binary falls back to no page pruning. Added eval_skips_pruning_for_byte_array_decimal_bounds to cover it.

[laskoviymishka]

The other unsupported-type arms (FIXED_LEN_BYTE_ARRAY, INT96) return FeatureUnsupported, but this returns DataInvalid — a BYTE_ARRAY holding an unexpected field type is an encoding limitation, not a corrupt file, and DataInvalid will mislead anyone reading errors or metrics. It also hard-errors and aborts the whole scan, where the conservative move when we can't interpret a bound is to fall back to select_all_rows() and just not prune. wdyt?

[anoopj]

Incorporated. The BYTE_ARRAY arm now checks the field type first: anything that isn't string or binary returns Ok(None), which falls through to select_all_rows(). So the decimal case just skips page pruning instead of erroring.

Good point on DataInvalid vs FeatureUnsupported. It's moot now since the gating moved up to the arm level.

The FIXED_LEN_BYTE_ARRAY (line 358) and INT96 (line 364) arms were already hard-erroring, which is why I'd matched them. Your reasoning applies to those too, so I converted all three to Ok(None).

It had a nice side effect: FIXED_LEN_BYTE_ARRAY backs fixed, uuid, and decimal(P>18), so with row selection on (off by default) a predicate on one of those over a file with a page index currently aborts the whole scan. Now it just skips pruning. Added eval_skips_pruning_for_fixed_len_byte_array for it.

Happy to split the FIXED_LEN_BYTE_ARRAY/INT96 change into its own PR if you'd rather keep this one to the panic fix.

[laskoviymishka]

The decimal hold from last round is resolved the way I hoped: the BYTE_ARRAY branch now skips pruning (Ok(None)) instead of decoding bounds it can't trust, and the FIXED_LEN_BYTE_ARRAY / INT96 hard-errors degrade the same way instead of aborting the scan. I traced the downstream path and the always-applied Arrow row filter means "skip" only forgoes an optimization, never drops rows. That's exactly the shape I was asking for, thanks.

One thing I'd still fix before merge: the String path is the single case where this PR adds real decoding, and it's the single case that still aborts the whole scan when a bound won't decode. An invalid-UTF-8 String bound returns DataInvalid and .transpose()? propagates it out through eval and the pipeline, failing the entire read — where BYTE_ARRAY-decimal, FIXED_LEN, and INT96 all fall back to skip. A writer that truncates a min/max stat mid-UTF-8-sequence would trip this on an otherwise conformant String column. I'd degrade it to Ok(None) like the others, and add an eval-level test for it — the current test only checks the helper's error kind, so it wouldn't catch the fix.

Separate and pre-existing, not something to hold this PR on: the INT32/INT64 decimal arms build their page bounds with Int/Long literals rather than Int128, so Datum::partial_cmp falls through to None and inequality/IN predicates silently over-prune on the most common decimal encoding. arrow/schema.rs already wraps these as Int128 at the row-group layer, so the two pruning layers disagree. It's outside this diff, but it directly undercuts the "fully support decimal page-index pruning" followup, so I'd file it as a companion issue rather than let the followup close over it.

A couple of smaller things inline — a tracing::debug! at the skip sites so operators can see when page pruning is dropped, and the "Unreachable" comment on the fallback arm (it's reachable — the tests call it directly).

Fix the String abort and this is good to land — happy to take another pass once it degrades like the rest.

[laskoviymishka]

This is the one path in the arm that still aborts the whole scan instead of degrading.

When field_type is String and a page bound isn't valid UTF-8, byte_array_bound_to_datum returns DataInvalid, and .transpose()? propagates it out through collect() → eval → the pipeline, failing the entire file read — not just page pruning for this column. That's the one case where we added real decoding, and it's the one case that doesn't fall back to Ok(None) like the BYTE_ARRAY-decimal / FIXED_LEN / INT96 skips right above it. A writer that truncates a min/max stat mid-UTF-8-sequence is a real class of bug, and it'd take down a read on an otherwise spec-conformant String column.

I'd restructure the arm so a decode error on either bound short-circuits to Ok(None) for the column rather than erroring through collect(). Once it degrades like the rest, happy to approve.

[anoopj]

Fixed. The BYTE_ARRAY arm is now a loop, and a decode error on either bound short-circuits the whole column, so an invalid-UTF-8 String bound degrades well. Added eval_skips_pruning_for_non_utf8_string_bound to verify this.

[laskoviymishka]

This asserts on byte_array_bound_to_datum directly, so it locks in the helper's error kind but never exercises what eval actually does with an invalid-UTF-8 String bound — which is where the abort behavior above lives.

I'd add a test that builds a col_string BYTE_ARRAY column index with a non-UTF-8 bound and asserts on eval's return. As it stands, if we fix the abort to skip, this test passes unchanged — so it isn't guarding the behavior that matters.

[anoopj]

Done. Replaced the helper-only assertion with eval_skips_pruning_for_non_utf8_string_bound. Dropped the old byte_array_bound_errors_on_invalid_utf8_string test since the arm now swallows that error.

[laskoviymishka]

While we're here — both new skip branches return Ok(None) silently, so a table with fixed/uuid/decimal(P>18) or INT96 columns permanently loses page-index pruning on every filtered scan with no signal to an operator. The absent-column-index path in row_filter.rs already emits a tracing::debug! for exactly this kind of skip.

A debug!/trace! here noting the column-index type and field id would keep it diagnosable. Not blocking.

[anoopj]

Added tracing::debug! with field_id/type at all three skip sites

[laskoviymishka]

Small thing on this comment — the _ arm is reachable. byte_array_bound_to_datum is a plain associated fn with no gating, and the test module calls it directly (that's how byte_array_bound_errors_on_invalid_utf8_string works). Keeping the Err fallback is right, but "Unreachable" could mislead someone into treating it as dead code. I'd reword to something like "Defensive: the production caller only invokes this for String/Binary."

[anoopj]

Reworded

[anoopj]

Thanks for the review @laskoviymishka. Good find on the existing issue with INT32/64 encoded decimals. I filed #3264 to track it.