andygrove opened a new issue, #6854:
URL: https://github.com/apache/datafusion-comet/issues/6854
### What is the problem the feature request solves?
Comet's native Iceberg scan never passes iceberg-rust a predicate on a
decimal column. `IcebergReflection.pageIndexUnsupportedColumns` lists every
`decimal`, `uuid`, `fixed` and `binary` column.
`CometIcebergNativeScan.icebergExprToProto` then drops any residual predicate
that references one, including the `IS NOT NULL` that Iceberg adds for every
filtered column.
Results stay correct. Iceberg's planning still prunes files on manifest
statistics, and the filter runs after the scan. But the native reader never
prunes row groups or pages on a decimal column. A selective decimal filter
therefore reads every row group and page of every file that survives planning.
#4982 added the gate because iceberg-rust, at the pins before #6649, could
not handle these columns in the page index:
- A `FIXED_LEN_BYTE_ARRAY` column index failed the scan with
`FeatureUnsupported`. That layout backs `uuid`, `fixed`, and decimals of
precision 19 and above.
- A `binary` page bound that was not UTF-8 panicked in
`String::from_utf8(..).unwrap()`.
- Pruning on an `INT32` or `INT64` decimal (precision 18 and below) could
drop pages that held matching rows (apache/iceberg-rust#3314).
None of these holds at the current pins:
- `main` has been at af1da4c since #6649, and that pin includes
apache/iceberg-rust#3314. It skips a `FIXED_LEN_BYTE_ARRAY` or `INT96` column
index instead of failing, and it decodes `binary` bounds as bytes.
- #6851 moves the pin to 1f3bc34, which adds page-index pruning for
`FIXED_LEN_BYTE_ARRAY` decimals (apache/iceberg-rust#3328).
- iceberg-rust's row-group statistics path decodes decimals of every
precision at both pins.
The Scaladoc on `pageIndexUnsupportedColumns` still describes the behavior
from before #6649.
### Describe the potential solution
Push decimal predicates through to iceberg-rust:
- `CometIcebergNativeScan.predicateLiteralToProto`:
- Build `IcebergLiteral.decimal_val` for a `DecimalType` column, at the
column's precision and scale.
- If a literal can't be represented exactly at that scale, drop that
predicate on its own. A literal that fails to bind would make iceberg-rust drop
the whole residual.
- `iceberg_literal_to_datum` in `planner.rs`: turn `DecimalVal` into a
`Datum` with `Datum::decimal_with_precision`. iceberg-rust's `Decimal` is now
`fastnum::D128`, so all 38 digits fit.
- `IcebergReflection.pageIndexUnsupportedColumns`: stop listing decimals.
- Update three comments that say decimal pushdown is deferred: the Scaladoc
on `pageIndexUnsupportedColumns`, the doc comment on
`iceberg_literal_to_datum`, and the comment on `IcebergDecimal` in
`operator.proto`.
What works at which pin:
- At `main`'s pin: page pruning for decimals of precision 18 and below, and
row-group pruning for every precision.
- Page pruning for wider decimals needs #6851.
The other gated types:
- `uuid` and `fixed` should stay in the list. iceberg-rust still skips their
page indexes, and a `uuid` predicate's string literal doesn't bind to a `uuid`
field.
- `binary` could come out too, but there is no binary predicate literal, so
only `IS [NOT] NULL` would gain anything.
Tests should compare against Spark with `checkIcebergNativeScan`. The data
should be decimal columns at precisions 9, 18 and 38, written in order over
many small pages (`write.parquet.page-row-limit`). Cover:
- values on both sides of zero;
- range, equality and `IN` predicates;
- a literal at a different scale from the column.
A pruned page that held a match loses rows silently, so these comparisons
are the main guard. A check that the scan actually prunes, for example on the
scan's `bytes_scanned` metric, would show the change has an effect.
### Additional context
Found while reviewing #6851. A probe on that branch wrote a sorted
`decimal(38, 2)` column over 200 pages, from -10100.00 to 10098.99, and ran
nine predicates through the native scan. All nine matched Spark. But they only
exercised the filter that runs after the scan, because the gate kept the
predicates away from iceberg-rust.
--
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]
