shyjsarah commented on PR #858:
URL: https://github.com/apache/paimon-rust/pull/858#issuecomment-5748174572
## Code Review Summary
**Mode**: full
**Scope**: apache/paimon-rust#858 at
`764df07534677e9944764001125f3510fab42ae6`
**Files Changed**: 11 files (+2395/-4 lines)
**Score**: 41/100
**GAN Stats**: Generators found 9 issues -> Discriminator accepted 6 /
challenged 2 / rejected 1 -> Arbiter included 6 / adjusted 2 / excluded 1
**CI**: All reported GitHub checks pass, including three-platform
build/unit, check, and DataFusion integration.
### Critical Issues (must fix before merge)
None found.
### Major Issues (should fix)
1. **[logic-1] Known-zero partitions create groups that do not exist in the
table**
Location:
`crates/integrations/datafusion/src/partition_count_pushdown.rs:374-407`
`counts_to_batch` materializes every `PartitionRowCount`, including
`Some(0)`. For a partition whose rows are all removed by deletion vectors, a
normal scan supplies no input row to `GROUP BY`, but the rewritten plan
supplies a synthetic `(partition, 0)` row and returns an extra group.
**Reproduction**: insert one row into a partition, delete it with a
deletion vector, then run `SELECT partition_col, COUNT(*) ... GROUP BY
partition_col`; the optimized query can emit the deleted partition with count
zero.
**Fix**: filter out known-zero counts before building the batch, while
retaining `None` so unknown counts still trigger fallback. Preserve the
ungrouped empty-input behavior via the existing coalesce-to-zero path.
2. **[logic-2] The optimized physical plan does not pin its snapshot**
Location:
`crates/integrations/datafusion/src/partition_count_pushdown.rs:335-345`
The normal provider plans splits during `TableProvider::scan`, thereby
fixing the snapshot represented by the physical plan. `PartitionRowCountExec`
stores only a cloned `Table` and resolves the latest snapshot when execution
polls the stream. A commit between physical planning and collection can
therefore change this rewritten query's result, and repeated execution of the
same physical plan can change again.
**Fix**: resolve and store the selected snapshot during
`PartitionRowCountProvider::scan`; defer manifest I/O, but not snapshot
selection, until execution.
3. **[perf-1] One unknown count discards all exact manifest counts and
forces a whole-table fallback**
Location:
`crates/integrations/datafusion/src/partition_count_pushdown.rs:347-365`
After reading and aggregating all selected manifests, any `None` count
causes `scan_by_reading` to scan the complete selected table. A single legacy
deletion vector without cardinality makes the query pay both the full metadata
pass and the full data scan, while exact counts for all other partitions are
discarded.
**Fix**: retain known partition counts and restrict fallback to unknown
full partition keys. At minimum, detect unavoidable unknown-cardinality
fallback before doing redundant manifest aggregation.
4. **[perf-3] Projection is applied only after all partition columns are
decoded and allocated**
Location:
`crates/integrations/datafusion/src/partition_count_pushdown.rs:374-413`
`counts_to_batch` constructs arrays for every partition field and only
then calls `batch.project(projection)`. Ungrouped `COUNT(*)` needs only the
row-count column; subset grouping needs only selected partition fields. On
high-partition-count tables this adds substantial avoidable CPU and peak
memory.
**Fix**: construct only projected source columns and build the
output-schema `RecordBatch` directly.
5. **[perf-4] The retained-entry budget is not a real memory bound**
Location: `crates/paimon/src/table/partition_row_count.rs:210-232`
The budget counts ADD entries, but each retained identity clones
variable-sized `extra_files`, `embedded_index`, and `external_path` data;
DELETE identities have no count or byte budget. Delete-heavy manifests or large
embedded indexes can still consume hundreds of MB or more and defeat the
feature's OOM-avoidance goal.
**Fix**: account retained state by estimated heap bytes and spill exact
identities after a configurable byte limit; apply the same policy to DELETE
identities.
### Minor Issues
1. **[perf-2] Fallback output partitions are consumed sequentially**
Location:
`crates/integrations/datafusion/src/partition_count_pushdown.rs:358-364`
Manual `stream::iter(streams).flatten()` serializes final output
consumption and can increase buffering or spill on the unknown-count fallback.
Upstream scan work is still driven concurrently by `RepartitionExec`, so this
is minor rather than major. Use
`datafusion::physical_plan::execute_stream(plan, context)` to coalesce outputs
through DataFusion's standard helper.
2. **[arch-1] Slim Avro decoders duplicate storage-format logic**
Location: `crates/paimon/src/spec/avro/manifest_entry_decode.rs:170-337`
The slim data- and index-manifest paths duplicate field dispatch,
union/null handling, defaults, and deletion-vector traversal. Tests cover
several schema variants and no current divergence was shown, but future schema
evolution must now keep parallel handwritten decoders aligned. Share
traversal/field-selection machinery with the canonical decoder where practical.
3. **[arch-2] `DeleteSet` duplicates the canonical manifest file-identity
contract**
Location: `crates/paimon/src/table/partition_row_count.rs:169-293`
The new representation manually enumerates the fields already centralized
by `Identifier`. A future identity-field change can make normal manifest
merging and partition counting disagree. Introduce a shared borrowed identity
view/accessor so both paths inherit the same equality contract without
restoring the allocations this path is trying to avoid.
### Suggestions
None beyond the fixes above.
### Positive Observations
- The optimization has dedicated integration coverage, including fallback
for deletion vectors without cardinality and a check that `EXPLAIN` does not
perform table I/O.
- The new index-manifest decoder tests exercise nullable and non-null array
items, missing cardinality, reordered fields, and unknown fields.
- Existing CI is green across Linux, macOS, Windows, unit tests, and
DataFusion integration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
