Re: [PR] feat(datafusion): add push down temporal literals [paimon-rust]

["JingsongLi (via GitHub)" <gi...@apache.org>]

JingsongLi commented on PR #430:
URL: https://github.com/apache/paimon-rust/pull/430#issuecomment-4889320460

   I think temporal literal pushdown has a correctness risk for negative sub-millisecond timestamps.
   
   `filter_pushdown.rs::scalar_to_timestamp_parts` converts microsecond/nanosecond timestamp literals with `div_euclid` / `rem_euclid`. But the current Arrow-to-BinaryRow write path uses truncating division and remainder, e.g. in `spec/binary_row.rs`:
   
   - `millis = micros / 1000`
   - `nanos = (micros % 1000) * 1000`
   
   For negative timestamps this produces different `(millis, nanos)` pairs. For example, `-1us` becomes `(-1ms, 999000ns)` in the pushdown literal path, but the write path represents it as `(0ms, -1000ns)`. This can make pushed timestamp predicates false-negative for pre-epoch timestamps with fractional milliseconds.
   
   Could we either:
   1. align the BinaryRow timestamp write/extract path with the euclidean representation, or
   2. make pushdown use the same representation as the existing write path until the core timestamp representation is changed?
   
   It would also be good to add an integration test that writes a negative microsecond/nanosecond timestamp and verifies a pushed equality/range predicate through DataFusion scan.
   


-- 
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: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org