shyjsarah commented on PR #858:
URL: https://github.com/apache/paimon-rust/pull/858#issuecomment-5749102578
## Incremental Code Review Summary
**Mode**: full incremental review
**Scope**: apache/paimon-rust#858,
`764df07534677e9944764001125f3510fab42ae6` ->
`b2cbc6290c21723ef081b45cf81f9de6ae77e9ad`
**Incremental Changes**: 3 files (+168/-44 lines; 375 diff lines)
**Current Score**: 61/100 (previously 41/100)
**GAN Stats**: Generators found 6 current issues -> Discriminator accepted 6
/ challenged 0 / rejected 0 -> no arbitration required
**CI**: All current GitHub checks pass, including three-platform build/unit,
check, and DataFusion integration.
### Resolution of Previous Findings
- **Fixed — logic-1**: Known-zero partition rows are removed before batch
construction. A new deletion-vector regression test confirms grouped `COUNT(*)`
omits a fully deleted partition.
- **Fixed — logic-2**: Snapshot selection now occurs during physical
planning, and both the manifest-count path and unknown-cardinality fallback use
the pinned table. New tests cover commits made after physical-plan creation.
- **Fixed — perf-2**: The manual sequential stream flatten was replaced with
DataFusion's standard `execute_stream` helper.
- **Fixed — perf-3**: `counts_to_batch` now constructs only projected
columns instead of decoding every partition field first.
- **Remaining — perf-1**: One unknown count still discards all exact counts
and scans the complete selected table.
- **Remaining — perf-4**: Retained identity memory is still not byte-bounded.
- **Remaining — arch-1**: Slim Avro decoders still duplicate the canonical
storage-format traversal.
- **Remaining — arch-2**: `DeleteSet` still duplicates the canonical
file-identity contract.
### Critical Issues (must fix before merge)
None found.
### Major Issues (should fix)
1. **[arch-4, new] `with_table` can break the provider's schema invariant**
Location: `crates/integrations/datafusion/src/table/mod.rs:107-180`
`PaimonTableProvider` derives and caches its Arrow schema when it is
constructed, but the new `with_table` method replaces only `self.table`. If the
snapshot resolved during physical planning has a different schema, the provider
can expose the old cached schema while filters, projections, and fallback reads
operate on the replacement table's schema. A concurrent add/drop/reorder before
a partition column can therefore select the wrong field, index out of bounds,
or fail with an output-schema/type mismatch when the unknown-cardinality
fallback is used.
**Fix**: replace the generic mutator with a fallible snapshot-pinning
operation that re-derives the replacement table's Arrow schema and verifies it
matches the logical provider schema before swapping. If it differs, decline the
count rewrite or rebuild all related provider state consistently.
2. **[perf-1, remaining] One unknown count still discards all exact manifest
counts**
Location:
`crates/integrations/datafusion/src/partition_count_pushdown.rs:384-395`
After aggregating all matching manifests, any `None` count still replaces
the entire result with `scan_by_reading()` over the full selected table. The
update improves how fallback output is consumed, but it neither reuses known
counts nor restricts the scan to unknown partition keys.
**Fix**: concatenate known manifest-count rows with an ordinary scan
filtered to the unknown full partition keys. At minimum, detect an unavoidable
full fallback before performing redundant manifest aggregation.
3. **[perf-4, remaining] The retained-entry budget is still not a
heap-memory bound**
Location: `crates/paimon/src/table/partition_row_count.rs:210-232`
ADD accounting still charges one unit while cloning variable-sized
`extra_files`, `embedded_index`, and `external_path` values; DELETE identities
remain unbudgeted. Large embedded indexes or delete-heavy snapshots can still
retain hundreds of MB or more and defeat the optimization's OOM-avoidance goal.
**Fix**: account by estimated heap bytes and spill exact identities after
a configurable byte limit, applying the same policy to DELETE identities.
### Minor Issues
1. **[perf-5, new] Latest-snapshot pinning fetches the same snapshot twice**
Location:
`crates/integrations/datafusion/src/partition_count_pushdown.rs:295-304`
`get_latest_snapshot()` loads the snapshot, but only its id is retained.
`copy_with_time_travel_strict(snapshot-id)` then resolves and reads the same
snapshot again. This adds an avoidable object-store metadata round trip and
JSON read to every eligible latest-snapshot plan. Reuse the already resolved
`Snapshot` through a public equivalent of `copy_with_resolved_snapshot`.
2. **[arch-1, remaining] Slim Avro decoders duplicate storage-format logic**
Location: `crates/paimon/src/spec/avro/manifest_entry_decode.rs:170-337`
The parallel decoder paths remain unchanged and can drift when
schema/default/nullability behavior evolves. Share traversal and
field-selection machinery with the canonical decoder where practical.
3. **[arch-2, remaining] `DeleteSet` duplicates the canonical file-identity
contract**
Location: `crates/paimon/src/table/partition_row_count.rs:169-293`
The representation still manually mirrors `Identifier` fields. A shared
borrowed identity view/accessor would keep equality semantics aligned without
restoring unnecessary allocations.
### Positive Observations
- The two prior correctness blockers are fixed and now have targeted
regression coverage.
- Fallback output uses DataFusion's standard concurrent execution helper.
- Projection is applied during column construction, eliminating the prior
unnecessary decoding and allocation.
- The current CI matrix is fully green.
--
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]
