Repository navigation
fix(core): align evolution stats and aggregation flush with Java - #839
Conversation
|
Non-blocking, pre-existing follow-up found during the cross-reader validation: the Rust aggregation reader can still aggregate a partition primary-key column. Reproduction: create a table with columns The write side in this PR correctly passes full primary keys. However, I also reproduced the same result with the pre-fix binding from #838, and the relevant read-side production wiring is unchanged by this PR. This is therefore a separate existing issue, not a regression or a blocker for these DE/flush fixes. Please cover a partitioned aggregation table across at least two commits in the follow-up; a single-file physical-row assertion does not exercise this reader path. |
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
LGTM at 362a408d780931ee64ad96d6e92a2fcf9e4ccf33. No blocking issues found in the changes under review.
Checked the DE schema/field-ID mapping, dense statistics, conservative handling of changed or missing schemas, provider ties and incomplete row-range coverage, and statistics validity against Java. Also checked flush ordering, full primary-key exclusion, input changelog preservation, and emitted sequence bounds against the Java merge/write paths.
Independent validation:
cargo test --locked -p paimon --lib table::: 1,278 passed, 1 ignored;cargo fmt --all -- --checkpassed.- Built the Python wheel from this commit. Seventeen focused interop cases passed, covering Java- and Python-written DE files after drop/reorder/rename/type widening/drop-and-readd, full/projected statistics, physical aggregation output for fixed/dynamic buckets, sequence ties, NULL/default aggregators, original input changelog rows, and cross-commit aggregate values. Eight of these cases failed with the pre-fix #838 binding.
- Expanded related Python/native regression suite: 358 passed, with 168 tracked native plans; three Vortex tests were skipped because the local interpreter is Python 3.10.
A separate pre-existing read-side partition-PK aggregation issue is documented in my follow-up comment. The write-side fix passes, but this approval should not be read as claiming that the older reader issue is fixed.
CI is not fully green: the Lumina job failed while linking blob_descriptor_functions, with rust-lld terminated by signal 7 / Bus error. The log shows a linker crash rather than a test assertion failure; the other current checks succeeded. These local tests do not replace the full CI matrix.
A file written under aggregation could hold several versions of a key (before apache#839), and ALTER can switch the table to deduplicate afterwards. Skip the sort merge only when every file was written under deduplicate or first-row.
A file written under aggregation could hold several versions of a key (before apache#839), and ALTER can switch the table to deduplicate afterwards. Skip the sort merge only when every file was written under deduplicate or first-row.
…ting schemas Neither the stored nor the read schema proves which engine wrote a file: before apache#839 copy_with_options could switch the writer to aggregation under the same schema ID, and that writer kept every version of a key. Drop the stored-schema lookup. The unmerged path now holds one batch back and checks adjacent keys; at the first repeated key it hands the held batch and the rest of the run to the sort merge.
DE stats pruning currently interprets historical files using the current schema layout. Dropping a column can make a predicate on the next column read the wrong statistics and discard matching rows. Aggregation writes also retain duplicate keys within a single data file, violating the merge-tree reader assumption shared with Java and Python.
This aligns both paths with Java:
Regression tests cover schema changes with full/dense stats, missing schemas, provider ties and partial overlap, invalid stats, physical Parquet rows, null/default aggregators, sequence ties, partition keys, changelog preservation, retract rejection and aggregation across commits.
Validation:
cargo test --locked -p paimon --lib table::— 1,278 passed, 1 ignored.cargo clippy --locked -p paimon --all-targets -- -D warnings— passed.cargo fmt --all --check— passed.Java references: DE evolution stats, write-buffer merge, output sequence metadata.