Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
1 similar comment
|
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.
Static review at exact head 3606a1a2af1b2a3632f0c6d57e89e44959f55205. I found six distinct issues: three can return wrong results or query the wrong index (selector collision, outer-join SEARCH(NULL), and analyzer-selected EXACT), one newly rejects a valid passthrough alias, one rejects equivalent analyzer spellings, and one evaluates the same MATCH virtual expression twice.
Checkpoint conclusions: the change is focused on the necessary analyzer, rewrite, expression, and translation stages, and the changed tests broadly cover direct/VARIANT OLAP joins, MOW behavior, TERM/PHRASE analyzers, and typed VARIANT TopN. They do not cover the six inline triggers. No new shared-state concurrency, lifecycle/static initialization, configuration, persistence/EditLog, transaction/write, or storage-format mechanism is introduced. Existing field-binding/index-properties transport remains positionally aligned for ordinary and VARIANT slots, but it cannot preserve explicit-selector intent for EXACT as implemented. Join types, ExprIds/nullability, predicate movement, top-down reentry, direct/residual paths, options fields, generated outputs, ordering, and negative-test form were checked; the duplicate virtual MATCH is the only additional performance issue. Existing diagnostics are otherwise sufficient.
This was a static-only review as required: I did not run builds or tests. The author reports validation on an older development baseline, and current exact-head CI is separate from that claim. There was no additional user focus and no existing inline review thread to duplicate.
| while (analyzerSeparator > 0 && bindingName.charAt(analyzerSeparator - 1) == '\\') { | ||
| analyzerSeparator = bindingName.lastIndexOf('@', analyzerSeparator - 1); | ||
| } | ||
| if (analyzerSeparator >= 0 && findSlotByName(bindingName, scan) == null) { |
There was a problem hiding this 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.
| } | ||
| String fieldKey = normalizedFieldName.toLowerCase(Locale.ROOT); | ||
| if (fieldAnalyzers.containsKey(fieldKey) | ||
| && !Objects.equals(fieldAnalyzers.get(fieldKey), binding.getAnalyzerName())) { |
There was a problem hiding this 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.
| originalFieldName, search.getDslString())); | ||
| } | ||
| checkInvertedIndexExists(scan.getTable(), slot.getName(), search.getDslString(), false); | ||
| checkInvertedIndexExists(tableForSlot(slot, scan), slot.getName(), search.getDslString(), false); |
There was a problem hiding this 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.
| for (Expression child : children) { | ||
| if (!(child instanceof SlotReference || child instanceof ElementAt)) { | ||
| if (!(child instanceof SlotReference || child instanceof ElementAt | ||
| || child instanceof NullLiteral)) { |
There was a problem hiding this 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.
| return null; | ||
| } | ||
| List<NamedExpression> projects = new ArrayList<>(project.getProjects()); | ||
| projects.add(result.second); |
There was a problem hiding this 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.
| Column column = slot.getOriginalColumn().orElse(null); | ||
| if (column != null) { | ||
| invertedIndex = olapTbl.getInvertedIndex(column, slot.getSubPath()); | ||
| invertedIndex = olapTbl.getInvertedIndex(column, slot.getSubPath(), analyzer); |
There was a problem hiding this 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.
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16789 ms |
TPC-DS: Total hot run time: 81601 ms |
ClickBench: Total hot run time: 14.85 s |
2 participants
What problem does this PR solve?
SEARCH in WHERE is rejected when its input contains an OLAP join. Residual predicates such as
(MATCH AND EXISTS (...)) OR joined_column = ...can also reach execution without an inverted-index evaluation path. Filtering the scan by MATCH alone would incorrectly remove rows selected by the other OR branch.This PR binds SEARCH field dependencies before pruning and predicate movement, and extends the existing scan virtual-column rule to materialize MATCH/SEARCH booleans used by projections, residual filters, and join conditions. The original SQL boolean expression and join multiplicity are preserved. This includes WHERE predicates moved into INNER JOIN conditions by the optimizer.
It also supports per-field analyzer selection, for example
search('name@exact:"John Smith" AND title@text:software'), including selectors in thefieldsoption. Selected index properties use the existing FE/BE interface. Quoted literal@field names remain supported.Each SEARCH expression still references one table instance; separate SEARCH expressions can be combined across tables using SQL AND/OR. SEARCH across an outer join's null-generating side remains conservatively gated. Explicit SEARCH projections/ON clauses, tuple IN subqueries, analyzer-IN, and multiple analyzers for the same field within one SEARCH are outside this change.
No BE code, storage format, Thrift, or new plan-node changes are included. Typed VARIANT TopN uses existing lazy materialization; sorting an untyped VARIANT value still requires an explicit cast.
Release note
Support SEARCH predicates in OLAP join queries and per-field analyzer selection, and evaluate residual MATCH/SEARCH expressions through indexed scan virtual columns.
Check List (For Author)
Validation on the original development baseline
16ab0566e9796d0498e6d2b3221e2a59d5e94ef7with these changes:test_crm_search_join_document,test_crm_search_analyzers,test_crm_search_variant_topn,test_search_usage_restrictions,test_search_null_semantics,test_search_variant_subcolumn_analyzer, andtest_match_projection_virtual_column.match_any not support execute_matchfor the OR/EXISTS control query; enabling it returned the expected five rows.For this PR, only the two feature/test commits were cherry-picked onto master
df36e174b99557a004bc3ad57faa019da4a7d91f. Range comparison shows unchanged test changes and only surrounding Analyzer context differences. Tests have not been rerun on this rebased master head; the draft records that validation boundary explicitly. No C++ files changed, so clang-format is not applicable.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)