JingsongLi commented on PR #530:
URL: https://github.com/apache/paimon-rust/pull/530#issuecomment-4994371168
## Code Review Summary
**Mode**: full
**Scope**: apache/paimon-rust PR #530
(`cd8db7ba89fc3f3a7b1758fb49cf701bd9c427c2`)
**Files Changed**: 5 files (+1119/-242 lines)
**Score**: 77/100
**GAN Stats**: Generators found 6 issues -> 4 after deduplication.
Discriminator accepted 2 / challenged 2 / rejected 0. Arbiter included 2 /
downgraded 2 / excluded 0.
### Critical Issues (must fix before merge)
None found.
### Major Issues (should fix)
1. **[major] The fallback stream eagerly opens and retains every overlapping
historical BLOB file before yielding its first batch**
Location:
`crates/paimon/src/table/data_evolution_reader/blob_fallback.rs:73-120`
The setup loop awaits `input.reader()` and `IndexedBlobReader::open()`
for all selected files and stores every reader and full row index before
constructing `RowIdBatchCursor`. A long compaction history therefore makes
time-to-first-row, index memory, and retained reader resources scale with all
historical files, including later row ranges that the first batch cannot touch.
The existing single-sequence path opens one file at a time.
**Recommendation**: Keep only lightweight range metadata in the plan;
open historical files on demand for the current unresolved batch, and use a
bounded cache or release readers after their range is passed.
2. **[major] Older BLOB payloads are fetched even after a row has already
been resolved**
Location:
`crates/paimon/src/table/data_evolution_reader/blob_fallback.rs:139-173`
`file_positions` is built for every overlapping row and `read_positions`
completes before the code checks `resolved[output_position]`. Consequently,
older full values are transferred and allocated only to be discarded when a
newer value or explicit NULL already won. With large BLOBs and several retained
versions, ordinary scans multiply object-store requests, bytes, latency, and
memory pressure.
**Recommendation**: Add positions only when the current state is
`BlobReadValue::Placeholder`, and stop visiting older groups once no
placeholders remain.
### Minor Issues
1. **[minor] `BlobBunch::add` repeatedly rebuilds and sorts the accumulated
logical ranges**
Location: `crates/paimon/src/table/data_evolution_reader.rs:1800-1822`
Each insertion scans prior same-sequence files, clones all existing
ranges, and sorts/merges the full prefix again, resulting in worst-case `O(F^2
log F)` metadata work. This PR increases `F` by retaining historical sequence
groups. The impact is bounded to one field/merge group and is not benchmarked,
so this is not merge-blocking.
**Recommendation**: Store files during construction and validate/merge
ranges once after the bunch is complete, or maintain an incrementally merged
interval set.
### Suggestions
1. **[suggestion] Reuse the production deletion-vector serializer in the new
test helper**
Location: `crates/paimon/src/table/data_evolution_reader.rs:4590-4623`
`write_test_deletion_file` hand-encodes the deletion-vector format even
though `DeletionVector::serialize_to_bytes` is available. Prefer the production
serializer locally; a repository-wide test-helper cleanup is outside this PR's
required scope.
### Verification
- `cargo fmt --all -- --check`: passed.
- `cargo test -p paimon --all-targets --features fulltext,vortex`: passed
(1580 unit tests passed, 1 ignored; all listed integration/example targets
passed).
- GitHub CI: builds, checks, unit tests, and the DataFusion/Go/Python/Rust
integrations passed. `integration (lumina)` was automatically rerun and is
still pending.
### Positive Observations
- The change models `Null`, `Placeholder`, and `Value` explicitly instead of
conflating sentinel states.
- Java-compatible fixtures and tests cover descriptors, row selections,
deletion vectors, and schema-evolution fallback behavior.
- The focused local verification suite passes cleanly.
--
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]
