mbutrovich commented on code in PR #91: URL: https://github.com/apache/datafusion-iceberg/pull/91#discussion_r4419564938
########## AGENTS.md: ########## @@ -73,7 +73,7 @@ Tests are self-contained (in-memory catalogs, temp dirs, checked-in metadata JSO `IcebergTableScan` converts the projection to column names and the filters to a single Iceberg `Predicate` (`physical_plan/expr_to_predicate.rs`). It applies `limit` in-stream, has one output partition, and delegates the actual read to iceberg's `TableScan::to_arrow()`. -Both providers report every filter as `Inexact`, so DataFusion re-applies the original filters after the scan. The pushed-down predicate therefore only has to be *implied by* the original filter. It may match extra rows but must never drop a row that matches. Unconvertible parts are dropped: an `AND` keeps whichever side converted, while an `OR` needs both sides. Preserve this soundness rule when extending `expr_to_predicate.rs`; for example, it is why date casts and some NaN arithmetic are deliberately not pushed down. +Both providers report every filter as `Inexact`, so DataFusion re-applies the original filters after the scan. The pushed-down predicate therefore only has to be *implied by* the original filter. It may match extra rows but must never drop a row that matches. Unconvertible parts are dropped: an `AND` keeps whichever side converted, while an `OR` needs both sides. Preserve this soundness rule when extending `expr_to_predicate.rs`; for example, it is why casts and some NaN arithmetic are deliberately not pushed down. DataFusion's simplifier already unwraps the casts that can be removed without changing results, so any cast that reaches the scan is one that can't. Review Comment: Could we reword the last sentence? This PR's own sqllogictest shows a removable cast reaching the scan: `CAST(d AS Timestamp(ns)) < TimestampNanosecond(...)` can be rewritten exactly on the date column, but DataFusion doesn't do that yet (https://github.com/apache/datafusion/issues/26289). An agent reading "any cast that reaches the scan is one that can't" could take it as a fact to rely on. ```suggestion Both providers report every filter as `Inexact`, so DataFusion re-applies the original filters after the scan. The pushed-down predicate therefore only has to be *implied by* the original filter. It may match extra rows but must never drop a row that matches. Unconvertible parts are dropped: an `AND` keeps whichever side converted, while an `OR` needs both sides. Preserve this soundness rule when extending `expr_to_predicate.rs`; for example, it is why casts and some NaN arithmetic are deliberately not pushed down. Removing a cast is left to DataFusion's simplifier (`unwrap_cast_in_comparison`), which rewrites a comparison only when the result doesn't change. ``` -- 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]
