Skip to content

fix(variant): convert shredded timestamps to the column's time unit - #882

Open
jackylee-ch wants to merge 2 commits into
apache:mainfrom
jackylee-ch:fix/variant-shredded-timestamp-unit
Open

jackylee-ch wants to merge 2 commits into
apache:mainfrom
jackylee-ch:fix/variant-shredded-timestamp-unit

Conversation

@jackylee-ch

Copy link
Copy Markdown
Contributor

ShreddedValue::Timestamp is always micros, but the shredded leaf's Arrow unit follows the declared precision (MILLIS <4, MICROS 4..=6, NANOS >6). Both sides passed the integer through unchanged, so a TIMESTAMP leaf outside 4..=6 was 1000x off. A NANOS leaf is spec-conforming, so foreign files were mis-read too.

Write now scales micros to the leaf unit, flooring into MILLIS, and errors on micros beyond i64 nanoseconds (~1677..2262) instead of wrapping. Read keys off the array's TimeUnit, so a foreign file decodes from its own schema.

Disclosure: files paimon-rust already wrote outside 4..=6 hold micros under a MILLIS/NANOS annotation. They read back correctly only because the reader repeated the mistake; after this change they decode 1000x wrong, and nothing in the file tells them apart. Only an explicit variant.shreddingSchema produces such a leaf — inference picks precision 6.

ShreddedValue::Timestamp always carries microseconds, but timestamp_array
wrote that integer into an Arrow array whose unit follows the declared
precision, and timestamp_value_at read it back unconverted. Outside
precision 4..=6 both sides were off by 1000x.

Precision 7..=9 maps to a NANOS leaf, one of the four timestamp forms
parquet-format's shredded-value table permits, so a conforming file
written by another engine was also mis-read.

Convert on both sides instead of reinterpreting: the write scales micros
to the leaf unit, rejecting a micros value that cannot be expressed in
i64 nanoseconds, and the read keys off the array's own TimeUnit so a
foreign file decodes correctly regardless of the local option.
@JingsongLi

JingsongLi commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Reviewed head b75b8b9. Requirement fit: SUPPORTED. The seven focused shredded-timestamp tests pass, but two follow-ups remain. Integration/rebase follow-up: main commit d05cb03 (#884) added TimeUnit::Second for precision-zero timestamps after this branch diverged. This PR still has the earlier 0..=3 millisecond arm and no Seconds reader arm. GitHub marks the PR CONFLICTING, so this is not a demonstrated regression in a merged result; please reconcile the two changes on rebase, preserve Seconds↔micros conversion for TIMESTAMP(0) and TIMESTAMP_LTZ(0), and add precision-zero read/write tests. Persisted-file compatibility: as the PR body discloses, older Rust writes with explicit precision 1–3 or 7–9 shredding schemas stored micros under millis/nanos annotations. The new reader would silently reinterpret those typed values, and the files carry no marker to distinguish them from conforming files. Please make the migration/compatibility policy explicit (for example, a deliberate legacy mode or required rewrite) and include an old-writer fixture so the impact is testable.

… precision-zero tests and old-writer compat fixture
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants