[opt](lance) add some profiles in lance node - #67005
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Request changes: four concrete issues remain.
Correctness and compatibility: the new metadata COUNT path can return a wrong COUNT(*) for supported Lance fragments whose deletion count is absent from metadata. Snapshot pinning, COUNT argument classification, pushed/residual predicate gating, runtime-filter gating, and zero/multi-fragment advancement otherwise checked out.
Lifecycle: the new metadata shortcut bypasses FileScannerV2's asynchronous IO stop signal. Scanner/dataset reuse, split reset, EOF/error/abort cleanup, Arrow stream ownership, and timer RAII otherwise checked out.
Profiles and physical-plan diagnostics: the changed InitReader average is re-averaged across relation request blocks, and the new FlatKnn counter can label an unsegmented default-index fallback as flat. The new Lance timer units, per-task ownership, serialization, optional mixed-version keys, and backend aggregation otherwise checked out.
Tests: the added tests exercise current/synthetic no-deletion metadata, a null IO context, and preformatted profile strings, so they do not cover the four failing branches. No additional user focus was supplied. Review coverage is complete and converged after two rounds: both normal full-coverage reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS in Round 2. Builds were not run, per the review-task instructions.
5b23576 to
2c1e9cc
Compare
2c1e9cc to
c5d9554
Compare
|
/review |
There was a problem hiding this comment.
Requesting changes for six new findings (two P1 and four P2): a deterministic C++ type-declaration compile conflict, stale distribution/license metadata for the lance-c bump, incomplete execution statistics on successful early-stop scans, ambiguous fragment-local top-k labels, loss of vector-column identity in EXPLAIN, and Lance metrics attached outside the V2 profile ownership hierarchy.
The existing live row-ID average thread at rowid_fetcher.cpp:1132 still applies and was intentionally not duplicated. I reviewed all changed files plus the required V2 design/checklist, callback lifecycle, row-ID profile propagation, dependency patch/API/ABI/license state, and current CI evidence. The macOS-arm64 failure occurs earlier in unrelated libunwind setup and is not a basis for these findings. No builds were run, as required by the review bundle.
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by at least one committer and no changes requested. |
What problem does this PR solve?
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)