[improvement](lance) Balance fragment splits by row count - #66597
Conversation
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated code review: changes requested.
Critical checkpoints:
- Scheduling correctness: the catalog path now produces bounded weights that reach
FederationBackendPolicy, but S3/file Lance TVFs still discard fragment row counts and remain equally weighted. - Compatibility and performance: FE uses logical rows after deletions, while the pinned BE Lance legacy reader applies deletions only after reading and merging physical batches, so tombstone-heavy legacy fragments can be severely underweighted.
- Query semantics and protocol: split count/order, pinned version and fragment IDs, scan-range serialization, and the intentional single-split local-TVF/vector-search modes remain unchanged; no query-result or protocol defect was found.
- Tests: the new unit test covers standard, proportional, and minimum raw weights, but does not cover S3/file TVF parity or deletion-heavy legacy fragments.
- User focus: no additional focus was supplied; the full PR was reviewed.
- Completion: the review converged in two rounds with every candidate accepted, deduplicated, or dismissed; two inline issues are attached.
| } | ||
| splits.add(new LanceSplit(metadata.getDatasetUri(), metadata.getVersion(), | ||
| fragment.getId(), fragment.getRowCount())); | ||
| targetRows = Math.max(targetRows, Math.max(fragment.getRowCount(), 1)); |
There was a problem hiding this comment.
[P2] Base legacy-fragment weights on physical scan work
fragment.getRowCount() comes from Java FragmentMetadata#getNumRows(), which is physical rows minus deletions. However, the BE links lance-c 0.1.2/Lance 4.0.1, whose legacy fragment reader reads and merges the projected physical batches before applying the deletion vector. A legacy fragment with 1,000,000 physical rows and 990,000 tombstones can therefore receive only a 1 percent weight even though its scan still does close to a full fragment's I/O/decoding work; compared with the previous equal weights, this can actively co-locate expensive tombstone-heavy fragments. Please use physical rows or another format-aware scan-cost metric for this normalization and cover a deletion-heavy legacy fragment.
| for (LanceTableMetadata.LanceFragmentInfo fragment : metadata.getFragments()) { | ||
| LanceSplit split = new LanceSplit(metadata.getDatasetUri(), metadata.getVersion(), | ||
| fragment.getId(), fragment.getRowCount()); | ||
| split.setTargetSplitSize(targetRows); |
There was a problem hiding this comment.
[P2] Keep S3/file TVF fragment weights in sync
This sets the common target only for catalog-backed LanceScanNode splits. The S3 Lance TVF path loads the same fragment row counts via loadLatestForTvf(), but setLanceTableMetadata() retains only IDs; TVFScanNode#getLanceSplits() then passes rowCount=1 and never sets a target. The file(...) TVF delegates S3 Lance scans to this same path, so both entry points still give every fragment the standard scheduling weight and keep the skew this change is intended to remove. Please retain row counts through the TVF metadata boundary, apply the same normalization there, and add a parity test for that split path.
FE Regression Coverage ReportIncrement line coverage |
… TVF Address review feedback on fragment split balancing: - Weight splits by physical rows instead of post-deletion logical rows. The pinned BE legacy reader scans and merges physical batches before applying the deletion vector, so a tombstone-heavy fragment does close to a full fragment's I/O; logical rows underweighted it. LanceFragmentInfo now carries physicalRows (rowCount kept for statistics) and both the target and per-split weight use it. - Keep S3/file Lance TVF splits in sync. The TVF path retained only fragment ids and passed rowCount=1, leaving every fragment equally weighted. ExternalFileTableValuedFunction now retains full fragment info and TVFScanNode applies the same physical-row normalization. - Add a deletion-heavy fragment case and an S3 TVF weight-parity assertion.
github-actions
Bot
removed
the
approved
|
@yiguolei @zhangstar333 mind taking a look when you have a sec? 🙏 |
|
/review |
|
run buildall |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
@Jay-ju Sure. And you can add more features to doris lance catalog for example hybrid search. |
There was a problem hiding this comment.
Requesting changes for two P2 scheduling correctness gaps that remain after the legacy-row and S3/file parity fixes.
- The new loader consumes raw cached
physical_rowsfor a supported broken v0.8.0 migration case even though the pinned Lance implementation deliberately distrusts that value. - The unconditional physical-row metric fixes legacy fragments but overweights deletion-heavy stable-format fragments, whose reader plans valid ranges before data I/O.
I reviewed all nine changed files and traced catalog, S3, delegated file(...), local, and external-search paths through fixed-version fragment serialization and the BE reader. Catalog/S3/file(...) now share the same immutable fragment snapshot and normalization; local/search exclusions are intentional. Split count/order, thrift identifiers, query protocol, zero/extreme normalization, and the generic scheduler's one-standard-split tolerance did not reveal another defect. The new unit tests verify handcrafted DTO weights but do not exercise either raw-versus-trusted migration metadata or a real stable reader. No build or test was run, as required by the supplied review constraints.
The prior two inline threads were rechecked live: S3/file parity is addressed, and legacy underweighting is addressed when physical metadata is trustworthy. The two comments below are distinct follow-up failures. Review converged after two rounds; all other candidates were dismissed with evidence or duplicate-covered.
| fragments.add(new LanceTableMetadata.LanceFragmentInfo( | ||
| fragment.getId(), fragment.metadata().getNumRows())); | ||
| fragment.getId(), fragment.metadata().getNumRows(), | ||
| fragment.metadata().getPhysicalRows())); |
There was a problem hiding this comment.
[P2] Fence broken migration metadata before trusting physicalRows
The pinned Lance implementation does not trust cached physical_rows when manifest.writer_version is absent: it opens a data file instead because the v0.8.0 migration from older deletion-bearing datasets could store a post-deletion count there. Java Dataset#getFragments() exports raw fragment metadata, so getPhysicalRows() here bypasses that fence. For that supported migrated dataset class, a deletion-heavy fragment can remain severely underweighted even though the BE derives and reads its actual physical rows. This is distinct from the earlier logical-row thread: switching from getNumRows() to this raw field still fails when the populated cache is unvouched. Please use a trust-aware SDK/JNI count or retain standard weights for such manifests, and cover the broken migration fixture.
| List<Split> splits = new ArrayList<>(plannedFragments); | ||
| for (LanceTableMetadata.LanceFragmentInfo fragment : metadata.getFragments()) { | ||
| LanceSplit split = new LanceSplit(metadata.getDatasetUri(), metadata.getVersion(), | ||
| fragment.getId(), fragment.getPhysicalRows()); |
There was a problem hiding this comment.
[P2] Keep stable-format fragments on a deletion-aware weight
Physical rows match the legacy reader, but Lance 4.0.1 dispatches stable storage to a different read path. Its FilteredReadExec turns the deletion vector into valid ranges and passes those ranges to read_ranges() before data I/O. For a multi-page stable fragment with 1,000,000 physical rows and 990,000 contiguous tombstones, the reader can skip pages wholly covered by the deleted region and read/decode only the live ranges plus boundary-page and deletion-metadata overhead, yet this assigns the same weight as a fully live fragment. The earlier thread covered legacy underweighting; this is the opposite failure in the separate stable path. Please carry a format-aware work metric through both catalog and TVF builders and cover a deletion-heavy stable fixture.
FE Regression Coverage ReportIncrement line coverage |
|
skip buildall |
4 participants
What problem does this PR solve?
Lance fragment metadata already carries the visible row count, and
LanceSplitstores it asselfSplitWeight. However,FileSplit#getSplitWeightonly uses that value when a commontargetSplitSizeis set. As a result, all Lance fragments currently have the same standard scheduling weight regardless of row count, which can cause uneven scan work across BEs when fragment sizes differ.This PR uses the largest fragment row count as one standard split and assigns smaller fragments a proportional weight. It changes only FE scheduling metadata:
Verification
LanceScanNodeTestcovering fragment row counts1000,250, and0, which map to raw split weights100,25, and the minimum1.mvn validate -pl fe-core -am -DskipTestspassed for all 15 FE reactor modules with zero Checkstyle violations.Check List