mbutrovich commented on PR #4982: URL: https://github.com/apache/datafusion-comet/pull/4982#issuecomment-5040136582
Thanks for the careful read, @andygrove. All four addressed in the latest commit. **Stale `planner.rs` comment.** Fixed. Removed the section banner describing the old Spark-Catalyst round-trip and the leftover one-line docstring, so only the accurate multiline docstring remains. **Byte/Short pushdown.** Restored. `predicateLiteralToProto` now widens `ByteType`/`ShortType` to `int_val`, matching the old `Datum::int` path and how iceberg-rust stores these (it has no narrower integer type, so byte/short map to Iceberg `int`). **Nested-column gate.** I verified the concern does not reach the gate. Spark does emit nested references as dotted paths, but Comet reads the residual's column name (`CometIcebergNativeScan.scala`, `icebergExprToProto`) and looks it up in `attributeMap`, which is keyed by top-level output attribute name. A dotted `struct.field` never matches, so a nested residual is dropped there, one step before `pageIndexUnsupportedColumns` is consulted. So the top-level-only gate is sound. I added a comment documenting that invariant, and the new fuzz test below locks it in. **Binary.** This one turned out to be the opposite of a missed optimization. iceberg-rust's page-index evaluator accepts `BYTE_ARRAY` but decodes the column-index min/max as UTF-8 (`String::from_utf8(..).unwrap()` in `page_index_evaluator.rs`), and that decode runs in `calc_row_selection` before the predicate closure, so even a bare `IS NOT NULL` triggers it. Binary was not in the gate, so a filtered binary column already pushed its implicit `IS NOT NULL` and panicked the native scan on non-UTF-8 bounds. I confirmed it with a failing test (a `0xFF`-prefixed binary column, `WHERE b IS NOT NULL`), then added `binary` to `pageIndexUnsupportedColumns`, which turns it green. So rather than adding binary literal pushdown, the fix gates binary alongside the FIXED_LEN_BYTE_ARRAY types. **Fuzz coverage.** Good catch that the existing test cannot see pushdown. I added `filter pushdown - residual pool reflects whether the column type is pushable`, which runs an equality per column and asserts the serialized residual pool is non-empty exactly when the type is pushable and empty for a gated type. The fuzz table is unpartitioned, so the residual is the full filter, which makes that a real signal. It anchors both the byte/short fix and the binary gate. -- 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]
