Repository navigation
fix(writer): write a nullable timestamp with nulls as datetimeparts - #635
Merged
Merged
Conversation
Rust's split_temporal keeps a nullable timestamp's validity on the days child, so a column holding nulls is still vortex.datetimeparts. The Java encoder declined any NullableData, so a chunk with a null fell back to vortex.ext over a masked i64, and since #627 made the Parquet import hand nullable timestamps over as NullableData, one null in a repartitioned chunk switched it. DateTimePartsEncodingEncoder now accepts NullableData over long[] storage: the validity goes on the days child (seconds and subseconds stay non-nullable), an all-valid validity is dropped as Rust's AllValid writes no bitmap, an all-null chunk is left to vortex.ext's null constant, and the min/max bounds skip the invalid slots' placeholders. The days child takes the column's nullability whatever shape the storage arrives in, as Rust's does (same bytes, a nullable dtype). The zone-stats table of every timestamp column, nullable or not, now goes datetimeparts too: ZoneAccumulator hands its min/max columns over as NullableData, and Rust's TemporalScheme always splits timestamps. TimestampFilterInteropIntegrationTest checked the whole file for the encoding and now checks the data column it meant. The full encoding trees still differ from Rust in the days child's cascade and in sequence vs datetimeparts, with or without nulls (#634). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fRbYryTxfcu5j2CXwkSzd
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #627 (review finding on #630).
Rust's
split_temporalkeeps a nullable timestamp's validity on thedayschild, so a column holding nulls is stillvortex.datetimeparts.DateTimePartsEncodingEncoderdeclined anyNullableData, so a chunk with a null fell back tovortex.extover a masked i64, and since #627 one null in a repartitioned Parquet-import chunk switched it.Change (
DateTimePartsEncodingEncoder)NullableDataoverlong[]timestamp storage: validity on the days child, seconds/subseconds non-nullableAllValidwrites no bitmap); an all-null chunk is left tovortex.ext's null constantSide effect: every timestamp column's zone-stats table now goes datetimeparts too (
ZoneAccumulatorhands min/max asNullableData; Rust'sTemporalSchemealways splits timestamps).TimestampFilterInteropIntegrationTest#timestampColumnsAreStoredAsPartsAndAsFlatLongschecked the whole file and now checks the data column it meant.Tests
DateTimePartsEncodingEncoderTest$Nullable: validity placement, bounds over valid rows, all-valid, all-null.NullableTimestampInteropIntegrationTest: against vortex-jni, the data column's root encoding and validity placement match for four null shapes and the Parquet import path; the cases with nulls fail without the change.The full encoding trees still differ from Rust in the days child's cascade (RLE vs FoR) and sequence vs datetimeparts, with or without nulls: #634.
Reviewed independently (vortex-reviewer agent): approved; its should-fix items (test naming, imports, a test that didn't check validity, stale comments) are addressed in this commit.
🤖 Generated with Claude Code
https://claude.ai/code/session_011fRbYryTxfcu5j2CXwkSzd