github-actions[bot] commented on code in PR #67932:
URL: https://github.com/apache/doris/pull/67932#discussion_r4005450775
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/RewriteSearchToSlots.java:
##########
@@ -103,11 +108,27 @@ private Expression rewriteSearch(Search search,
LogicalOlapScan scan) {
}
Map<String, String> normalizedFields = new HashMap<>();
+ Map<String, String> fieldAnalyzers = new HashMap<>();
+ Set<List<String>> qualifiers = new HashSet<>();
// Create slot reference children from field bindings
List<Expression> slotChildren = new ArrayList<>();
for (SearchDslParser.QsFieldBinding binding :
qsPlan.getFieldBindings()) {
- String originalFieldName = binding.getFieldName();
+ String bindingName = binding.getFieldName();
+ String originalFieldName = bindingName;
+ int analyzerSeparator = bindingName.lastIndexOf('@');
+ while (analyzerSeparator > 0 &&
bindingName.charAt(analyzerSeparator - 1) == '\\') {
+ analyzerSeparator = bindingName.lastIndexOf('@',
analyzerSeparator - 1);
+ }
+ if (analyzerSeparator >= 0 && findSlotByName(bindingName,
scan) == null) {
Review Comment:
[P1] Preserve analyzer-selector syntax before schema lookup. For an unquoted
`name@english` the parser leaves `@` unescaped, but this branch treats it as a
selector only when a slot named `name@english` does not exist. If a table has
both indexed `name` and a literal ``name@english`` column, the query silently
binds the literal column and its index instead of applying the `english`
analyzer to `name`, so it can return different rows. Please carry
quoted/unquoted provenance through parsing and split every unquoted selector
independently of slot collisions, with a collision regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/RewriteSearchToSlots.java:
##########
@@ -156,12 +177,26 @@ private Expression rewriteSearch(Search search,
LogicalOlapScan scan) {
"Field '%s' not found in table for search: %s",
originalFieldName, search.getDslString()));
}
- checkInvertedIndexExists(scan.getTable(), slot.getName(),
search.getDslString(), false);
+ checkInvertedIndexExists(tableForSlot(slot, scan),
slot.getName(), search.getDslString(), false);
Review Comment:
[P1] Check the physical original column rather than the visible alias. For a
passthrough such as `(SELECT content AS body FROM t) s`, `findSlotByName`
returns `body` and that slot retains
`originalTable=t`/`originalColumn=content`, but this call asks `t` for a column
named `body` and falsely reports that no index exists. The VARIANT parent path
has the same alias issue. Please use the slot's original column (plus subpath
where applicable) for index validation while retaining the alias only for DSL
binding, and add renamed-output regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/RewriteSearchToSlots.java:
##########
@@ -156,12 +177,26 @@ private Expression rewriteSearch(Search search,
LogicalOlapScan scan) {
"Field '%s' not found in table for search: %s",
originalFieldName, search.getDslString()));
}
- checkInvertedIndexExists(scan.getTable(), slot.getName(),
search.getDslString(), false);
+ checkInvertedIndexExists(tableForSlot(slot, scan),
slot.getName(), search.getDslString(), false);
childExpr = slot;
normalizedFieldName = slot.getName();
}
- normalizedFields.put(originalFieldName, normalizedFieldName);
+ for (Slot input : childExpr.getInputSlots()) {
+ qualifiers.add(input.getQualifier());
+ }
+ if (qualifiers.size() > 1) {
+ throw new AnalysisException("Each SEARCH expression must
reference fields from one table; "
+ + "combine separate SEARCH expressions with SQL
AND/OR");
+ }
+ String fieldKey = normalizedFieldName.toLowerCase(Locale.ROOT);
+ if (fieldAnalyzers.containsKey(fieldKey)
+ && !Objects.equals(fieldAnalyzers.get(fieldKey),
binding.getAnalyzerName())) {
Review Comment:
[P2] Compare analyzer identities with the same normalization used for index
lookup. `isAnalyzerMatched` accepts analyzer names case-insensitively, so both
`name@CRM_DOC_TEXT` and `name@crm_doc_text` resolve to the same index, but this
`Objects.equals` check then rejects them as two analyzers for one field.
Normalize with `Locale.ROOT` (or compare case-insensitively) and cover
mixed-case spellings in one DSL.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownMatchProjectionAsVirtualColumn.java:
##########
@@ -124,13 +141,150 @@ private LogicalProject<?> pushDown(
newProjections.build(), childRebuilder.apply(newScan));
}
+ private boolean isIndexSearch(Expression expression) {
+ return expression instanceof Match || expression instanceof
SearchExpression;
+ }
+
+ private Plan pushDownResidual(Plan plan) {
+ Plan child = plan.child(0);
+ Map<Expression, Expression> replacements = new LinkedHashMap<>();
+ for (Expression expression : plan.getExpressions()) {
+ for (Expression search : expression.<Expression>collect(e ->
isIndexSearch((Expression) e))) {
+ if (replacements.containsKey(search)) {
+ continue;
+ }
+ Pair<Plan, Slot> result = materialize(search, child);
+ if (result != null) {
+ child = result.first;
+ replacements.put(search, result.second);
+ }
+ }
+ }
+ if (replacements.isEmpty()) {
+ return null;
+ }
+ if (plan instanceof LogicalFilter) {
+ Set<Expression> conjuncts = new LinkedHashSet<>();
+ for (Expression expression : ((LogicalFilter<?>)
plan).getConjuncts()) {
+ conjuncts.add(ExpressionUtils.replace(expression,
replacements));
+ }
+ // Hide additional scan values from the original filter's
consumers.
+ return new LogicalProject<>(ImmutableList.copyOf(plan.getOutput()),
+ new LogicalFilter<>(conjuncts, child));
+ }
+ LogicalProject<?> project = (LogicalProject<?>) plan;
+ List<NamedExpression> projects = new ArrayList<>();
+ for (NamedExpression expression : project.getProjects()) {
+ projects.add((NamedExpression) ExpressionUtils.replace(expression,
replacements));
+ }
+ return project.withProjectsAndChild(projects, child);
+ }
+
+ private Plan pushDownJoin(LogicalJoin<?, ?> join) {
+ List<Plan> children = new ArrayList<>(join.children());
+ Map<Expression, Expression> replacements = new LinkedHashMap<>();
+ for (Expression expression : join.getExpressions()) {
+ for (Expression search : expression.<Expression>collect(e ->
isIndexSearch((Expression) e))) {
+ if (replacements.containsKey(search)) {
+ continue;
+ }
+ for (int side = 0; side < children.size(); side++) {
+ // Join conditions consume child values before this join's
NULL extension.
+ // This also handles WHERE predicates moved into an inner
join by rewriting.
+ Pair<Plan, Slot> result = materialize(search,
children.get(side));
+ if (result != null) {
+ children.set(side, result.first);
+ replacements.put(search, result.second);
+ break;
+ }
+ }
+ }
+ }
+ if (replacements.isEmpty()) {
+ return null;
+ }
+ Plan rewritten = join.withConjunctsChildren(
+ ExpressionUtils.replace(join.getHashJoinConjuncts(),
replacements),
+ ExpressionUtils.replace(join.getOtherJoinConjuncts(),
replacements),
+ ExpressionUtils.replace(join.getMarkJoinConjuncts(),
replacements),
+ children.get(0), children.get(1),
join.getJoinReorderContext());
+ return new LogicalProject<>(ImmutableList.copyOf(join.getOutput()),
rewritten);
+ }
+
+ private Pair<Plan, Slot> materialize(Expression expression, Plan plan) {
+ Set<Slot> inputs = expression.getInputSlots();
+ if (inputs.isEmpty() || !plan.getOutputSet().containsAll(inputs)) {
+ return null;
+ }
+ if (plan instanceof LogicalOlapScan) {
+ LogicalOlapScan scan = (LogicalOlapScan) plan;
+ if (!canPushDown(scan)) {
+ return null;
+ }
+ for (NamedExpression column : scan.getVirtualColumns()) {
+ if (column instanceof Alias && ((Alias)
column).child().equals(expression)) {
+ return Pair.of(scan, column.toSlot());
+ }
+ }
+ Alias alias = new Alias(expression);
+ return Pair.of(scan.appendVirtualColumns(ImmutableList.of(alias)),
alias.toSlot());
+ }
+ if (plan instanceof LogicalProject) {
+ LogicalProject<?> project = (LogicalProject<?>) plan;
+ if (project.containsNoneMovableFunction()) {
+ return null;
+ }
+ Expression rewritten = ExpressionUtils.replace(expression,
project.getAliasToProducer());
+ Pair<Plan, Slot> result = materialize(rewritten, project.child());
+ if (result == null) {
+ return null;
+ }
+ List<NamedExpression> projects = new
ArrayList<>(project.getProjects());
+ projects.add(result.second);
Review Comment:
[P2] Record or reuse the materialization before leaving this project. If the
same MATCH appears in this child projection and a preserved-side outer-join
`ON` condition, `pushDownJoin` first reaches this path and appends one virtual
slot; top-down traversal then reaches the rebuilt project, where the direct
Project-to-scan rule allocates a second alias because it does not consult the
scan's existing virtual columns. Both ExprIds stay referenced and the segment
iterator evaluates/materializes the predicate twice. Please centralize the
reuse check and assert this plan has one virtual MATCH column.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/SearchExpression.java:
##########
@@ -72,11 +74,14 @@ public boolean foldable() {
@Override
public SearchExpression withChildren(List<Expression> children) {
- // Validate that all children are SlotReference or ElementAt (for
variant subcolumns)
+ // Null-rejection inference temporarily replaces input slots with NULL.
+ // Such symbolic expressions are not execution-time field bindings.
for (Expression child : children) {
- if (!(child instanceof SlotReference || child instanceof
ElementAt)) {
+ if (!(child instanceof SlotReference || child instanceof ElementAt
+ || child instanceof NullLiteral)) {
Review Comment:
[P1] Do not let inference-only NULL children persist into executable SEARCH
plans. For `a LEFT JOIN b ON FALSE` with `search('content:john') IS NULL` on
`b`, join elimination produces `NULL AS content` and filter-through-project
substitutes it here, yielding `Search(NULL)` directly over `a`'s scan. The
materializer skips it, the final Filter-to-scan check admits it, and BE's
no-iterator path produces empty data and null bitmaps, so SEARCH is
false/non-null and the preserved rows are wrongly rejected. Please keep
symbolic NULL replacement non-persistent or reject non-slot/subcolumn children
before translation, and cover false outer-join padding.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java:
##########
@@ -698,19 +698,25 @@ public Expr visitSearchExpression(SearchExpression
searchExpression,
// Look up the inverted index for each field (needed for variant
subcolumn analyzer)
Index invertedIndex = null;
+ String analyzer = searchExpression.getQsPlan().getFieldBindings()
+ .get(fieldIndexes.size()).getAnalyzerName();
if (slotExpr instanceof SlotReference) {
SlotReference slot = (SlotReference) slotExpr;
OlapTable olapTbl = getOlapTableDirectly(slot);
if (olapTbl != null) {
Column column = slot.getOriginalColumn().orElse(null);
if (column != null) {
- invertedIndex = olapTbl.getInvertedIndex(column,
slot.getSubPath());
+ invertedIndex = olapTbl.getInvertedIndex(column,
slot.getSubPath(), analyzer);
Review Comment:
[P1] Ensure this selected analyzer also constrains BE reader choice for
`EXACT`. FE resolves the requested index here and sends its properties, but
`FieldReaderResolver` derives `analyzer_key` only when the query type is not
`EQUAL_QUERY`; SEARCH maps `EXACT` to `EQUAL_QUERY`. Two custom
standard/keyword analyzers are both FULLTEXT readers, so the empty-key selector
can pick the lower index ID instead of the requested keyword analyzer and
return different rows. Please honor an explicit analyzer for every clause type
(then apply type preference within that analyzer) and add a two-analyzer EXACT
regression.
--
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]
