anoopj commented on code in PR #3247:
URL: https://github.com/apache/iceberg-rust/pull/3247#discussion_r4074984677
##########
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:
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.
##########
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:
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.
--
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]
