github-actions[bot] commented on code in PR #68813:
URL: https://github.com/apache/doris/pull/68813#discussion_r4227447649
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LanceExternalSearchTableValuedFunction.java:
##########
@@ -196,6 +197,10 @@ protected static PreparedSearch prepareSearch(CommonSearch
common, int fieldId,
.setFormat(TSearchFilterFormat.SQL)
.setPayload(validateAndEncodeSqlFilter(common.params.get(FILTER))));
}
+ // Keep the search-filter timing common to vector and full-text TVFs.
Omission defaults
+ // to postfilter for both kinds of search.
Review Comment:
[P1] Preserve filter semantics during BE rolling upgrades. A new FE sends
`prefilter=false` for an omitted property, which an old BE ignores and still
prefilters; conversely, an old FE sends no field, which this new BE treats as
postfilter. With `top_k=3` and `filter=category = 'odd'`, one BE returns only
row 2 while the other returns rows 2, 4, and 6, so the same query changes with
split placement. Version or capability gate this semantic change on both sides,
or retain the old default until all eligible BEs understand it.
##########
be/test/format_v2/table/lance_reader_test.cpp:
##########
@@ -923,15 +923,15 @@ TEST(LanceTableReaderVectorSearchTest,
MultiVectorScoresFiltersOffsetsAndIndexed
}
// Read metrics after close: lance-c publishes its final
execution summary
// when the stream is released, including for an early
top-k stop.
- for (const char* name : {"LancePrefilterLoads",
"LancePrefilterInputRows",
- "LancePrefilterInputBatches",
"LancePrefilterRowIds",
- "LancePrefilterLoadTime",
"LancePrefilterInputTime",
- "LancePrefilterBuildTime"}) {
+ for (const char* name : {"RowIdPrefilterLoads",
"RowIdPrefilterInputRows",
+ "RowIdPrefilterInputBatches",
"RowIdPrefilterIds",
+ "RowIdPrefilterLoadTime",
"RowIdPrefilterInputTime",
Review Comment:
[P2] Set `prefilter=true` in the BE cases that still assert prefilter
behavior. This filtered test only sets `search_filter`, so the new BE passes
false to Lance, while the changed assertions require every row-ID prefilter
counter to be positive. The filtered TopK expectations in this file also assume
prefilter-before-ranking and will fail. Set the flag explicitly in those
existing cases and cover the new false/default path separately.
##########
thirdparty/patches/lance-c-foyer.patch:
##########
@@ -1003,32 +7293,20 @@ index
0000000000000000000000000000000000000000..cfc47f1528d11b6be068c09dd4703ff4
+ },
+ )
+ .await?;
-+ // HTTP responses carry transport extensions even for immutable
files.
-+ // Cache metadata independently; request-specific extensions are
never replayed.
++ // Cache only immutable metadata and attributes, never
request-specific state.
+ self.reader.cache.metadata.insert(
-+ metadata_key.clone(),
++ metadata_key,
+ (result.meta.clone(), result.attributes.clone()),
+ );
-+ (result.meta, result.attributes, result.extensions)
++ (result.meta, result.attributes)
+ };
+ let object_size = metadata.size;
-+ let size_bytes = object_size.to_le_bytes();
-+ // WriteOnInsertion enqueues disk I/O even for an identical value.
Look in
-+ // both tiers so warm reads, including recovered entries, remain
read-only.
Review Comment:
[P2] Skip the size-key insert on warm reads. This `cached_get` path is
reached by ordinary Lance range reads, and `WriteOnInsertion` enqueues a disk
write for the same 8-byte size record on every call, even when metadata and
data blocks are cached. The previous patch checked the memory and disk tiers
before inserting and had a zero-write warm-read test; retain that guard and
test to avoid persistent write amplification.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -1180,7 +1066,8 @@ void LanceTableReader::_collect_scan_statistics(void*
callback_ctx, const void*
update_counter(reader->_execution_iops, statistics->iops, "iops");
update_counter(reader->_execution_requests, statistics->requests,
"requests");
update_counter(reader->_execution_bytes_read, statistics->bytes_read,
"bytes_read");
- update_counter(reader->_index_partition_cache_miss_loads,
statistics->index_partitions_loaded,
+ update_counter(reader->_index_object_loads, statistics->indices_loaded,
"indices_loaded");
+ update_counter(reader->_index_components_loaded,
statistics->index_partitions_loaded,
"index_partitions_loaded");
update_counter(reader->_index_comparisons, statistics->index_comparisons,
"index_comparisons");
Review Comment:
[P2] Keep the counter tree stable across Lance scanner tasks. These groups
are now created only for metrics reported by each task, but FE
`RuntimeProfile.mergeCounters` traverses only the first task's child-counter
map. A vector query with an uncovered flat split and an indexed split can
therefore lose all `LanceVectorIndex` metrics in its aggregate Profile when the
flat task is first. Pre-register the hierarchy for every scanner or merge the
union of counter names, and cover the mixed-split aggregate.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -1197,38 +1084,20 @@ void LanceTableReader::_collect_scan_statistics(void*
callback_ctx, const void*
continue;
}
const std::string_view name(metric.name == nullptr ? "" : metric.name,
metric.name_len);
- RuntimeProfile::Counter* counter = nullptr;
- switch (metric.kind) {
- case LANCE_SCAN_METRIC_COUNT: {
- const auto found = reader->_lance_count_metrics.find(name);
- if (found != reader->_lance_count_metrics.end()) {
- counter = found->second;
+ for (const auto& definition : LANCE_SCAN_METRICS) {
+ if (definition.native_name != name) {
+ continue;
}
- break;
- }
- case LANCE_SCAN_METRIC_TIME_NANOSECONDS: {
- const auto found = reader->_lance_time_metrics.find(name);
- if (found != reader->_lance_time_metrics.end()) {
- counter = found->second;
- } else if (name == "search_time") {
- // Scalar-index metrics exist only when Lance includes the
corresponding
- // execution node in this scan plan.
- counter = ADD_CHILD_TIMER_WITH_LEVEL(reader->_scanner_profile,
Review Comment:
[P2] Keep metric allocation out of the Lance statistics callback.
`add_lance_counter` now allocates counters and map entries for every first-seen
metric at EOF; an allocation failure can throw through
`LanceScanStatisticsCallback`, whose ABI requires callbacks to return normally,
potentially aborting the BE. Pre-register these metrics before handing the
callback to Lance, or catch allocation failures inside the callback and skip
best-effort profiling.
##########
thirdparty/patches/lance-c-foyer.patch:
##########
@@ -547,32 +6861,9 @@ index
0000000000000000000000000000000000000000..cfc47f1528d11b6be068c09dd4703ff4
+use crate::runtime::block_on;
+use crate::session::{LanceSession, session_new_with_data_cache_factory};
+
-+const CACHE_KEY_VERSION: &str = "lance-data-v2";
++const CACHE_KEY_VERSION: &str = "lance-data-v1";
+const FOYER_PAGE_SIZE: usize = 4096;
+
-+struct OriginNamespace {
-+ origin: Weak<dyn ObjectStore>,
-+ namespace: uuid::Uuid,
-+}
-+
-+static ORIGIN_NAMESPACES: LazyLock<Mutex<HashMap<usize, OriginNamespace>>> =
-+ LazyLock::new(|| Mutex::new(HashMap::new()));
-+
-+fn origin_namespace(origin: &Arc<dyn ObjectStore>) -> uuid::Uuid {
-+ // store_prefix omits endpoint/credentials, and this interface exposes no
stable
-+ // backend identity. Only the same live origin may reuse metadata or data.
-+ // Persist a random namespace, never an address that another process can
reuse.
-+ let mut namespaces = ORIGIN_NAMESPACES.lock().unwrap();
-+ namespaces.retain(|_, entry| entry.origin.strong_count() != 0);
-+ namespaces
-+ .entry(Arc::as_ptr(origin) as *const () as usize)
-+ .or_insert_with(|| OriginNamespace {
-+ origin: Arc::downgrade(origin),
-+ namespace: uuid::Uuid::new_v4(),
-+ })
-+ .namespace
-+}
-+
+/// Configuration for the optional Foyer data-file cache.
+#[repr(C)]
Review Comment:
[P1] Include the actual storage origin in Foyer cache keys. This now retains
only Lance's `store_prefix`, which is `s3$bucket` for S3 and omits
`aws_endpoint`, while Doris passes per-catalog endpoints into a BE-wide
default-enabled session. Two catalogs with the same bucket and object path on
different endpoints can read the first endpoint's cached data (or a recovered
disk block) instead of their own, even when the second origin would deny the
read. Key both metadata and blocks by a stable endpoint/authorization identity,
or isolate origins, and restore the distinct-endpoint tests.
##########
docs/lance-prefilter-profile.md:
##########
@@ -32,17 +32,17 @@ vectors use a different loader and are not included in
these counters.
| Counter | Meaning |
| --- | --- |
-| `LancePrefilterLoads` | Number of row-ID prefilter loader executions
started. |
-| `LancePrefilterInputBatches` | Successfully consumed input batches. |
-| `LancePrefilterInputRows` | Non-null input row IDs, including duplicates. |
-| `LancePrefilterRowIds` | Sum of distinct row IDs in successfully completed
allow sets. |
-| `LancePrefilterLoadTime` | Total loader wall time, including input polling
and set construction. |
-| `LancePrefilterInputTime` | Wall time polling input batches, including
upstream execution, I/O, decoding and scheduling. |
-| `LancePrefilterBuildTime` | Wall time inserting row IDs into the allow set,
measured once per batch. |
+| `RowIdPrefilterLoads` | Number of row-ID prefilter loader executions
started. |
Review Comment:
[P3] Update the prefilter guide for the new default. The paragraph above
this renamed counter table still says an actual filter uses the row-ID
prefilter path, but a `vector_search` with `filter` and no `prefilter` now
postfilters and can have no row-ID prefilter counters. Qualify that statement
with `prefilter=true` and describe the false/default case so profiles are
interpreted correctly.
--
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]
