Conversation
…n scalar index scans
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
COMPLETE static review of PR #68687 at f46234d. Three rounds converged: both normal full-review scans and the separate risk scan returned NO_NEW_VALUABLE_FINDINGS against the final five-comment set. I verified and deduplicated every candidate, swept all 43 changed paths and related FE/BE paths, and found no unresolved candidate. Five new P2 findings are attached inline: three-label OR loses Lance pushdown (M1); the committed array fixture is omitted from --check (M2); partial index coverage and runtime use are unproven (M3); broad NOT can collapse many fragment scans onto one BE (M4); and the SQL LIMIT case never sends a scanner limit (M5). There are no pre-existing P0/P1 inline findings to carry forward.
Critical checkpoint conclusions:
- Goal and proof: The change adds Lance array membership pushdown through Substrait, selects LabelList scalar index segments for Boolean expressions, upgrades lance-c, and adds indexed/partial/unindexed fixture tests. The converter, planner, FE unit assertions, and SQL cases demonstrate intended paths in source, but M1, M3, and M5 leave common or claimed paths unproved; this review did not run them.
- Scope: Changes are concentrated in FE converter/serializer/planner, FE tests, the Lance fixture generator and committed data, one regression suite, and the pinned lance-c version/patch. The fixture and dependency changes are necessary to exercise the feature. M2 identifies an omitted integration into the existing committed-fixture checker.
- Concurrency and locks: FE planning handles query-local expressions and split lists. BE scanner work may read multiple fragments locally, while scheduling assigns each segment split to one BE; M4 records the cross-BE parallelism loss. No new shared mutable lock protocol, lock order, or critical section was introduced in the changed source.
- Lifecycle and static initialization: The review traced FE predicate conversion through scan planning, thrift serialization, BE scanner creation, and scanner teardown/fallback. No new cross-translation-unit static dependency, resource lifecycle defect, or circular initialization was substantiated.
- Configuration: No new dynamic configuration item was added. Existing lance_fragments_per_split defaults to zero and materially affects M4; the changed path provides no selectivity gate that restores fragment fanout for a broad segment result.
- Compatibility: The new Substrait array_has binding and nullable List serialization were checked against the pinned lance-c/DataFusion implementation. Unsupported array shapes remain Doris residuals, and pinned lance-c can safely fall back within the assigned fragment domain. No new incompatibility or rolling-upgrade defect was substantiated in the reviewed paths.
- Parallel paths and conditions: Normal Nereids rewrites, NOT/OR driver collection, segment and uncovered-fragment splits, TopN/Limit translation, and index fallback were traced. The three-label rewrite bypasses conversion (M1); segment grouping causes M4; ordered TopN bypasses scanner LIMIT (M5). Converter eligibility and BE complete-expression checks preserve row correctness for admitted predicates in the static analysis.
- Tests and expected results: FE unit and Groovy expected row sets were checked against the generated 16-row fixture and appeared consistent. The SQL profile assertions cover the fully indexed table but do not prove partial coverage or runtime segment use (M3), and the ordered LIMIT result does not test scanner LIMIT (M5). The committed binary fixture needs its own --check validation (M2). No independent Doris build, test, or runtime profile was executed; author/CI claims are not treated as independent validation.
- Observability: Existing Lance EXPLAIN fields and scalar index profile counters expose segment searches, candidates, and fallbacks. The regression should assert those counters on the partial table (M3); no separate production logging or metric defect was substantiated.
- Transactions, persistence, and writes: Runtime changes are read-side scan planning and filtering. The committed Lance fixture contains prebuilt data/index manifests but does not change Doris transaction, EditLog, failover, or data-write logic. Fixture generation writes staging data outside runtime; M2/M3 concern validation of that committed artifact.
- FE/BE transport: Pushed filter bytes, selected segment and fragment IDs, and optional scan limit were traced through thrift to the BE Lance scanner. FE unit coverage checks limit serialization; M5 concerns the missing SQL-to-BE runtime exercise. No other transport omission was substantiated.
- Performance and other issues: M1 misses index acceleration for a normal OR shape and M4 reduces cluster scan parallelism for broad complement results. Their exact wall-time cost depends on data and workload. The final candidate sweep found no additional substantiated issue.
User focus: No additional focus was supplied. Review evidence is static only; no builds, tests, or source edits were performed.
| } | ||
| String functionName = function.getFnName().getFunction().toLowerCase(Locale.ROOT); | ||
| switch (functionName) { | ||
| case "array_contains": |
There was a problem hiding this comment.
[P2] Preserve pushdown for three or more OR'ed label checks. Nereids' normal ArrayContainToArrayOverlap rewrite replaces three distinct array_contains(labels, ...) disjuncts with arrays_overlap(labels, [...]) before this converter runs. This switch leaves arrays_overlap residual, so the new LabelList path and segment pruning never run for that common OR shape; the two-label regression cannot catch it. Please handle the rewritten expression with semantics checked against Doris (including NULLs), or keep these disjuncts available to Lance, and add a three-label SQL/profile case.
There was a problem hiding this comment.
Fixed in 531fcbd. Nonempty constant-string arrays_overlap now lowers to a balanced OR of array_has calls, including the three-label Nereids rewrite and either operand order. NULL needles and unsupported forms remain residual. Added FE tests and three-label SQL/Profile cases; the new FE test failed before the fix. Actual FE Substrait integration with upstream main plus Foyer passed, including nullable lists and NOT overlap. Full SQL execution on this revision is pending CI.
| staging = Path(staging_name) / "lance" | ||
| staging.mkdir() | ||
| build(staging, all_types_source, time_travel_source) | ||
| build_array_predicates(staging / "predicate_arrays") |
There was a problem hiding this comment.
[P2] Include predicate_arrays in the committed-fixture self-check. The --check path calls only check_catalog(output), which never opens the new predicate_arrays datasets; build_array_predicates() runs only during rebuild. A missing or stale committed indexed/partial/unindexed fixture can therefore pass --check and fail later in the SQL suite. Please add a reusable check for these datasets to check_catalog().
There was a problem hiding this comment.
Fixed in 531fcbd. Extracted a reusable array-fixture checker and call it from check_catalog(), so the existing --check path validates all three datasets without rebuilding. The full committed-catalog check passes, and a negative probe confirms missing array datasets are rejected.
| ids = dataset.to_table(columns=["id"], filter=predicate, | ||
| use_scalar_index=use_index)["id"].to_pylist() | ||
| assert sorted(ids) == expected, (name, predicate, ids) | ||
| assert len(dataset.list_indices()) == (0 if name == "unindexed" else 2) |
There was a problem hiding this comment.
[P2] Assert the partial index's actual fragment coverage. The checks here establish two fragments and two index names, but never inspect their fragment_ids; the partial SQL cases assert a SEGMENT plan and correct rows without checking runtime segment search/fallback counters. If both fragments become indexed, or the segment tasks all fall back, the suite still passes without exercising the intended indexed-plus-unindexed path. Please verify one covered and one uncovered fragment for both indexes and add a partial-table runtime counter assertion.
There was a problem hiding this comment.
Fixed in 531fcbd. Fixture checks now verify the exact fragment_ids for both LabelList and BTree indexes, including exactly one covered and one uncovered fragment in partial. A negative probe rejects an accidentally fully indexed partial fixture. The SQL suite now checks actual segment searches, candidate rows, and zero fallbacks on partial; actual C API integration confirmed those candidate counts.
| if (((CompoundPredicate) expr).getOp() == CompoundPredicate.Operator.AND) { | ||
| expr.getChildren().forEach(child -> collectDriverSlots(child, slots)); | ||
| CompoundPredicate.Operator op = ((CompoundPredicate) expr).getOp(); | ||
| if (op == CompoundPredicate.Operator.NOT) { |
There was a problem hiding this comment.
[P2] Preserve scan parallelism for broad NOT results. With one LabelList segment covering many fragments, NOT array_contains(labels, 'rare') now selects that segment and groupFragments makes one split for all covered fragments; previously NOT produced a split per fragment. If the label is rare or absent, the exact complement still reads nearly every row, but one BE does the work while the other BEs cannot help (the changed scan-node test records four splits becoming one). Please use a cost/selectivity gate to emit fragment splits for unselective complements, or otherwise subdivide the segment domain safely, and cover this plan shape in a test.
There was a problem hiding this comment.
Fixed in 531fcbd. Complement predicates retain one segment-scoped task per covered fragment instead of grouping the whole segment into one task. Tasks share the immutable segment UUID but have disjoint fragment ownership, preserved on fallback. The FE test now requires four distinct fragment splits for NOT (it failed before the fix); SQL coverage includes an absent-label complement and asserts the runtime search count. Disjoint-domain NOT and NOT-overlap C API integration passed.
| } | ||
| assertEquals([6, 14], sql(mixed).collect { (it[0] as Number).intValue() }) | ||
| // The appended fragment must still contribute matching rows, including with LIMIT. | ||
| assertEquals([[15L]], sql("SELECT id FROM ${relation} WHERE ${red} ORDER BY id DESC LIMIT 1")) |
There was a problem hiding this comment.
[P2] Add a query that actually exercises the Lance scanner LIMIT. ORDER BY id DESC LIMIT 1 becomes TopN; its bound stays on SortNode, and the TopN translator forwards a sort limit only to OlapScanNode. The Lance scan therefore keeps limit -1 and never sets TLanceFileDesc.limit, so this result check cannot detect a regression in indexed or uncovered split-limit handling. Add a fully pushed unordered LIMIT case, assert lanceLimit=1 in EXPLAIN, and check a selective result on each fragment path.
There was a problem hiding this comment.
Fixed in 531fcbd. Added fully pushed, unordered LIMIT 1 queries with selective predicates targeting the indexed fragment and the uncovered fragment separately, and assert lanceLimit=1 in EXPLAIN. The existing ordered check remains as TopN coverage. Actual FE Substrait/C API integration with scanner limit 1 returns the expected row on both paths; SQL execution is pending CI.
FE Regression Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Adopt merged upstream lance-c main and retain only the unchanged Foyer cache patch until its upstream PR merges. Convert non-null string overlap needles, keep complement scans split by fragment, and verify committed fixture coverage and runtime partial-index and scanner-limit paths. Validation: 61 focused FE tests, Checkstyle, 451 Rust/C API tests, 27 FE Substrait integration scans, fixture checks, and patch harnesses. Full Doris SQL regression on this revision is pending CI.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review status: INCOMPLETE under the required three-round cap. A distinct valuable finding arose in round 3, so a fourth convergence round is unavailable under the prompt. All four substantiated findings found so far are included as P2 inline comments. I found no P0/P1 issue and independently confirmed no still-applicable existing P0/P1 inline comment on this head.
Goal and outcome. This PR pushes safe direct List<Utf8> membership and constant-string overlap predicates into Lance, and lets same-field Boolean filters select scalar index segments. The converter keeps NULL needles, dynamic operands, unsupported list layouts, UDFs, and ordered-subsequence array_contains_all in Doris. FE serialization, disjoint fragment assignment, native full-filter recheck, partial-index fallback, and scanner LIMIT handling are consistent on the paths inspected. The four comments identify performance and build-integration gaps: repeated segment searches for fragment-split complements; large overlap expressions hitting the native index-planning budget and serial fallback; a stale installed lance-c archive surviving a pin change; and an OR with a leading-wildcard LIKE branch selecting a grouped segment that cannot be indexed.
Critical checkpoints (static inspection):
- Purpose, scope, and tests: The production edits are focused on FE conversion/planning and the pinned native dependency; fixtures and tests cover indexed, partial, and unindexed domains. FE unit tests and the SQL suite inspect small positive/negative/null/OR/NOT cases, candidate counters, and selective unordered LIMIT. They do not cover the multi-fragment mixed-complement, 65-needle budget, or unsupported OR-driver cases in the inline comments. I inspected tests and the author's stated validation; I did not execute builds or tests.
- Concurrency and lifecycle: FE planning is query-local. BE scanners own disjoint fragment domains and native candidate masks; no changed shared lock, lock order, thread entry, cross-translation-unit initializer, or ownership cycle was found. M-01 can multiply concurrent index work and mask memory despite correct fragment ownership.
- Configuration and compatibility: No new dynamic configuration is added. The new Substrait list/function definition uses the existing filter transport and the pinned C scanner API. M-03 shows that a complete older compilation image can still link an installed native archive from the previous pin without a version check, losing the new Boolean index behavior. No new FE-to-BE thrift variable or storage-format migration was added.
- Parallel and conditional paths: Indexed segment, uncovered fragment, native fallback, external-search bypass, residual evaluation, count shortcut, and LIMIT paths were traced. The special guards on direct string lists and non-NULL literals preserve the checked Doris semantics. M-01, M-02, and M-04 show avoidable loss or duplication of parallel index work; the final row filter remains in place on native fallback.
- Observability and results: Existing scanner metrics expose segment searches, candidates, and fallbacks; the new SQL suite checks them for its small fixtures. No new transaction, persistence, EditLog, or runtime data-write path is changed. Three committed fixture version hints, 28 binary fixture files, the generator/self-check, MinIO mirror, Foyer patch context, and all changed text files were included in the sweep. No
.outresult file was changed.
The five existing P2 threads address three-label overlap, committed-fixture checking, partial index coverage, broad NOT split parallelism, and scanner LIMIT coverage. Their cited changes are present on this head; these four new comments concern different instances or mechanisms. User focus: no additional focus was supplied. This is a static review only; full Doris SQL execution was pending CI in the PR description and was not independently verified here.
| // Complement selectivity is unknown. Preserve fragment-level scheduling so a | ||
| // broad NOT cannot funnel all covered data through one BE. Each task retains | ||
| // the same segment UUID but owns a disjoint domain, including on fallback. | ||
| if (splitComplementByFragment) { |
There was a problem hiding this comment.
[P2] Avoid repeating a segment-wide index search for every fragment. splitComplementByFragment is enabled by any pushed complement, including one on an unrelated conjunct such as key = 1 AND other <> 0. Every resulting task gets the same segment UUID; lance-c evaluates that segment's key index for each task, then applies the candidate mask to its fragment-scoped reader. F fragments therefore repeat the full search F times and can hold F candidate masks. Keep fragment parallelism for broad complements without repeating a full-segment search for a positive driver, and cover a multi-fragment mixed-conjunct case.
There was a problem hiding this comment.
Fixed in f5b9247. Segment grouping now requires a positive, indexable necessary condition; a separate complement no longer splits that segment into repeated searches. Complement-only scans retain one task per fragment with scalar-index use disabled. Added a two-fragment test for both a combined AND tree and separate pushed conjuncts. Actual FE plans executed through lance-c return the expected rows with exactly one segment search for the mixed predicate, including partial coverage.
| continue; | ||
| } | ||
| Plan candidate = groupFragments(metadata, segments, visibleFragments); | ||
| Plan candidate = groupFragments(metadata, segments, visibleFragments, |
There was a problem hiding this comment.
[P2] Keep large overlap filters out of a single segment split. arrays_overlap(labels, [65 distinct strings]) becomes 65 array_has leaves plus 64 OR nodes, while the pinned lance-c scoped planner stops at 128 nodes and falls back with expression_budget. This plan groups every fragment covered by the LabelList segment into one task, so one BE scans the full domain and evaluates 65 membership calls per row. Use fragment splits or another bounded plan when the native index expression cannot be used, and cover this threshold in a regression.
There was a problem hiding this comment.
Fixed in f5b9247. FE now bounds the translated scalar expression against the pinned native 128-node/32-depth limits before grouping. A 64-label overlap can use one segment search; 65 labels keep parallel fragment scans with the full filter pushed and scalar-index use disabled. Added unit tests for the boundary, enclosing conjuncts, and depth, plus SQL regression assertions for fragment grouping and runtime counters. Native verification of FE-produced plans confirmed identical expected rows, one search at 64 labels, and two fragment scans with zero searches/fallbacks at 65.
| LANCE_C_SOURCE="lance-c-9bd730add2ac70316c1d642b8459011e2dd92022" | ||
| LANCE_C_MD5SUM="63851b09bf1689032579f1a094ff2f37" | ||
| # Includes lance-c #93: scoped boolean scalar-index expressions and Substrait label filters. | ||
| LANCE_C_DOWNLOAD="https://codeload.github.com/lance-format/lance-c/tar.gz/cd63420bfbe27f6f0a1edcc873b9191af7d52852" |
There was a problem hiding this comment.
[P2] Make the updated lance-c revision reach builds with an installed archive. A complete pre-change compilation image has installed/lib64/liblance_c.a and the other sentinels that let build.sh skip the third-party rebuild, so it links the previous 9bd730 archive after this pin changes to cd63420. That old scoped planner falls back for OR/IN, while the new regression requires index searches and zero fallbacks; the unchanged C ABI gives no link error. Record and check the installed Lance revision, or force a rebuild when this pin changes.
There was a problem hiding this comment.
Fixed in f5b9247 and synchronized to master PR #68689 in c285848. The build compares the installed source/archive/patch fingerprint with the current checkout and checks nonempty Lance headers/archive. Installation invalidates the marker before publishing either file and writes it only after success; stale external builder output is rejected. The real build-gate/publication harness passes on both branches, covering unversioned/stale/matching installs, pin and patch changes, incomplete artifacts, missing sources, interrupted publication and retry.
| } else { | ||
| // One selected index must supply candidates for both OR branches, and for | ||
| // every leaf of a negated subtree. lance-c rechecks the complete predicate. | ||
| left.retainAll(right); |
There was a problem hiding this comment.
[P2] Do not treat a shared column as proof both OR branches can use the index. For key = 'x' OR key LIKE '%y%' on a multi-fragment BTree segment, both branches contribute key here, so the planner chooses one grouped segment task. The pinned Lance planner cannot derive a prefix for %y%, drops the OR index query, and lance-c falls back to scanning every covered fragment in that one task. This loses the prior per-fragment parallelism for a common predicate. Check branch indexability before grouping, or use fragment splits for unsupported shapes, and cover this case in a multi-fragment test.
There was a problem hiding this comment.
Fixed in f5b9247. Driver selection now checks positive leaf indexability for the selected index type before intersecting OR branches. Suffix/contains LIKE cannot justify grouping, while simple BTree prefixes remain supported. Added a multi-fragment unit case for equality OR suffix LIKE and a prefix control. Executing actual FE plans through lance-c confirmed two fragment scans and zero searches for the suffix case, versus one segment search for the prefix case, with expected rows in both cases.
…endency ### What problem does this PR solve? Related PR: apache#68687 Problem Summary: Complement predicates repeated a full segment search per fragment, while oversized overlap and unsupported OR drivers coalesced fallback scans onto one task. Select positive indexable drivers within the native expression budget; otherwise keep fragment scans with scalar-index use disabled. Preserve simple BTree prefixes and full predicate evaluation. Record and check the installed Lance source and patch fingerprint so an ABI-compatible old library cannot be reused. ### Release note Preserve parallel Lance predicate scans and rebuild stale Lance dependencies. ### Check List (For Author) - Test: 65 focused FE JUnit tests; FE Checkstyle; 17 FE-generated plans / 30 native scans; build-gate and interrupted-publication tests; Groovy compilation. Full distributed SQL regression execution is pending CI. - Behavior changed: Yes, unsafe segment grouping falls back to fragment scans. - Does this need documentation: Existing predicate documentation PR apache#4184.
|
run buildall |
|
/review |
### What problem does this PR solve? Related PR: apache#68687, apache#68689 Problem Summary: The archive-presence sentinel reused an old ABI-compatible Lance library after the dependency pin changed. Compare the installed source/archive/patch fingerprint with this checkout, rebuild stale or incomplete installs, and fail if an external builder still supplies mismatched output. Invalidate the fingerprint before header/archive publication and publish it only after both succeed. ### Release note Rebuild stale Lance dependencies instead of silently linking the previous revision. ### Check List (For Author) - Test: Build-gate and publication harness passed on master and branch-4.1; shell syntax checks passed. Full Doris compilation and BE UT are pending CI. - Behavior changed: Yes, outdated compilation images require refreshed dependencies. - Does this need documentation: No new user-facing interface.
### What problem does this PR solve? Problem Summary: An incomplete or outdated external third-party tree could erase installed dependencies before its builder failed. Check required inputs and compare the external Lance fingerprint with the checkout before removal. Make the installation harness use independent external definitions, verify preservation on rejected rebuilds, and run it in the third-party CI script job. ### Release note Preserve installed third-party dependencies when Lance rebuild inputs are incomplete or do not match the checkout. ### Check List (For Author) - Test: Installation harness passed on master and branch-4.1; the new missing-helper regression failed before the fix. Shell syntax, workflow YAML, and diff checks passed. - Behavior changed: Yes. Reject invalid Lance rebuild inputs before deleting installed dependencies. - Does this need documentation: No.
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: No final review submission was declared Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Review status: COMPLETE, static only. Reviewed the authoritative merge-base diff for head 436954625d8184cb20e5142a6a223278faa64935 across all 49 changed paths, including the 28 binary fixture files, and checked the nine preexisting inline threads. Six distinct P2 findings are inline. No preexisting P0/P1 issue remains to carry; existing P2 concerns are not reposted. Three review rounds converged, with both normal reviewers and the separate risk reviewer reporting NO_NEW_VALUABLE_FINDINGS in the final round. No additional user focus was supplied.
Part 1.3 critical checkpoints:
- Goal and proof: The PR adds Lance array predicate pushdown, scalar-index planning, an updated lance-c archive, install-fingerprint checks, and indexed/partial fixtures. FE unit tests, SQL regression cases, and an install harness were added or changed. Their contents exercise the intended common paths; the six inline cases show missing boundary coverage. I did not run builds or tests.
- Scope and clarity: The 21 text paths and 28 binary fixture paths correspond to those features. The workflow's top-level path filter misses a changed build entry point (M2). No other independently substantiated scope issue survived review.
- Concurrency: FE planning is query-local; the changed paths introduce no new shared thread, lock, or cross-thread state. Multi-fragment assignment can lose BE parallelism in the scenarios described by M4 and M5; no lock-order or data race concern was substantiated.
- Lifecycle and initialization: Traced build preflight, archive install, fingerprint invalidation/publication, and scan split to scanner handoff. No new static initialization dependency or resource-ownership defect was substantiated. M1 and M6 identify cases where the handoff disables an otherwise usable native index.
- Configuration: No new dynamic runtime configuration item is added; the build fingerprint and existing scalar-index flag were checked at their use sites.
- Compatibility: Checked FE Substrait array-function declarations against the pinned native dependency and the existing BE flag path. An old-archive OR/IN fallback is already covered by an existing thread. No distinct rolling-upgrade wrong-row failure was substantiated.
- Parallel paths and guards: Traced indexed, partially indexed, and unindexed fragments; segment and fragment splits; AND, OR, NOT, array, IN, and prefix predicates; native scoped search and full-filter fallback. The exact gate/budget/index-name failures are M1 and M3–M6. The fallback applies the full filter to its explicit fragment domain, so these findings concern lost index acceleration or parallelism, not a proven wrong-row result.
- Test coverage: FE converter/planner/scan tests, SQL result and profile assertions, fixture self-checks, and the install test cover common positive and fallback paths. They omit the six concrete boundary scenarios called out inline; no other distinct test gap survived duplicate review.
- Test results: Inspected changed SQL expectations and ordered result checks, fixture version hints, and test assertions. No test was executed independently, so runtime or CI success is unverified.
- Observability: Existing scan profile counters and native fallback reasons provide signals for these paths; no separate missing metric or excessive new logging was substantiated.
- Transactions and persistence: No Doris transaction, visible-version, delete-bitmap, or EditLog path changes. Fixture dataset files and a build fingerprint are generated artifacts; their publication and checker paths were traced without finding another independent issue.
- Data writes and crashes: No runtime table-write or storage mutation path changes. The offline fixture/build artifact paths were checked for stale publication after failure; no additional substantiated fault was found.
- FE/BE variables: No new wire field is introduced. The existing
use_scalar_indexfield is propagated through the scan descriptor to the BE scanner; M1 and M6 concern the changed value chosen for it. - Performance and remaining issues: M1, M3, M4, M5, and M6 give concrete index or parallelism regressions; M2 is a CI trigger gap. All candidate findings were validated against changed lines, native behavior, and existing threads. No unresolved suspicious point remains.
| plan.addUncoveredFragments(visibleFragments.values(), 1, scalarIndexPlan != null); | ||
| // A rejected segment driver must not be rediscovered independently by every | ||
| // fragment scanner. Keep the pushed filter, but evaluate it in parallel scans. | ||
| plan.addUncoveredFragments(visibleFragments.values(), 1, !lancePushedConjuncts.isEmpty()); |
There was a problem hiding this comment.
[P2] Preserve native index use when FE lacks segment metadata. A legacy scalar index without details is intentionally omitted from FE planning (LanceDatasetIndexDiscovery says scanner selection remains available), and a Dictionary field can similarly make field IDs unavailable even if the filter targets an indexed primitive column. In both cases scalarIndexPlan is null, but this new condition marks every pushed-filter fragment split use_scalar_index=false; BE then uses non-indexed fragment scans instead of the native index search previously available for selective predicates. Restrict the disable flag to filters FE can conclusively reject, and cover an unavailable-metadata indexed scan.
| - 'thirdparty/**' | ||
| - 'env.sh' | ||
| focused_test_changes: | ||
| - 'build.sh' |
There was a problem hiding this comment.
[P2] Include build.sh in the workflow trigger. This new focused_test_changes entry cannot make a build.sh-only PR run the Lance installation test: on.pull_request.paths above still admits only thirdparty/**, env.sh, and the workflow file, so GitHub skips the entire workflow before this job filter is evaluated. Add build.sh to the top-level paths list as well.
| Set<Integer> filterFields = collectFilterFields(metadata, pushedConjuncts); | ||
| if (filterFields.isEmpty()) { | ||
| return null; | ||
| int nodes = Math.max(0, pushedConjuncts.size() - 1); |
There was a problem hiding this comment.
[P2] Use the translated conjunction's actual depth for the budget. The converter sends top-level pushed predicates as one n-ary and:bool, which the pinned DataFusion Substrait consumer balances. With 34 simple pushed equalities, including one on an indexed column, the complete native tree is about 67 nodes and depth 6 (and the selected index query can be smaller), within its 128-node/32-depth limits. This code instead starts every leaf at depth 33 and rejects the segment plan; the fallback disables scalar indexing and scans fragments. Count the balanced top-level tree and add a wide-conjunction regression.
| expr.collect(SlotRef.class, slots); | ||
| return 2 * labels - 1; | ||
| } | ||
| return 1; |
There was a problem hiding this comment.
[P2] Account for short IN expansion in this budget. For arrays_overlap(labels, [63 strings]) AND category IN (0, 1) with indexes on both columns and a multi-fragment LabelList segment, FE counts 125 overlap nodes, one IN, and one AND (127) and groups the fragments. Pinned Lance rewrites the two-value IN into two equality leaves plus OR, making 129 native index-expression nodes. scoped_expression then reports expression_budget and one BE scans the entire grouped domain. Count the optimized shape or leave this case as fragment splits, and cover the boundary with a mixed-index predicate.
| } else { | ||
| // Sharing a slot is insufficient: both OR branches must actually be | ||
| // indexable. A suffix LIKE, for example, cannot supply BTree candidates. | ||
| left.retainAll(right); |
There was a problem hiding this comment.
[P2] Keep OR grouping aligned with the index name native will use. Lance permits two named indexes on key; if a_key_idx is listed first but covers one fragment and z_key_idx covers four, key = 1 OR key = 2 now passes this intersection and FE selects Z for its wider coverage. Native's first parser gives both equality leaves A, so the Z segment finds no driver and one BE full-scans all four fragments. Previously this OR used separate fragment tasks. Select the same logical index as native, or pass the chosen index into native planning, and cover unequal same-column index coverage.
| } else if (!"starts_with".equalsIgnoreCase(name)) { | ||
| return false; | ||
| } | ||
| return !prefix.isEmpty() && prefix.indexOf('%') < 0 && prefix.indexOf('_') < 0 && prefix.indexOf('\\') < 0; |
There was a problem hiding this comment.
[P2] Preserve BTree drivers for native-indexable prefixes. starts_with(key, 'test_ns$') treats _ literally, and pinned Lance creates a LikePrefix('test_ns$') query. It also uses LikePrefix('foo') plus a refine filter for key LIKE 'foo%bar%'. This blanket wildcard rejection discards both valid drivers, then fragment splits set use_scalar_index=false and selective queries lose index acceleration. Treat starts_with text literally and derive the safe leading prefix for LIKE, with multi-fragment tests for both shapes.
### What problem does this PR solve? Problem Summary: Missing FE metadata and competing same-column index names could disable native index use or coalesce fragments around the wrong index. Preserve native selection in those cases. Match the balanced top-level AND and short IN expansion when estimating the native expression budget. Admit literal starts_with prefixes and refined LIKE prefixes while retaining fragment scans for OR branches that native cannot union. Include build.sh in both CI trigger layers. Add planner and scan-descriptor regressions plus mixed overlap/IN SQL boundary coverage. ### Release note Preserve Lance index acceleration and fragment parallelism for metadata fallbacks, competing indexes, wide conjunctions, and prefix predicates. ### Check List (For Author) - Test: 70 focused FE tests passed; new regressions failed before the fixes. 12 FE-generated plans passed 30 native scans with row and index-counter assertions. FE Checkstyle, Groovy syntax, installation harness, and workflow trigger checks passed. Distributed SQL execution remains pending CI. - Behavior changed: Yes. Preserve native index selection when FE cannot safely group fragments, and align expression budgets and prefix eligibility with the pinned native planner. - Does this need documentation: No new user interface.
|
run buildall |
|
/review |
### What problem does this PR solve? Related PR: apache#68687 Problem Summary: Remove the added build.sh path triggers while retaining the Lance installation test in the existing thirdparty script job. ### Release note None ### Check List (For Author) - Test: Workflow YAML and trigger assertions, git diff --check, and Lance installation script tests passed. - Behavior changed: Yes; build.sh-only changes no longer select the thirdparty script job. - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: No final review submission was declared Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Static review COMPLETE for PR #68687 at 0b8517a. Three new P2 performance regressions are reported inline. No existing P0/P1 inline finding still applies, so the blocking-ID list is empty. Existing P2 discussions were used as duplicate fences, including the same-anchor indexed-complement budget case and the workflow trigger gap. All three round-3 reviewers returned NO_NEW_VALUABLE_FINDINGS after independent missed-issue checks. No additional user focus was supplied.
Part 1.3 checkpoints:
- Goal and proof: The change pushes array membership/overlap through Lance Substrait and improves scalar-index segment selection. FE unit tests, committed fixture checks, and the SQL suite exercise supported shapes, partial coverage, plans, rows, and profile counters. The three inline cases show that segment selection and index-budget handling remain incomplete.
- Scope: The FE converter/planner/serializer, function mapping, Lance pin/build gate, fixtures, and tests are connected to that goal. The changed-file sweep covered all 49 paths, including 31 committed fixture data/index/transaction/version files.
- Concurrency: FE planning state is request-local. BE fragment tasks can run concurrently, but the changed code adds no shared mutable planner state or lock acquisition; the risks found concern task grouping and repeated searches.
- Lifecycle and static initialization: Checked third-party marker invalidation, header/archive publication, retry gate, and split ownership/fallback. No new cross-translation-unit static dependency or unreleased lifecycle was identified.
- Configuration: No new dynamic runtime configuration was introduced. Existing split/index settings and the new CI path filter were checked; the latter omission already has an inline P2 thread.
- Compatibility: No new FE-BE thrift field or persisted storage format was introduced. Function anchors, list types, pinned native consumer, and existing scalar-index flags were traced; runtime compatibility was not independently executed.
- Parallel paths: Indexed, partially indexed, unindexed, missing-metadata, complement, OR, and fragment fallback paths were checked. Existing threads cover known parallel-path gaps; the three new cases are distinct.
- Conditional checks: The new positive-leaf and expression-budget guards are the source of the three inline findings. Supported array shapes and NULL/empty guards were separately traced to residual handling; no additional row-semantic failure was substantiated.
- Test coverage: Current FE and SQL assertions cover normal array pushdown, partial fragments, plans, results, limits, and profile counters, but lack the three multi-fragment boundary cases described inline. The fixture checker verifies row/index/fragment coverage.
- Test results: Static inspection only. No builds, tests, or product-source edits were performed under the review contract; inspected assertions and author/CI claims are not independent test results.
- Observability: Existing plan text and Lance profile counters expose segment searches, candidate rows, and fallback counts for these paths. No separate missing production metric or logging issue was substantiated.
- Transactions and persistence: Query changes are read-only and add no EditLog or transactional write. Fixture publication and the Lance installation fingerprint/publication order were reviewed; no additional persistence or crash-atomicity issue survived.
- Data writes: Only test fixture generation and third-party archive/header installation write data; their checked marker and fixture-validation paths provide the relevant retry/consistency boundaries. No table-write path changed.
- FE-BE variables: Existing Substrait filter and use_scalar_index transport were followed from FE split planning to BE scanner setup, including disabled-index fallback; no new transmitted variable requires another send path.
- Performance and other invariants: The inline findings cover grouped full scans, redundant per-fragment searches, and loss of a valid native index query. Supported array NULL semantics, error/fallback paths, memory/ownership, and data visibility yielded no further substantiated issue in this read-only change.
Review is complete on this exact head. The accepted new inline set is the three comments below; no duplicate comment was reposted.
| if (op == CompoundPredicate.Operator.AND) { | ||
| // A refine-only conjunct anywhere below OR makes native reject the | ||
| // union, even if its sibling could independently drive this index. | ||
| if (requireExact && (left.isEmpty() || right.isEmpty())) { |
There was a problem hiding this comment.
[P2] Reject OR branches with an unindexed conjunct before grouping the segment. With only key indexed, (key = 1 AND other = 3) OR (key = 2 AND other = 4) gives both AND branches nonempty field sets here, so FE makes one task covering every fragment in the key segment. Lance treats each other equality as a refine expression and rejects the OR index query, so lance-c falls back to a full scan on one BE. Check exact eligibility against the selected index, or keep fragment splits for this shape; cover distinct other values in a multi-fragment test.
| } | ||
| if (expr instanceof BinaryPredicate) { | ||
| BinaryPredicate.Operator op = ((BinaryPredicate) expr).getOp(); | ||
| return (op == BinaryPredicate.Operator.EQ || op == BinaryPredicate.Operator.GT |
There was a problem hiding this comment.
[P2] Include pushed boolean and null-safe equality filters in segment driver selection. WHERE flag on an indexed boolean column and WHERE key <=> 5 on an indexed key are translated to native indexable equality filters, but this check rejects their original SlotRef/EQ_FOR_NULL forms. FE then emits one fragment task per fragment with native indexing still enabled, repeating the logical index search instead of searching the segment once. The previous slot collector grouped these filters; cover both shapes with a multi-fragment index.
| } | ||
| // Missing metadata or an ambiguous index name is not evidence against native | ||
| // index use. Disable only known expensive shapes, independent of FE discovery. | ||
| return exceedsExpressionBudget(pushedConjuncts) |
There was a problem hiding this comment.
[P2] Apply this budget to the index query rather than every pushed conjunct. With a multi-fragment key BTree index, key = 1 plus 64 equalities on unindexed columns counts as 129 FE nodes, so this condition disables scalar indexing on every fragment. Pinned Lance keeps only the key equality in filter_plan.index_query; the other predicates are refinements, and native's 128-node segment budget would see one node. Keep the valid driver or preserve native index use on the fragment splits, and cover this boundary with one indexed field and many residual conjuncts.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
What problem does this PR solve?
Ordinary Lance scans currently retain array label membership predicates in Doris, and scalar index planning misses usable boolean drivers. This adds Substrait pushdown for built-in
array_containson a directList<Utf8>column with a non-NULL constant string, and for nonempty constant-stringarrays_overlap(including the normal Nereids rewrite of three or more membership disjuncts). Positive indexable boolean conditions can select a LabelList/BTree/Bitmap segment. OR requires usable drivers on both branches of the same selected index. Literal starts_with prefixes and LIKE filters with a usable leading prefix remain eligible for BTree planning. OR branches requiring a refine filter retain fragment scans. Missing FE field/index metadata and competing logical indexes on one column preserve native automatic index selection and fragment ownership. For example, two label conditions joined by AND now reach Lance together and can reduce the index candidate set before row materialization.NULL needles, other array types, dynamic operands, UDFs, and
array_contains_allremain residual. Dorisarray_contains_alltests a contiguous ordered subsequence, so translating it to Lance set containment would change results. Each task still selects one logical index; this change does not implement multi-column index intersection. Fragment coverage, residual evaluation, and limit handling are preserved. Complement-only predicates and expressions exceeding the pinned native node/depth budget use parallel fragment scans with scalar-index use disabled. An independent positive condition retains one search per index segment even when another conjunct is a complement. This avoids repeated segment-wide searches and prevents large overlap or unindexable OR filters from serializing fallback scans.Dependency: lance-format/lance-c#93, merged in upstream main at
cd63420bfbe27f6f0a1edcc873b9191af7d52852, which is now the pinned dependency. The existing Foyer payload is reapplied without implementation changes because lance-c#73 is still open and supplies cache APIs used by Doris. Remove that remaining patch after #73 merges. The build now records and validates the installed Lance source/archive/patch fingerprint, detects incomplete publication, and refuses stale external build output. Before removing an existing installation, it verifies required rebuild inputs and rejects external definitions that differ from the checkout. The third-party CI script job runs the installation harness under the existing workflow path filters. These third-party/build changes are synchronized to #68689 for master. This PR targetsbranch-4.1.Documentation: apache/doris-website#4184 (English and Chinese; draft pending implementation).
Validation
Release note
Support ordinary Lance array-label membership pushdown and safe boolean scalar index planning.
Check List (For Author)
Check List (For Reviewer who merge this PR)