Skip to content

[python] Reject unsupported aggregation options before writing - #10165

Open
TheR1sing3un wants to merge 2 commits into
apache:masterfrom
TheR1sing3un:codex/python-aggregation-write-validation
Open

TheR1sing3un wants to merge 2 commits into
apache:masterfrom
TheR1sing3un:codex/python-aggregation-write-validation

Conversation

@TheR1sing3un

@TheR1sing3un TheR1sing3un commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Purpose

Aggregation tables configured with unsupported options can commit data using deduplicate semantics. A later read error cannot recover the discarded input. Validate these options when constructing the writer, before dynamic bucket assignment can retain hashes for rejected rows.

Reuse the read-side guard for unsupported retract options, sequence groups, aggregator identifiers and invalid sequence configurations. Explicitly false retract flags remain writable. This guard alone does not provide correct writes for supported aggregation configurations: those still use the existing deduplicate fallback. #10162 is required for supported aggregation write semantics; please keep that dependency explicit when merging. Native read validation of false-valued retract flags remains a separate compatibility gap.

Tests

  • Current branch is rebased on master 66e2530a7 (merged [python] Honor sequence.field in primary-key writes #10166), retaining both sequence and aggregation validation before row-key extraction. PyArrow 16.0.0 / NumPy 1.26.4: 179 aggregation, partial-update, sequence read/write, merge-buffer and dispatch tests passed, plus 4 subtests. Changed-file lint, Python 3.6 syntax and diff checks passed.
  • Batch, stream and direct writer construction reject unsupported configurations before creating the dynamic bucket extractor; no snapshot, HASH index files or Parquet data files are created.
  • False-valued retract flags are tested through successful writes and commits, independently of native reader support.
  • PyArrow 16.0.0 / NumPy 1.26.4: 265 relevant write, aggregation, partial-update and sequence tests passed, plus 33 subtests.
  • PyArrow 19.0.1 / NumPy 2.4.6: 62 aggregation, partial-update and sequence tests passed, plus 4 subtests. The new construction regressions fail before this fix.
  • Changed-file flake8, Python 3.6 syntax parsing, license headers and git diff --check passed.

Combined validation

Validated #10162 (8002ab54c), this PR (d846b29aa) and #10166 (2605de145) together in an isolated checkout. The integration retains this PR's early aggregation guard, #10166's early sequence validation and IEEE-aware ordering, and #10162's supported aggregation dispatch. The overlapping late-validation/fallback expectations from #10162 are replaced by the early-rejection tests.

  • PyArrow 16.0.0 / NumPy 1.26.4: 181 tests and 4 subtests passed across aggregation, partial-update, sequence read/write, merge-buffer and merge-engine dispatch. This reuses [python] Aggregate same-key rows in the write buffer of aggregation tables #10162's test_sum_aggregates_same_key_rows_in_one_batch (10 + 20 + 30 = 60, with another key preserved) and test_sequence_field_honored_within_one_batch.
  • Six additional real-table smoke cases passed: dynamic buckets, sum plus sequence-ordered last_value, ascending/descending FLOAT64 sequences, and same-batch/chunked/separate-commit writes. Each reads back 10 + 20 = 30; a NaN sequence wins ascending and the finite sequence wins descending. Both retract flags are explicitly false; the Python reader is selected for this check.

The combined implementation is verification-only; this PR remains scoped to early rejection of unsupported configurations.

Comment thread paimon-python/pypaimon/write/file_store_write.py Outdated
Comment thread paimon-python/pypaimon/tests/test_aggregation_e2e.py
@TheR1sing3un
TheR1sing3un force-pushed the codex/python-aggregation-write-validation branch from d88e800 to d846b29 Compare September 25, 2026 03:12
@JingsongLi

Copy link
Copy Markdown
Contributor

Reviewed the current head as a data-loss prevention change. It has standalone end-to-end value for unsupported aggregation configurations: writer construction now fails before a row can enter the buffer or dynamic-bucket extractor, so a later read error cannot be the first indication that input was discarded. I checked the construction order (FileStoreWrite precedes row-key extraction), the native dispatch (aggregation is excluded from native writes), and the read-side option guard reused here. Explicitly false retract flags remain accepted by the write guard.

Local verification: 91 relevant aggregation, partial-update, sequence, write-buffer, and native-dispatch tests passed; 14 optional-Rust cases were skipped and 4 subtests passed. The current CI is green for Native CI and all scheduled Python versions. The earlier dynamic-bucket side-effect and false-flag test issues have been addressed at this head. No new blocker found in this guard.

Production scope to keep visible: this PR does not make normal aggregation writes safe by itself. FileStoreWrite._build_pk_merge_function still falls back to deduplicate for otherwise supported aggregation configurations; #10162 is needed for actual aggregation write semantics. Please keep that dependency explicit in the merge decision and add a supported-aggregation write/read smoke test with the final combined behavior. The false-valued retract case is currently tested for write acceptance only, because native read validation remains a separate compatibility gap.

@TheR1sing3un
TheR1sing3un force-pushed the codex/python-aggregation-write-validation branch from d846b29 to 6c50dd9 Compare September 25, 2026 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants