github-actions[bot] commented on code in PR #68687:
URL: https://github.com/apache/doris/pull/68687#discussion_r4151995496
##########
.github/workflows/build-thirdparty.yml:
##########
@@ -60,6 +60,7 @@ jobs:
- 'thirdparty/**'
- 'env.sh'
focused_test_changes:
+ - 'build.sh'
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScanPlanner.java:
##########
@@ -195,7 +195,9 @@ private List<Split>
createNormalFragmentSplits(LanceTableMetadata metadata,
} else {
plan = new LanceSplitBuilder(metadata.getDatasetUri(),
metadata.getVersion(), 0);
}
- 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());
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScalarIndexPlanner.java:
##########
@@ -53,23 +67,31 @@ static Plan plan(LanceTableMetadata metadata, List<Expr>
pushedConjuncts,
|| !metadata.getIndexMetadataState().canPlanIndexSegments()) {
return null;
}
- Set<Integer> filterFields = collectFilterFields(metadata,
pushedConjuncts);
- if (filterFields.isEmpty()) {
- return null;
+ int nodes = Math.max(0, pushedConjuncts.size() - 1);
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScalarIndexPlanner.java:
##########
@@ -80,26 +102,125 @@ static Plan plan(LanceTableMetadata metadata, List<Expr>
pushedConjuncts,
return selected;
}
- private static Set<Integer> collectFilterFields(LanceTableMetadata
metadata, List<Expr> pushedConjuncts) {
- Set<SlotRef> slots = new HashSet<>();
- pushedConjuncts.forEach(expr -> collectDriverSlots(expr, slots));
+ private static Set<Integer> collectFilterFields(LanceTableMetadata
metadata,
+ List<Expr> pushedConjuncts, IndexType indexType) {
+ Set<Integer> fields = new HashSet<>();
+ for (Expr expr : pushedConjuncts) {
+ fields.addAll(collectDriverFields(metadata, expr, indexType));
+ }
+ return fields;
+ }
+
+ private static Set<Integer> collectDriverFields(LanceTableMetadata
metadata, Expr expr, IndexType indexType) {
Set<Integer> fields = new HashSet<>();
+ if (expr instanceof CompoundPredicate) {
+ CompoundPredicate.Operator op = ((CompoundPredicate) expr).getOp();
+ if (op == CompoundPredicate.Operator.NOT) {
+ return fields;
+ }
+ Set<Integer> left = collectDriverFields(metadata,
expr.getChild(0), indexType);
+ Set<Integer> right = collectDriverFields(metadata,
expr.getChild(1), indexType);
+ if (op == CompoundPredicate.Operator.AND) {
+ left.addAll(right);
+ } 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);
+ }
+ return left;
+ }
+ if (!isPositiveIndexLeaf(expr, indexType)) {
+ return fields;
+ }
+ Set<SlotRef> slots = new HashSet<>();
+ expr.collect(SlotRef.class, slots);
for (SlotRef slot : slots) {
metadata.getLanceFieldId(slot.getColumnName()).ifPresent(fields::add);
}
return fields;
}
- private static void collectDriverSlots(Expr expr, Set<SlotRef> slots) {
- // A predicate below OR or NOT is not a necessary condition of the
whole filter.
- // Do not select its index and then force every task into a
non-indexed fallback.
+ private static boolean isPositiveIndexLeaf(Expr expr, IndexType indexType)
{
+ if (indexType == IndexType.LABEL_LIST) {
+ if (!(expr instanceof FunctionCallExpr) ||
expr.getChildren().size() != 2) {
+ return false;
+ }
+ String name = ((FunctionCallExpr) expr).getFnName().getFunction();
+ if ("array_contains".equalsIgnoreCase(name)) {
+ return expr.getChild(0) instanceof SlotRef && expr.getChild(1)
instanceof LiteralExpr;
+ }
+ return "arrays_overlap".equalsIgnoreCase(name) &&
overlapSize(expr) > 0;
+ }
+ if (expr instanceof BinaryPredicate) {
+ BinaryPredicate.Operator op = ((BinaryPredicate) expr).getOp();
+ return (op == BinaryPredicate.Operator.EQ || op ==
BinaryPredicate.Operator.GT
+ || op == BinaryPredicate.Operator.GE || op ==
BinaryPredicate.Operator.LT
+ || op == BinaryPredicate.Operator.LE)
+ && ((expr.getChild(0) instanceof SlotRef &&
expr.getChild(1) instanceof LiteralExpr)
+ || (expr.getChild(1) instanceof SlotRef &&
expr.getChild(0) instanceof LiteralExpr));
+ }
+ if (expr instanceof InPredicate) {
+ return !((InPredicate) expr).isNotIn() && expr.getChild(0)
instanceof SlotRef;
+ }
+ if (expr instanceof IsNullPredicate) {
+ return !((IsNullPredicate) expr).isNotNull() && expr.getChild(0)
instanceof SlotRef;
+ }
+ // Bitmap has no prefix-query support. Keep only unambiguous BTree
prefixes;
+ // escaped and other LIKE shapes remain pushed filters in fragment
scans.
+ if (indexType != IndexType.BTREE || expr.getChildren().size() != 2
+ || !(expr.getChild(0) instanceof SlotRef) ||
!(expr.getChild(1) instanceof StringLiteral)) {
+ return false;
+ }
+ String name = expr instanceof FunctionCallExpr
+ ? ((FunctionCallExpr) expr).getFnName().getFunction() : "";
+ boolean like = (expr instanceof LikePredicate && ((LikePredicate)
expr).getOp() == LikePredicate.Operator.LIKE)
+ || "like".equalsIgnoreCase(name);
+ String prefix = ((StringLiteral) expr.getChild(1)).getStringValue();
+ if (like && prefix.endsWith("%")) {
+ prefix = prefix.substring(0, prefix.length() - 1);
+ } else if (!"starts_with".equalsIgnoreCase(name)) {
+ return false;
+ }
+ return !prefix.isEmpty() && prefix.indexOf('%') < 0 &&
prefix.indexOf('_') < 0 && prefix.indexOf('\\') < 0;
+ }
+
+ private static int overlapSize(Expr expr) {
+ if (!(expr instanceof FunctionCallExpr) || expr.getChildren().size()
!= 2
+ || !"arrays_overlap".equalsIgnoreCase(((FunctionCallExpr)
expr).getFnName().getFunction())) {
+ return 0;
+ }
+ for (int i = 0; i < 2; i++) {
+ if (expr.getChild(i) instanceof ArrayLiteral && expr.getChild(1 -
i) instanceof SlotRef) {
+ return expr.getChild(i).getChildren().size();
+ }
+ }
+ return 0;
+ }
+
+ private static int expressionNodes(Expr expr, int depth) {
+ if (depth > MAX_EXPRESSION_DEPTH) {
+ return MAX_EXPRESSION_NODES + 1;
+ }
if (expr instanceof CompoundPredicate) {
- if (((CompoundPredicate) expr).getOp() ==
CompoundPredicate.Operator.AND) {
- expr.getChildren().forEach(child -> collectDriverSlots(child,
slots));
+ int nodes = 1;
+ for (Expr child : expr.getChildren()) {
+ nodes += expressionNodes(child, depth + 1);
+ if (nodes > MAX_EXPRESSION_NODES) {
+ break;
+ }
+ }
+ return nodes;
+ }
+ int labels = overlapSize(expr);
+ // The converter emits a balanced OR tree: N memberships require 2*N-1
nodes.
+ if (labels > 0) {
+ int levels = 32 - Integer.numberOfLeadingZeros(labels - 1);
+ if (labels > MAX_EXPRESSION_NODES / 2 || depth + levels >
MAX_EXPRESSION_DEPTH) {
+ return MAX_EXPRESSION_NODES + 1;
}
- } else {
- expr.collect(SlotRef.class, slots);
+ return 2 * labels - 1;
}
+ return 1;
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScalarIndexPlanner.java:
##########
@@ -80,26 +102,125 @@ static Plan plan(LanceTableMetadata metadata, List<Expr>
pushedConjuncts,
return selected;
}
- private static Set<Integer> collectFilterFields(LanceTableMetadata
metadata, List<Expr> pushedConjuncts) {
- Set<SlotRef> slots = new HashSet<>();
- pushedConjuncts.forEach(expr -> collectDriverSlots(expr, slots));
+ private static Set<Integer> collectFilterFields(LanceTableMetadata
metadata,
+ List<Expr> pushedConjuncts, IndexType indexType) {
+ Set<Integer> fields = new HashSet<>();
+ for (Expr expr : pushedConjuncts) {
+ fields.addAll(collectDriverFields(metadata, expr, indexType));
+ }
+ return fields;
+ }
+
+ private static Set<Integer> collectDriverFields(LanceTableMetadata
metadata, Expr expr, IndexType indexType) {
Set<Integer> fields = new HashSet<>();
+ if (expr instanceof CompoundPredicate) {
+ CompoundPredicate.Operator op = ((CompoundPredicate) expr).getOp();
+ if (op == CompoundPredicate.Operator.NOT) {
+ return fields;
+ }
+ Set<Integer> left = collectDriverFields(metadata,
expr.getChild(0), indexType);
+ Set<Integer> right = collectDriverFields(metadata,
expr.getChild(1), indexType);
+ if (op == CompoundPredicate.Operator.AND) {
+ left.addAll(right);
+ } 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);
+ }
+ return left;
+ }
+ if (!isPositiveIndexLeaf(expr, indexType)) {
+ return fields;
+ }
+ Set<SlotRef> slots = new HashSet<>();
+ expr.collect(SlotRef.class, slots);
for (SlotRef slot : slots) {
metadata.getLanceFieldId(slot.getColumnName()).ifPresent(fields::add);
}
return fields;
}
- private static void collectDriverSlots(Expr expr, Set<SlotRef> slots) {
- // A predicate below OR or NOT is not a necessary condition of the
whole filter.
- // Do not select its index and then force every task into a
non-indexed fallback.
+ private static boolean isPositiveIndexLeaf(Expr expr, IndexType indexType)
{
+ if (indexType == IndexType.LABEL_LIST) {
+ if (!(expr instanceof FunctionCallExpr) ||
expr.getChildren().size() != 2) {
+ return false;
+ }
+ String name = ((FunctionCallExpr) expr).getFnName().getFunction();
+ if ("array_contains".equalsIgnoreCase(name)) {
+ return expr.getChild(0) instanceof SlotRef && expr.getChild(1)
instanceof LiteralExpr;
+ }
+ return "arrays_overlap".equalsIgnoreCase(name) &&
overlapSize(expr) > 0;
+ }
+ if (expr instanceof BinaryPredicate) {
+ BinaryPredicate.Operator op = ((BinaryPredicate) expr).getOp();
+ return (op == BinaryPredicate.Operator.EQ || op ==
BinaryPredicate.Operator.GT
+ || op == BinaryPredicate.Operator.GE || op ==
BinaryPredicate.Operator.LT
+ || op == BinaryPredicate.Operator.LE)
+ && ((expr.getChild(0) instanceof SlotRef &&
expr.getChild(1) instanceof LiteralExpr)
+ || (expr.getChild(1) instanceof SlotRef &&
expr.getChild(0) instanceof LiteralExpr));
+ }
+ if (expr instanceof InPredicate) {
+ return !((InPredicate) expr).isNotIn() && expr.getChild(0)
instanceof SlotRef;
+ }
+ if (expr instanceof IsNullPredicate) {
+ return !((IsNullPredicate) expr).isNotNull() && expr.getChild(0)
instanceof SlotRef;
+ }
+ // Bitmap has no prefix-query support. Keep only unambiguous BTree
prefixes;
+ // escaped and other LIKE shapes remain pushed filters in fragment
scans.
+ if (indexType != IndexType.BTREE || expr.getChildren().size() != 2
+ || !(expr.getChild(0) instanceof SlotRef) ||
!(expr.getChild(1) instanceof StringLiteral)) {
+ return false;
+ }
+ String name = expr instanceof FunctionCallExpr
+ ? ((FunctionCallExpr) expr).getFnName().getFunction() : "";
+ boolean like = (expr instanceof LikePredicate && ((LikePredicate)
expr).getOp() == LikePredicate.Operator.LIKE)
+ || "like".equalsIgnoreCase(name);
+ String prefix = ((StringLiteral) expr.getChild(1)).getStringValue();
+ if (like && prefix.endsWith("%")) {
+ prefix = prefix.substring(0, prefix.length() - 1);
+ } else if (!"starts_with".equalsIgnoreCase(name)) {
+ return false;
+ }
+ return !prefix.isEmpty() && prefix.indexOf('%') < 0 &&
prefix.indexOf('_') < 0 && prefix.indexOf('\\') < 0;
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScalarIndexPlanner.java:
##########
@@ -80,26 +102,125 @@ static Plan plan(LanceTableMetadata metadata, List<Expr>
pushedConjuncts,
return selected;
}
- private static Set<Integer> collectFilterFields(LanceTableMetadata
metadata, List<Expr> pushedConjuncts) {
- Set<SlotRef> slots = new HashSet<>();
- pushedConjuncts.forEach(expr -> collectDriverSlots(expr, slots));
+ private static Set<Integer> collectFilterFields(LanceTableMetadata
metadata,
+ List<Expr> pushedConjuncts, IndexType indexType) {
+ Set<Integer> fields = new HashSet<>();
+ for (Expr expr : pushedConjuncts) {
+ fields.addAll(collectDriverFields(metadata, expr, indexType));
+ }
+ return fields;
+ }
+
+ private static Set<Integer> collectDriverFields(LanceTableMetadata
metadata, Expr expr, IndexType indexType) {
Set<Integer> fields = new HashSet<>();
+ if (expr instanceof CompoundPredicate) {
+ CompoundPredicate.Operator op = ((CompoundPredicate) expr).getOp();
+ if (op == CompoundPredicate.Operator.NOT) {
+ return fields;
+ }
+ Set<Integer> left = collectDriverFields(metadata,
expr.getChild(0), indexType);
+ Set<Integer> right = collectDriverFields(metadata,
expr.getChild(1), indexType);
+ if (op == CompoundPredicate.Operator.AND) {
+ left.addAll(right);
+ } 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);
Review 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.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
