[opt](lance) add session cache and data cache in lance - #67650
zhangstar333 wants to merge 12 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
5b7a284 to
e078d4f
Compare
zhangstar333
changed the title
|
/review |
1 similar comment
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for seven P1 correctness, availability, and compatibility issues on head 8283a0ce9bfe4ca0b9481ade499041fdb14a3cc7.
Cache/session correctness: the default-on Foyer path can return truncated cache hits, its persistent namespace can collide across distinct S3-compatible endpoints, a cache-only initialization failure permanently gates every Lance read, and the pinned Lance v11 URI-scoped metadata cache can reuse stable-row-ID state across dataset generations.
FE/BE and metadata compatibility: ordinary scalar-segment plans emitted by a new FE are rejected by a base-version BE, while optional scalar-index discovery has become a hard dependency of schema discovery and ordinary scans.
Third-party repeatability: an existing generic patched_mark skips this revision's expanded Lance patch chain on a retained source tree.
Final checkpoint sweep covered all 32 authoritative changed paths and the required reader lifecycle, schema/identity, predicate/delete/fallback, COUNT/LIMIT/OFFSET, vector/FTS, lazy materialization, ABI/pin, and clean/incremental patch-build contracts. Two independent convergence rounds completed; the second round produced no new valuable findings. No additional user focus was specified. Per the review instructions, no build was run.
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Reviewed the complete 36-file authoritative diff at head 4db7c2372f1098eda589f37ca4fe2e04fd080a6b, including the shared Lance session/cache, scalar/vector/FTS planning and execution, metadata/snapshot paths, Nereids lazy materialization, Thrift compatibility, all six Lance-C patches, platform/build glue, and tests. Existing inline threads were treated as hard duplicate fences; the user focus contained no additional scope.
Requesting changes for three newly identified issues:
- A pushed filter that cannot drive the chosen physical scalar segment can collapse a fragment-parallel scan into one full scan on one BE.
- Per-dataset cache counters are not published until dataset close, leaving active and error-exit profiles stale.
- The new
Remotecache counter records logical miss bytes, excludes aligned read amplification, and also counts local origins.
Validation was static only because this review runner explicitly prohibits builds/tests. Observed CI at submission preparation: third-party Linux/macOS/macOS-arm64 builds, third-party script test, Cloud UT, formatting, licensing, dependency review, and related checks pass; compile, FE UT, and BE UT remain pending. No checkout-local code-review SKILL.md was discoverable after a full repository search, so the review followed the explicit prompt, required AGENTS.md files, and mandatory scanner-v2 design/review guides.
| LanceIndexSegmentInfo index = segments.get(0); | ||
| if ((index.getIndexType() != IndexType.BTREE && index.getIndexType() != IndexType.BITMAP | ||
| && index.getIndexType() != IndexType.LABEL_LIST) | ||
| || index.getFieldIds().size() != 1 || !filterFields.contains(index.getFieldIds().get(0))) { |
There was a problem hiding this comment.
[P1] Only create a segment split when the pushed filter can actually drive this index. This check accepts any pushed expression that mentions the field, but Doris pushes predicates such as !=, NOT IN, and IS NOT NULL that Lance v11 represents as Not(Query); PR #79's driver() only descends Query/And. It then reports no_driver after disabling ordinary scalar indexes and full-scans the segment's entire fragment domain. With the common single segment covering many fragments, this turns the previous fragment-parallel scan into one unsplittable full scan on one BE. Please align FE eligibility with the typed Lance planner, or retain fragment/global-index planning whenever a driver is not guaranteed.
| ADD_CHILD_COUNTER_WITH_LEVEL(_scanner_profile, "LanceDataCacheBytesReadFromCache", | ||
| TUnit::BYTES, LANCE_READER_PROFILE, 1); | ||
| _data_cache_bytes_read_from_remote = | ||
| ADD_CHILD_COUNTER_WITH_LEVEL(_scanner_profile, "LanceDataCacheBytesReadFromRemote", |
There was a problem hiding this comment.
[P2] Do not expose this logical miss counter as remote I/O bytes. The C ABI says bytes_read_from_remote counts requested bytes returned after a miss and excludes block-aligned amplification; cached_ranges() can fetch a full 1 MiB block for a tiny range while incrementing only the tiny range. The same field is also incremented for local origins. This profile value therefore understates physical reads and misattributes local I/O as remote. Please name it as logical origin/miss bytes, and separately publish actual aligned origin bytes/requests with the real source attribution.
| _fts_query_context = nullptr; | ||
| } | ||
| if (_dataset != nullptr) { | ||
| _collect_data_cache_statistics(); |
There was a problem hiding this comment.
[P2] Refresh these counters at scan batch boundaries. This is the only call to _collect_data_cache_statistics(), and _close_dataset() normally runs only when the reader closes (the same dataset can also span several splits). Every successful batch, intermediate split, cancellation, and scanner/conversion error therefore leaves the new cache counters at zero until the query is already tearing down, contrary to the scanner-v2 requirement that child statistics be visible for in-progress scans. Please sample the cumulative dataset snapshot on each get_block() exit, including EOF/error paths, while retaining the close-time refresh.
8347f2d to
7312a9c
Compare
|
run buildall |
1 similar comment
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
|
/review |
|
run buildall |
f542810 to
817ef5c
Compare
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
4 participants
What problem does this PR solve?
Problem Summary:
doc: apache/doris-website#4126
add session cache and data cache in lance
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)