Repository navigation
Conversation
Identity partition pruning on `timestamp_ns` over-prunes matching files. Min/max pruning also returns empty results when Iceberg `timestamp` (`DateTime64(6)`) lower/upper bounds are stored as nanoseconds, which is the customer `toDateTime64(..., 6)` case. Co-authored-by: Cursor <cursoragent@cursor.com>
Identity partition values for `timestamp_ns` arrive as Avro Int64, and some writers store nanosecond min/max bytes on Iceberg `timestamp` (`DateTime64(6)`). Both were compared at the wrong scale and dropped matching files. Write `DateTime64(9)` as `timestamp_ns` so ClickHouse does not recreate that layout. Co-authored-by: Cursor <cursoragent@cursor.com>
`1e16` microseconds is ~year 2286, so spec-correct Spark `9999-12-31` and `DateTime64` max `2299-12-31` bounds were divided by 1000 and over-pruned. Convert only `|ticks| > 1e18` (true nanosecond band); fail open in between. Co-authored-by: Cursor <cursoragent@cursor.com>
`timestamp_ns` is v3-only. Mapping `DateTime64(9)` without checking `iceberg_format_version` wrote invalid v2 metadata (default is 2). Throw instead of emitting v3 types into v1/v2 tables. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 381b43d5e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (scale == 6 && magnitudeExceeds(unscaled, microseconds_ambiguous_threshold)) | ||
| { | ||
| if (!magnitudeExceeds(unscaled, nanoseconds_lower_threshold)) | ||
| return std::nullopt; | ||
|
|
||
| unscaled = nanosecondsToMicrosecondsForBound(unscaled, lower_bound); |
There was a problem hiding this comment.
Skip pruning instead of guessing nanosecond bounds
When an Iceberg timestamp manifest contains a magnitude above this threshold, the schema still identifies the value as microseconds; magnitude alone cannot prove that the writer actually used nanoseconds. Dividing the value and then trusting the resulting range allows mayBeTrueInRange to prune a file whose corrupt or differently encoded bounds do not cover its data, producing missing query results. Treat these suspicious bounds as unavailable and skip min/max pruning rather than taking a consequential fallback path.
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is not a P1. The overlapping band already fail-opens; converting |ticks| > 1e18 is the customer ns-stats case, not a guess inside the µs/ns overlap.
Iceberg timestamp is still microseconds in the schema. Magnitude cannot prove nanoseconds, which is why |ticks| in (1e16, 1e18] already returns nullopt. Callers then skip that hyperrectangle (if (!left || !right) continue) and min/max pruning for that column is skipped. That covers spec-correct far-future microseconds: Spark 9999-12-31 (~2.53e17 µs) and ClickHouse DateTime64 max 2299-12-31 (~1.04e16 µs).
Conversion runs only for |ticks| > 1e18. As microseconds that is ~year 33658, which is outside ClickHouse DateTime64(6) (max ~2299, ~1.04e16 µs) and outside normal Iceberg timestamp sentinels. As nanoseconds it is the customer layout (~1.76e18 for 2026) that previously over-pruned every file.
Fail-opening that band as well would keep query results correct (files scanned) but would disable min/max pruning again for those tables (IcebergMinMaxIndexPrunedFiles: 0). That undoes the point of this heuristic.
A corrupt 8-byte value > 1e18 that is not nanoseconds could still be converted into a fake in-range window. That is a residual, not a realistic P1 on this path.
…n_pruning_by_nanoseconds
…n_pruning_by_nanoseconds
|
While writing some tests, AI found a potential issue: Inserting into a format-version 3 table partitioned by
This is the insert in These cases on the same build succeed:
|
…n_pruning_by_nanoseconds
`getAvroType` required `timestamp-micros` after the ClickHouse#109764 backport, so identity-partitioned `timestamp_ns` writes failed. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@DimensionWieldr Thanks, must be fixed now |
|
CI looks good from my end. Regression tests for timestamp nanosecond pruning are passing on both Ice and Glue catalogs: https://github.com/Altinity/clickhouse-regression/blob/main/iceberg/tests/iceberg_engine/timestamp_ns_pruning.py |
AI Audit SummaryTwo follow-ups on
Iceberg does not keep the original zone name. Microsecond
ClickHouse reads the integers through the Iceberg schema, so a ClickHouse round-trip still succeeds. A reader that checks the Parquet logical type against the Iceberg type can reject the file. This flag was already forced true for every |
`getSimpleType` hardcoded UTC for nanosecond timestamptz while microsecond `timestamptz` already used the setting. Co-authored-by: Cursor <cursoragent@cursor.com>
Default Parquet still marks every `DateTime64` as `isAdjustedToUTC`. Iceberg writes set the flag from the column timezone so `timestamp_ns` matches `TIMESTAMP(NANOS, false)`. Co-authored-by: Cursor <cursoragent@cursor.com>
…n_pruning_by_nanoseconds Co-authored-by: Cursor <cursoragent@cursor.com>
|
Found one more thing: Identity timestamp_ns partition fields lose the Avro logical type Impact: Manifests for tables partitioned by DateTime64(9) store that partition key as a bare Avro Anchor: Utils.cpp / getAvroType; contrib/avro/lang/c++/impl/Compiler.cc / makeLogicalType; IcebergWrites.cpp / extendSchemaForPartitions Trigger: INSERT into an Iceberg v3 table partitioned by DateTime64(9) or DateTime64(9, 'UTC'). Why defect: getAvroType emits logicalType: timestamp-nanos, then compileJsonSchemaFromString maps any unknown logical type to LogicalType::NONE. DataFileWriter persists that compiled schema, which omits the annotation. The comment in getAvroType says other engines still see it. Fix direction (short): Keep the original schema JSON in the manifest (or teach the bundled Avro writer a timestamp-nanos logical type) so the written header still contains it. |
contrib Avro `toJson` drops unknown logical types, so DataFileWriter was writing a bare `long`. Persist the original JSON in `avro.schema` instead. Co-authored-by: Cursor <cursoragent@cursor.com>
…n_pruning_by_nanoseconds
|
Debug build is failing. The build is compiling unit tests, and two of them still call |
|
Also another possible issue: Nanosecond bounds near the epoch are read as microseconds and can drop the whole file Impact: A query can skip a data file that contains matching rows. If the file’s lower bound falls in a short window after the epoch and its upper bound is a normal nanosecond timestamp, the decoded range is inverted and every row in that file is pruned. Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergFieldParseHelpers.cpp / deserializeDateTime64FromBinaryRepr Trigger: Iceberg timestamp (DateTime64(6)) whose 8-byte min/max are nanoseconds, with the lower bound in about 1970-01-21..1970-04-26 and the upper bound after about 2001-09-09 (the same non-spec layout this PR fixes for 2024+ bounds). Why defect: Only |ticks| > 1e18 is divided by 1000. |ticks| <= 1e16 is kept as microseconds (~115 days). A 2024 upper bound becomes ~1.7e15 µs while a lower bound of ~1e16 ns stays 1e16 µs, so lower > upper. Range::intersectsRange then matches nothing. Fix direction (short): If either bound is in the nanosecond band, convert both, or fail open when the decoded lower bound is greater than the upper bound. |
`getIcebergType` now requires `format_version`; the two `Nothing` tests still used two arguments and failed debug builds. Co-authored-by: Cursor <cursoragent@cursor.com>
Mixed-scale `timestamp` stats can decode a near-epoch ns min as microseconds and a later ns max into a smaller value. An inverted range pruned the whole file; skip that column's hyperrectangle instead. Co-authored-by: Cursor <cursoragent@cursor.com>
Funny bug. |
…n_pruning_by_nanoseconds
|
More issues: Manifest rewrite throws when a DateTime64(9) partition value is read back as Int64 Impact: OPTIMIZE manifest compaction of a table partitioned by DateTime64(9) / timestamp_ns fails with BAD_GET. The metadata commit has not run yet, so the table is left unchanged and the operation does not complete. Anchor: getDateTime64ValueForIcebergBounds in src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp, called from manifest-list partition summaries during compaction Trigger: Format version 3 table, identity (or any transform whose result type is DateTime64(9)) partition, then manifest rewrite. contrib Avro has no timestamp-nanos logical type, so the manifest value comes back as Int64. Inserts still work because ChunkPartitioner produces a Decimal64. Why defect: canDumpIcebergStats accepts the DateTime64 type, then safeGet() rejects the Int64 ticks. The manifest entry writer already accepts Int64; the summary path does not. Fix direction (short): In getDateTime64ValueForIcebergBounds, treat Int64/UInt64 as already-scaled ticks, same as the decimal payload. Nanosecond min/max stats within about 115 days of the epoch are decoded as microseconds and can drop the file Impact: A query can skip a data file that contains matching rows when Iceberg timestamp (DateTime64(6)) lower/upper bounds were stored as nanoseconds. Results are missing, not just under-pruned. Anchor: deserializeDateTime64FromBinaryRepr in src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergFieldParseHelpers.cpp, used by manifest min/max pruning Trigger: Both bounds are nanosecond counts with absolute value <= 1e16 (about 1970-01-01 through 1970-04-26, and the matching pre-epoch window), and the predicate hits the real time. Example: one day after the epoch is 8.64e13 ns, which is kept as 8.64e13 µs (about year 1997). The inverted-range check does not fire when both ends stay in that band. Why defect: The new decoder only divides by 1000 when |ticks| > 1e18. At or below 1e16 it treats the bytes as microseconds, so the hyperrectangle misses the real instants and mayBeTrueInRange prunes the file. Fix direction (short): Fail open (skip this column’s min/max) for any DateTime64(6) bound that is not a proven microsecond value, or only convert when both bounds sit in the nanosecond band and still form a valid range. |
…rite contrib Avro has no `timestamp-nanos`, so compaction reads identity partition keys as `Int64`. `getDateTime64ValueForIcebergBounds` required `Decimal64` and threw `BAD_GET` before the snapshot committed. Co-authored-by: Cursor <cursoragent@cursor.com>
`|ticks| <= 1e16` was kept as microseconds, so a 1-day-after-epoch ns min/max decoded as ~1972 and over-pruned. Skip that column's hyperrectangle below 2024-01-01 UTC so 2024+ pruning stays intact. Co-authored-by: Cursor <cursoragent@cursor.com>
`magnitudeExceeds` is strict, so `|ticks| == 2024-01-01 UTC` fail-opened while the comment said that instant is kept. Fail-open only below the threshold. Co-authored-by: Cursor <cursoragent@cursor.com>
|
One more! Nanosecond min/max on Iceberg timestamp still over-prunes a band around the epoch Impact: A query can drop a data file and return fewer rows. Same failure this PR is fixing, for bounds whose integer magnitude also looks like a 2024–2286 microsecond timestamp. Anchor: deserializeDateTime64FromBinaryRepr in src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergFieldParseHelpers.cpp Trigger: Iceberg timestamp (DateTime64(6)) lower/upper bounds stored as nanoseconds with |ticks| in [1.704e15, 1e16]. That is about 1970-01-20 through 1970-04-26, and the negative mirror about 1969-09-07 through 1969-12-12. Both ends decode, the range stays ordered, and a predicate on the real instant does not overlap the decoded 2024–2286 range. Why defect: Those ticks are kept as microseconds. 1e16 ns is ~115 days, and the same integer as microseconds is ~year 2286. Values below 2024-01-01 microseconds and the (1e16, 1e18] band fail open; this band does not. src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergFieldParseHelpers.cpp lines 89-101 Fix direction (short): Fail open [min_kept_microseconds, 1e16] as well, or otherwise refuse to prune when a timestamp bound is in that overlap. No integer test separates those two scales. |
`[1.704e15, 1e16]` is both ~19.7–115 days as nanoseconds and 2024–2286 as microseconds. Convert only `|ticks| > 1e18`; skip min/max pruning otherwise because no integer test splits those scales. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
CI is greeeeen 😭 AI audit (Grok 4.7) returned no more defects. All of LGTM |
|
Port on |
Solved #2240
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix partition and min/max pruning for Iceberg tables by
timestamp_nstype, fix writingDateTime(9)astimestamp_ns.Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: