leaves12138 commented on code in PR #884:
URL: https://github.com/apache/paimon-rust/pull/884#discussion_r4056310889
##########
crates/paimon/src/arrow/residual.rs:
##########
@@ -1114,7 +1114,15 @@ fn timestamp_scalar(
timezone: Option<&'static str>,
) -> crate::Result<Option<ArrayRef>> {
let array: ArrayRef = match precision {
- 0..=3 => {
+ 0 => {
+ let value = millis.div_euclid(1_000);
+ let array = TimestampSecondArray::new_scalar(value).into_inner();
Review Comment:
[P1] Match the decoded Parquet column's timestamp unit before comparison
Existing TIMESTAMP(0) Parquet files are decoded as `Timestamp(Millisecond)`.
The single-leaf fast path in `build_parquet_arrow_predicate` passes that
physical array directly to `evaluate_exact_leaf_predicate`, but this change now
constructs its comparison literal as `Timestamp(Second)`. Unlike the general
residual path, this decoder fast path does not normalize the array first.
I reproduced an equality filter on a millisecond-encoded Parquet file
containing `[-1000, 2000, 3000]`, with logical field type `TIMESTAMP(0)` and
literal `Datum::Timestamp { millis: 2000, nanos: 0 }`. The read now fails with:
```text
Invalid comparison operation: Timestamp(ms) == Timestamp(s)
```
The identical test passes on the pre-PR base. Please reconcile the physical
column and literal units in the decoder predicate path (without losing literal
precision), and cover filtering existing millisecond-encoded
TIMESTAMP(0)/TIMESTAMP_LTZ(0) Parquet files. Changing the returned Arrow schema
should not make existing files unreadable under ordinary predicates.
##########
crates/paimon/src/arrow/residual.rs:
##########
@@ -1114,7 +1114,15 @@ fn timestamp_scalar(
timezone: Option<&'static str>,
) -> crate::Result<Option<ArrayRef>> {
let array: ArrayRef = match precision {
- 0..=3 => {
+ 0 => {
+ let value = millis.div_euclid(1_000);
Review Comment:
[P2] Preserve subsecond precision in predicate literals
The column's precision does not mean that a comparison literal can be
rounded down to the same precision. `PredicateBuilder` accepts a timestamp
literal with `millis: 1500` for a TIMESTAMP(0) field. This division changes
that literal from 1.5 seconds to 1 second before an exact comparison.
For a valid TIMESTAMP(0) column containing `[1s, 2s]`, I reproduced both
incorrect masks through `evaluate_predicates_mask`:
- `ts = 1.5s`: actual `[true, false]`, expected `[false, false]`.
- `ts >= 1.5s`: actual `[true, true]`, expected `[false, true]`.
Both comparisons are correct on the pre-PR millisecond Arrow representation.
This is independent of the Parquet physical-unit issue: it also affects
residual evaluation on correctly constructed second-based Arrow arrays. Please
compare the column and literal in a common lossless unit, or use exact
timestamp-value comparison, rather than truncating the literal. A regression
test should exercise a fractional-second literal against whole-second stored
values.
##########
crates/paimon/src/arrow/mod.rs:
##########
@@ -149,7 +149,8 @@ pub fn paimon_type_to_arrow(dt: &PaimonDataType) ->
crate::Result<ArrowDataType>
fn timestamp_time_unit(precision: u32) -> crate::Result<TimeUnit> {
match precision {
- 0..=3 => Ok(TimeUnit::Millisecond),
+ 0 => Ok(TimeUnit::Second),
Review Comment:
[P1] Keep the Parquet physical timestamp representation in milliseconds
This mapping also reaches the write path (`TableWrite::new` ->
`build_target_arrow_schema` -> `ParquetFormatWriter`), not just the public read
schema. Parquet has no seconds timestamp logical type. With the current Arrow
writer, a `Timestamp(Second)` column is written as an unannotated INT64
containing seconds, whereas Paimon's precision <= 3 Parquet statistics/pruning
code still interprets the physical INT64 values as milliseconds.
I reproduced this using `ParquetFormatWriter` with a `TIMESTAMP(0)` field
and values `[-1s, 2s, 3s]`, followed by `ParquetFormatReader` with
`PredicateBuilder::equal("ts", Datum::Timestamp { millis: 2000, nanos: 0 })`.
An unfiltered read returns all three rows, but the filtered read silently
returns **zero rows instead of one**. The new file has no Parquet timestamp
annotation and physical min/max `[-1, 3]`, so statistics pruning treats 2000 ms
as outside the file. On the pre-PR base the same test writes `TIMESTAMP_MILLIS`
with min/max `[-1000, 3000]` and returns the matching row.
Could we keep the Arrow seconds representation at the API boundary but
normalize the Parquet writer's schema and arrays to millisecond timestamps?
Please add a TIMESTAMP(0)/TIMESTAMP_LTZ(0) Parquet write/read/filter round-trip
test, including the physical footer representation, so this does not change
Paimon's on-disk timestamp encoding.
--
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]
