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>
`getAvroType` required `timestamp-micros` after the ClickHouse#109764 backport, so identity-partitioned `timestamp_ns` writes failed. Co-authored-by: Cursor <cursoragent@cursor.com>
`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>
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>
`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>
…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>
`[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>
The engine reference still said every `DateTime64` maps to `timestamp` and that `timestamp_ns` partitions cannot be written. 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. |
ianton-ru
added
antalya
port-antalya
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da866b680a
ℹ️ 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".
| auto date_time64 = std::static_pointer_cast<const DataTypeDateTime64>(type); | ||
| if (date_time64->hasExplicitTimeZone()) | ||
| return {Iceberg::f_timestamptz_ns, true}; | ||
| return {Iceberg::f_timestamp_ns, true}; |
There was a problem hiding this comment.
When a v3 Iceberg table uses format=Avro, these returns advertise the column as timestamp_ns or timestamptz_ns, but AvroRowOutputFormat.cpp:214-219 serializes every scale-9 DateTime64 through the decimal fallback rather than as the long required by the Iceberg timestamp schema. An insert therefore produces a data file whose physical schema disagrees with the table metadata, so external Iceberg readers can reject or misdecode it; either implement the nanosecond timestamp encoding in the Avro writer or reject this type for Avro-backed Iceberg tables.
Useful? React with 👍 / 👎.
| | `fixed(N)` | `FixedString(N)` | | | ||
| | `decimal(P, S)` | `Decimal(P, S)` | | | ||
|
|
||
| Writes use the inverse mapping: `DateTime64(9)` is stored as `timestamp_ns`, and `DateTime64` with an explicit timezone and scale 9 as `timestamptz_ns`, but only when `iceberg_format_version` is 3 or higher. `DateTime` and `DateTime64` with scale 6 or less are stored as `timestamp`. Creating or altering an Iceberg table with `DateTime64` scale 9 on format version 1 or 2 raises an exception. |
There was a problem hiding this comment.
This paragraph is inside the page's AUTOGENERATED_START/AUTOGENERATED_END region, so it must not be changed directly. The corresponding source registration is already updated in registerStorageObjectStorage.cpp; keep that source change and let the dedicated documentation autogeneration workflow update this .mdx file.
AGENTS.md reference: docs/reference/AGENTS.md:L1-L3
Useful? React with 👍 / 👎.
ianton-ru
added
the
forwardport
Scale-9 `DateTime64` was serialized as decimal `bytes`, so an Avro data file disagreed with `timestamp_ns` table metadata. Co-authored-by: Cursor <cursoragent@cursor.com>
The `DateTime64(9)` write mapping stays in the engine registration. The autogenerated docs page is updated by the documentation workflow. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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 triage for this PR (head
|
This comment was marked as outdated.
This comment was marked as outdated.
|
@blau-ai fix the PR-caused failing (and flaky) tests |
getIdentityPartitionColumnValues passed the raw Avro `long` nanosecond ticks of a timestamp_ns / timestamptz_ns identity partition through convertFieldToTypeOrThrow, which treats an integer target of DateTime64 as a whole number of seconds and overflows while rescaling (DECIMAL_OVERFLOW on any read of such a table). Wrap the ticks into a DecimalField at the column scale directly, mirroring the existing pruning path (wrapIntegerPartitionValuesAsDateTime64). 05385_iceberg_avro_timestamp_ns created and read back a format-version-3 table; reading it logs a benign 'parsed using v2 specification' warning (v3 metadata has no top-level 'schema' field for the v1 parser), which lands on stderr and clickhouse-test turns into a failure. Suppress server logs in the test, as the other Iceberg .sh tests do. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
`deserializeDateTime64FromBinaryRepr` dropped every scale-6 bound with `|ticks| <= 1e18`, so ordinary Iceberg `timestamp` (`DateTime64(6)`) columns lost min/max pruning (`test_minmax_pruning`, `test_writes_statistics_by_minmax_pruning`). Read `|ticks| <= 1e16` as microseconds again, as the spec requires; keep failing open in `(1e16, 1e18]` and converting nanoseconds above `1e18`. Drop the near-epoch and overlap-band nanosecond-on-`timestamp` tests: those bounds are indistinguishable from spec-correct microseconds. #2610 (comment)
`DateTime64(9)` maps to `timestamp_ns`, which requires format version 3, so `CREATE TABLE` at the default version 2 threw and broke the batched setup. The arm now round-trips full nanoseconds. #2610 (comment)
v3 keeps the `schemas` array and has no top-level `schema`. Trying the v1 parser first logged a false spec-violation warning on every ClickHouse-written v3 read, including `timestamp_ns`. Co-authored-by: Cursor <cursoragent@cursor.com>
…n-pruning-by-nanoseconds
CI triage for
|
| Check | Class | Caused by PR? |
|---|---|---|
| DCO | process gate | Yes (missing sign-offs) |
Stateless (amd_debug, parallel) — 04890_iceberg_metadata_spec_violation_no_abort |
flaky | No (pre-existing) |
Stateless (amd_debug, distributed plan, s3) — 03572_export_merge_tree_part_limits_and_table_functions |
flaky/transient | No |
| Parser memory check | infra | No |
| Grype Scan keeper / server (-alpine) | container CVE scan | No |
| Regression — OAuth (9 scenarios) | flaky/infra | No |
| Regression — Swarms (3 scenarios) | flaky/infra | No |
DCO — PR-related (process gate)
None of the 23 commits on the branch carry a Signed-off-by: trailer, so the DCO check fails. This is the one red that is genuinely about this PR, but it's a policy gate, not a code defect.
Fix: re-apply sign-offs, e.g. git rebase --signoff origin/antalya-26.8 && git push --force-with-lease (I can't force-push for you), or merge with a squash commit that adds the trailer. If Altinity waives DCO on internal PRs, a maintainer can simply override it.
Stateless (amd_debug, parallel) — 04890...spec_violation_no_abort
-primitive to complex rejected
+NOT REJECTED
CI's own randomized-settings diagnosis: "Runs: 10, Failed: 3, Passed: 7 … Failure is flaky (3/10 reruns failed)." Only arm 4 (an old primitive field turning into a struct) intermittently fails to be rejected — a metadata-refresh/cache timing race, not a deterministic result.
Evidence it's not this PR: the test file is unchanged by the PR and its commit (0a6024c2092) is already an ancestor of the base branch antalya-26.8. The PR's only schema-path changes are (a) parseTableSchema using format_version >= 2 instead of == 2 — identical behavior for the v2 metadata this test writes — and (b) the timestamptz_ns timezone in getSimpleType. Neither touches the primitive→complex schema-evolution validator that arm 4 exercises.
Next step: safe to re-run. (Worth a maintainer noting it's a pre-existing flake in the Iceberg area, but it does not block this PR.)
Stateless (amd_debug, distributed plan, s3 storage, parallel) — 03572_export_merge_tree_part...
---- Count files in big_destination_max_bytes, should be 5 (4 parquet, 1 commit)
-5
+4
CI diagnosis: "Runs: 7, Failed: 0, Passed: 7 … All reruns passed. The failure is not reproducible (likely a transient issue)." A race on export-completion flushing the last file. This is MergeTree→S3 partition_strategy='hive' export — unrelated to Iceberg timestamp pruning.
Next step: safe to re-run.
Parser memory check
ERROR Resolve master binary No master `clickhouse-examples` artifact was found
Infra: the job couldn't fetch the master baseline binary to diff against, so it errored before measuring anything. Not related to the PR content.
Next step: re-run; nothing to fix in the PR.
Grype Scan (keeper: 4 high/critical, server -alpine: 1 high/critical)
Container-image vulnerability scans of the built Docker images (OS/base-image CVEs). A C++ Iceberg code change cannot introduce these, and these gates are red on essentially every PR against this base. Pre-existing / infra.
Next step: handled by base-image/scan-policy maintenance, not this PR.
Regression — OAuth (112 scenarios: 98 ok, 9 failed) & Swarms (3 failed)
TestFlows suites for external OAuth/identity-provider authentication (oauth/authentication, oauth/tokens, oauth/identity, oauth/tls, …) and swarm cluster discovery / node-failure injection (swarms/feature/node, cluster_discovery). Neither suite exercises Iceberg metadata, partition pruning, or the Parquet/Avro timestamp writers changed here. The OAuth run alone logged 1814 retries (298 failed) — the churn signature of an infra/provider-dependent flaky suite. Structurally impossible for this PR's diff to cause.
Next step: safe to re-run / known-flaky; not a blocker for this PR.
Bottom line: merge-blocking items are limited to the DCO sign-off gate (easy fix or maintainer waiver). The two stateless reds are CI-confirmed flakes and the regression/Grype/Parser-memory reds are infra — none implicate the Iceberg timestamp-pruning changes. A re-run should clear the flakes.
3 participants
Frontport of #2239 onto
antalya-26.8.Identity partition pruning on Iceberg
timestamp_nscompared AvroInt64ticks at the wrong scale and dropped matching files. Min/max pruning also dropped files when Icebergtimestampbounds were stored as nanoseconds.DateTime64(9)is now written astimestamp_ns(and timezone-awareDateTime64(9)astimestamptz_ns) on format version 3, and rejected on older format versions. Bounds whose magnitude is not above1e18fail open so mixed nanosecond and microsecond statistics are not guessed.Related: #2239
Related: #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 writingDateTime64(9)astimestamp_ns.Documentation entry for user-facing changes
Iceberg engine reference:
DateTime64(9)writes map totimestamp_ns/timestamptz_nson format version 3.CI/CD Options
Exclude tests:
Regression jobs to run:
Made with Cursor