github-actions[bot] commented on code in PR #67289:
URL: https://github.com/apache/doris/pull/67289#discussion_r3957156051
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughVectorSearchTopN.java:
##########
@@ -23,33 +23,35 @@
import org.apache.doris.nereids.trees.plans.logical.LogicalFilter;
import org.apache.doris.nereids.trees.plans.logical.LogicalTVFRelation;
import org.apache.doris.nereids.trees.plans.logical.LogicalTopN;
+import org.apache.doris.tablefunction.FullTextSearchTableValuedFunction;
import org.apache.doris.tablefunction.VectorSearchTableValuedFunction;
/**
- * Move an outer vector_search WHERE predicate below its Doris merge TopN.
+ * Move an outer Lance external-search WHERE predicate below its Doris merge
TopN.
*
- * <p>The TopN immediately above a vector_search TVF is added by {@code
BindExpression} to merge
+ * <p>The TopN immediately above a search TVF is added by {@code
BindExpression} to merge
* the candidates returned by all Lance fragment scans. The SQL WHERE
predicate must therefore be
* evaluated below this TopN so it can become a residual conjunct on the Doris
Lance scan node:
*
* <pre>
* Filter TopN
* TopN -> Filter
- * vector_search vector_search
+ * search TVF search TVF
* </pre>
*
- * <p>This remains a postfilter relative to Lance nearest(): every fragment
first returns its ANN
- * candidates, and Doris filters those candidates before the local/global
TopN. It is deliberately
- * not converted into the Lance prefilter carried by the TVF's {@code filter}
property.
+ * <p>This remains a postfilter relative to the Lance search: every split
first returns candidates,
+ * and Doris filters those candidates before the local/global TopN. It is
deliberately not
+ * converted into the Lance prefilter carried by the TVF's {@code filter}
property.
*/
public class PushDownFilterThroughVectorSearchTopN extends
OneRewriteRuleFactory {
@Override
public Rule build() {
return logicalFilter(logicalTopN(logicalTVFRelation()))
.then(filter -> {
LogicalTopN<LogicalTVFRelation> topN = filter.child();
- if (!VectorSearchTableValuedFunction.NAME.equals(
- topN.child().getFunction().getName())) {
+ String functionName = topN.child().getFunction().getName();
+ if
(!VectorSearchTableValuedFunction.NAME.equals(functionName)
+ &&
!FullTextSearchTableValuedFunction.NAME.equals(functionName)) {
Review Comment:
[P1] Keep the outer WHERE above FTS's global TopN. Every physical FTS split
is already capped at `top_k + offset` before Doris evaluates this residual
filter. For `top_k=1`, if the highest-scoring row fails the predicate and the
runner-up passes, one segment returns only the rejected row (empty result),
while placing those rows in two physical segments returns both split-local
candidates and the runner-up survives. Thus index maintenance alone can change
the SQL result. Either leave a post-search WHERE above the synthetic
snapshot-wide TopN, or make it a true Lance prefilter/otherwise overfetch until
the global filtered TopN is complete; please cover the one-vs-two-segment case.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -561,6 +689,41 @@ Status LanceTableReader::_open_dataset(const DatasetKey&
key) {
return Status::OK();
}
+Status LanceTableReader::_prepare_fts_query_context() {
+ DORIS_CHECK(_dataset != nullptr);
+ DORIS_CHECK(_fts_query_context == nullptr);
+ DORIS_CHECK(_scan_params != nullptr);
+ const auto& full_text =
+
_scan_params->lance_scan_params.external_search_request.search_query.full_text_search;
+ if (full_text.__isset.global_statistics) {
+ return Status::NotSupported(
+ "Lance FE-provided FTS global statistics require a lance-c
consumer API");
+ }
+ const auto coverage_mode = full_text.coverage_mode ==
TFtsCoverageMode::STRICT
+ ? LANCE_FTS_COVERAGE_STRICT
+ : LANCE_FTS_COVERAGE_INDEX_ONLY;
+ // Keep statistics preparation at the reader/scanner lifetime today. A
future FE-provided
+ // opaque statistics payload should enter through this boundary and create
the same context,
+ // leaving segment-scoped scanner execution unchanged.
+ if (full_text.query_type == TFtsQueryType::MATCH) {
+ const auto match_operator = full_text.match_operator ==
TFtsMatchOperator::AND
+ ? LANCE_FTS_MATCH_OPERATOR_AND
+ : LANCE_FTS_MATCH_OPERATOR_OR;
+ _fts_query_context = lance_dataset_prepare_fts_match_query(
Review Comment:
[P1] Exclude snapshot-pruned documents from the global BM25 scorer. Lance
updates/rewrites remove changed fragments from `IndexMetadata.fragment_bitmap`
while retaining the physical scalar segment; its search prefilter blocks those
stale rows from output, but the prepared context used here builds `num_docs`,
`total_tokens`, and term document frequencies from every raw physical index
before that mask is applied. Stale documents can therefore change average
document length/IDF and even reverse the ranking of live rows in the pinned
snapshot. Please build statistics from each segment's effective current
fragment coverage (or pass snapshot-scoped global statistics through the
reserved contract), with an update/rewrite regression against a clean rebuilt
index.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -533,6 +657,10 @@ Status LanceTableReader::_ensure_dataset_open(const
TFileRangeDesc& range) {
return Status::InvalidArgument(
"Lance reader cannot mix dataset snapshots or storage
options");
}
+ if (_search_kind == SearchKind::FULL_TEXT && prepare_fts_context &&
Review Comment:
[P2] Share FTS preparation across local scanners. FileScanLocalState can
start up to 16 FileScannerV2 instances, each with its own LanceTableReader, so
this branch makes every active scanner independently open the same snapshot,
enumerate every committed FTS segment, and rebuild the corpus-wide BM25 context
before scanning its assigned split. With N segment splits (up to the scanner
cap), that becomes N whole-index preparations and up to N-by-N segment opens
for one BE query. Please introduce a query/scan-local owner for the exact
dataset snapshot and prepared context (or an equivalent shared-session cache),
then attach it to the local segment scanners; expose preparation time/count and
cover multi-scanner execution.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScanNode.java:
##########
@@ -321,12 +352,44 @@ private Optional<List<Split>>
createIndexSegmentSplits(LanceTableMetadata metada
return Optional.of(plan.buildSplits());
}
- private static List<LanceIndexSegmentInfo> selectIndexSegments(
- List<LanceIndexSegmentInfo> indexSegments, int vectorFieldId) {
+ private List<Split> createFullTextIndexSegmentSplits(LanceTableMetadata
metadata,
+ Map<Long, LanceFragmentInfo> visibleFragments) throws
UserException {
+ TFullTextSearchParams fullText =
+ externalSearchRequest.getSearchQuery().getFullTextSearch();
+ if (searchFieldId < 0) {
+ throw new UserException("Lance full-text column '" +
fullText.getColumn()
+ + "' has no field ID in the Lance schema");
+ }
+ List<LanceIndexSegmentInfo> matchingSegments =
selectFullTextIndexSegments(
+ metadata.getIndexSegments(), searchFieldId,
fullText.getColumn());
+ if (matchingSegments.isEmpty()) {
+ throw new UserException("No committed Lance FTS index exists for
column '"
+ + fullText.getColumn() + "' at dataset version " +
metadata.getVersion());
+ }
+ IndexSegmentSplitPlan plan = planIndexSegments(
+ metadata, matchingSegments, visibleFragments, true)
+ .orElseThrow(() -> new UserException("Lance FTS index for
column '"
Review Comment:
[P2] Return an empty result for INDEX_ONLY with no visible coverage.
`planIndexSegments()` filters segment coverage against the pinned snapshot and
returns `Optional.empty()` when all matching committed segments cover zero
visible fragments, so this unconditional `orElseThrow()` runs before the
coverage-mode branch. For `coverage_mode=index_only`, searching only the
indexed subset when that subset is empty should produce zero rows; the generic
file scan path supports zero splits and Lance-C explicitly supports an empty
selected FTS segment. As written, a valid query fails after indexed fragments
are replaced/compacted out of the snapshot. Please preserve the distinction
between 'no matching index' and 'matching index with no visible coverage',
return zero splits for INDEX_ONLY, and add a regression for this case.
--
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]
