JingsongLi commented on PR #882: URL: https://github.com/apache/paimon-rust/pull/882#issuecomment-5831732468
Re-reviewed head `a1baa6eb` after the author reply. Requirement fit remains SUPPORTED, and the precision-zero SECOND arm now reconciles #884. All 20 focused `arrow::shredding::variant::tests` pass, as do formatting and diff checks. The two P1 production blockers from my previous review remain unresolved: 1. `try_typed_shred` still puts every microsecond timestamp into `typed_value` without checking whether the declared leaf can represent it. `timestamp_array` then floors a TIMESTAMP(0..3) value and `value` has no original fallback, so the full Variant roundtrip still loses the sub-second/sub-millisecond part. The same path still errors the entire write batch when a valid microsecond timestamp is outside the nanosecond leaf's i64 range. The new tests explicitly assert truncation at the array-conversion layer, but do not test that the full Variant value survives. Please fix representability before shredding and add a persisted-file roundtrip test. 2. The new “rewrite policy” is only a unit-test comment; there is still no usable inventory, rewrite, mixed-version rollout, verification, or rollback procedure for files written by earlier paimon-rust versions with explicit precision 0–3 or 7–9. The fixture confirms that old data is silently reinterpreted (1000× for the millis example). This remains a production migration blocker. Please publish an executable migration plan and cover precision-zero historical files too. The rebase/merge conflict concern is resolved. I am leaving the PR open for these fixes. -- 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]
