laskoviymishka commented on code in PR #3247:
URL: https://github.com/apache/iceberg-rust/pull/3247#discussion_r4066665902
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -379,6 +372,40 @@ impl<'a> PageIndexEvaluator<'a> {
Ok(Some(result?))
}
+ /// Converts a `BYTE_ARRAY` page bound into a [`Datum`] according to the
+ /// field's primitive type. Parquet stores Iceberg `string` and `binary`
+ /// bounds as `BYTE_ARRAY`, along with `decimal` bounds from non-standard
+ /// writers (the spec maps `decimal(P > 18)` to `fixed_len_byte_array`).
+ fn byte_array_bound_to_datum(field_type: &PrimitiveType, bytes: &[u8]) ->
Result<Datum> {
+ match field_type {
+ PrimitiveType::String => {
+ let value = std::str::from_utf8(bytes).map_err(|err| {
+ Error::new(ErrorKind::DataInvalid, "Invalid UTF-8 in
string page bound")
+ .with_source(err)
+ })?;
+ Ok(Datum::string(value))
+ }
+ PrimitiveType::Binary => Ok(Datum::binary(bytes.to_vec())),
+ PrimitiveType::Decimal { .. } => {
Review Comment:
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?
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -379,6 +372,40 @@ impl<'a> PageIndexEvaluator<'a> {
Ok(Some(result?))
}
+ /// Converts a `BYTE_ARRAY` page bound into a [`Datum`] according to the
+ /// field's primitive type. Parquet stores Iceberg `string` and `binary`
+ /// bounds as `BYTE_ARRAY`, along with `decimal` bounds from non-standard
+ /// writers (the spec maps `decimal(P > 18)` to `fixed_len_byte_array`).
+ fn byte_array_bound_to_datum(field_type: &PrimitiveType, bytes: &[u8]) ->
Result<Datum> {
+ match field_type {
+ PrimitiveType::String => {
+ let value = std::str::from_utf8(bytes).map_err(|err| {
+ Error::new(ErrorKind::DataInvalid, "Invalid UTF-8 in
string page bound")
+ .with_source(err)
+ })?;
+ Ok(Datum::string(value))
+ }
+ PrimitiveType::Binary => Ok(Datum::binary(bytes.to_vec())),
+ PrimitiveType::Decimal { .. } => {
+ // BYTE_ARRAY decimals are variable-length signed big-endian.
+ let unscaled = i128_from_be_bytes(bytes).ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!("Invalid decimal page bound: too many bytes (>
16): {bytes:?}"),
+ )
+ })?;
+ Ok(Datum::new(
+ field_type.clone(),
+ PrimitiveLiteral::Int128(unscaled),
+ ))
+ }
+ _ => Err(Error::new(
+ ErrorKind::DataInvalid,
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
