Repository navigation
Conversation
The hash write layout routes rows to lanes by partition key and sorts each lane by the partition key only. That sort is not stable, so the rows of a partition lost the order they arrived in: an input sorted by the cluster key, loaded into a table with WRITE_DISTRIBUTION_MODE = 'hash', wrote overlapping blocks even with a single thread. Each row now carries its arrival position, counted across the input lanes, as a last sort key, projected away after the sort. The compaction after the layout uses OrderedBlockCompactBuilder, which never emits a large block ahead of pending smaller ones. Refs databendlabs#20711. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmvURX4AcHbn5WuJr7gmQG
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 |
| // BlockCompactBuilder would pass a large block through ahead of | ||
| // pending smaller ones; keep the arrival order instead. | ||
| pipeline.add_accumulating_transformer(|| { | ||
| OrderedBlockCompactBuilder::new(block_thresholds, 0) |
There was a problem hiding this comment.
This is a specialized modification. Should PARTITION BY guarantee an ordering? I don’t believe such a rigid substitution is appropriate, nor should it be enabled via configuration, as it potentially alters the semantics themselves.
There was a problem hiding this comment.
Agreed, removed. The compactor swap wasn't needed: with only the tie-breaker the ordered loads still write contiguous blocks in every partition. What remains doesn't make PARTITION BY guarantee an ordering. The hash layout already sorts each lane by the partition key; the change only breaks ties by arrival position, so equal partition keys keep their arrival order instead of an arbitrary one. An unpartitioned table already writes a sorted input's blocks contiguously (0 overlapping pairs in the table on #20711), while the same input into a hash-partitioned table gets 192–293 even with one thread. This removes that difference, with no setting.
| @@ -0,0 +1,110 @@ | |||
| statement ok | |||
| create or replace database db_09_0103 | |||
There was a problem hiding this comment.
Given current trends in AI development, I believe we should avoid having every change result in a new SLT file; instead, we should aim to incorporate these tests into existing, similar SLT files.
There was a problem hiding this comment.
Done: the case is now a section at the end of 09_0054_partition_by.test (fails on main with 119 overlapping pairs, passes with the fix, standalone and on a 2-node cluster), and the separate file is gone.
⏱ CI timing · Devrun 40920334868 · attempt 2 (wall time includes re-run gap) · 25.4 min (main p50 55.4 min, −30.0 min)
Minutes unless stated; durations exclude queue time unless stated; "max" = longest job in the group. Baseline = median of the last 10 successful runs (Dev: |
Review: the tie-breaker in the hash layout's sort is enough on its own. Without the switch to OrderedBlockCompactBuilder the ordered loads still write contiguous blocks in every partition, so the hash path keeps BlockCompactBuilder. The regression case moves into 09_0054_partition_by.test: one ordered Parquet file copied into a hash-partitioned table (119 overlapping block pairs on main, 0 with the fix, standalone and on a 2-node cluster), and an unordered control. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmvURX4AcHbn5WuJr7gmQG
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
Part of #20711 (preserving the continuity of an ordered input through the write path, as discussed there).
build_table_write_layout_with_schemaroutes rows to lanes by partition key and then sorts each lane by the partition key only. That sort is not stable, so the rows of a partition lose the order they arrived in. Loading an input already sorted by the cluster key into a table withPARTITION BY … WRITE_DISTRIBUTION_MODE = 'hash'therefore writes overlapping blocks, even withmax_threads = 1.Each row now carries its arrival position, counted across the input lanes (a block takes the next range of positions when it arrives), as the last sort key after the partition key. The position column is projected away with the partition-key columns after the sort, so each partition keeps the order its rows arrived in.
No setting, no new sort and no ordering guarantee for
PARTITION BY: the lane sort already exists and only gains a tie-breaker, so equal partition keys keep their arrival order instead of an arbitrary one. An unpartitioned table already writes a sorted input's blocks contiguously; this removes the reordering that only the hash layout adds. Serialization and writes stay parallel.Scope: one ordered input (one file, or an ordered query result) keeps its order in every partition. When several files are read in parallel, their rows arrive interleaved, so the arrival order is not the input order; restoring it before the write (e.g. an order-restoring gather of parallel Parquet reads) is a separate follow-up for #20711.
Measured on main (6c7f657), 10M rows sorted by
k(200 keys),PARTITION BY (v % 2) CLUSTER BY (k) ROW_PER_BLOCK = 100000 WRITE_DISTRIBUTION_MODE = 'hash'. Block pairs of one partition whosekranges strictly overlap (0 = each partition's blocks are contiguous):The last two rows at 8 threads are the multi-file case left for the follow-up.
Tests
A new case at the end of
09_0054_partition_by.testcopies one Parquet file ordered byk(400k rows) into a hash-partitioned table and checks that no two blocks of a partition overlap onkbeyond a shared endpoint; an unordered control does overlap. On main the ordered case fails (119 overlapping pairs); with this change it passes, standalone and on a local 2-node cluster.Also run: 09_0041_auto_compaction, 09_0016_remote_alter_recluster, 03_0028_copy_into_stage (09_0041 has the same 6 failures on the local cluster with and without this change).
Type of change
AI assistance
This change is