github-actions[bot] commented on code in PR #67005:
URL: https://github.com/apache/doris/pull/67005#discussion_r3859652154
##########
be/src/format_v2/table/lance_reader.h:
##########
@@ -34,6 +34,7 @@
struct LanceBatch;
struct LanceDataset;
+struct LanceScanStatistics;
Review Comment:
[P1] Make this declaration match lance-c's public type
The patched v0.1.7 header defines this as an anonymous typedef, `typedef
struct { ... } LanceScanStatistics;`, rather than a tagged `struct
LanceScanStatistics`. Both `lance_reader.cpp` and the changed test include this
header before `<lance/lance.h>`, so this declaration introduces a different C++
type and the later typedef conflicts before either translation unit can
compile. Please either tag the public C struct consistently or include the
lance header here and remove this incompatible forward declaration.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -691,6 +758,10 @@ Status LanceTableReader::_open_scanner(const
TFileRangeDesc& range) {
return _lance_error("create Lance scanner");
}
std::unique_ptr<LanceScanner, LanceScannerDeleter> scanner_guard(scanner);
+ if (lance_scanner_set_statistics_callback(scanner,
&LanceTableReader::_collect_scan_statistics,
Review Comment:
[P2] Preserve statistics for successful early-stop scans
This callback runs only when Lance observes full EOF; the bundled API
explicitly does not invoke it on early scanner release. A successful Doris
outer LIMIT can consume a non-final batch, call `stop_scanners()`, set
`should_stop`, and close this scanner without another EOF-observing
`lance_scanner_next()`. Because this callback is the only updater for all the
new execution counters, that successful query reports zero (or only prior-split
totals) despite having performed I/O. Please expose/flush a partial summary
before close, with an incompleteness marker if needed, and cover a
LIMIT/early-close path with value assertions.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -303,21 +304,56 @@ Status LanceTableReader::init(TableReadOptions&& options)
{
_ctz = _runtime_state->timezone_obj();
const auto& lance_scan_params = _scan_params->lance_scan_params;
+ _dataset_open_time = ADD_TIMER(_scanner_profile, "LanceDatasetOpenTime");
Review Comment:
[P2] Attach these metrics to the V2 profile hierarchy
All of the new Lance timers/counters use the root-parent
`ADD_TIMER`/`ADD_COUNTER` forms even though `TableReader::init()` has already
created the required `FileScannerV2 -> TableReader -> FileReader -> IO` tree.
As a result, dataset/scanner/decode work is displayed beside `FileScannerV2`,
and `LanceExecutionIO*` is outside `IO`, so the profile cannot attribute the
new detail to its owning layer. Please create a Lance child under
`TableReader`, put the table/format metrics beneath it, place raw execution I/O
under `IO` (or an IO child), and assert parent relationships rather than only
flat counter presence.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -303,21 +304,56 @@ Status LanceTableReader::init(TableReadOptions&& options)
{
_ctz = _runtime_state->timezone_obj();
const auto& lance_scan_params = _scan_params->lance_scan_params;
+ _dataset_open_time = ADD_TIMER(_scanner_profile, "LanceDatasetOpenTime");
+ _scanner_configure_time = ADD_TIMER(_scanner_profile,
"LanceScannerConfigureTime");
+ _scanner_read_time = ADD_TIMER(_scanner_profile, "LanceScannerReadTime");
+ _arrow_to_doris_block_time = ADD_TIMER(_scanner_profile,
"LanceArrowToDorisBlockTime");
+ _execution_iops = ADD_COUNTER(_scanner_profile, "LanceExecutionIOOps",
TUnit::UNIT);
+ _execution_requests = ADD_COUNTER(_scanner_profile,
"LanceExecutionIORequests", TUnit::UNIT);
+ _execution_bytes_read =
+ ADD_COUNTER(_scanner_profile, "LanceExecutionIOBytesRead",
TUnit::BYTES);
+ _index_partition_cache_miss_loads =
+ ADD_COUNTER(_scanner_profile, "LanceIndexPartitionCacheMissLoads",
TUnit::UNIT);
+ _index_comparisons = ADD_COUNTER(_scanner_profile,
"LanceIndexComparisons", TUnit::UNIT);
+ // These scan counts are emitted by Lance's FilteredRead execution node.
For vector searches
+ // with an explicit fragment set, they normally describe the fragments,
ranges, and rows read
+ // while applying the row-id prefilter. They are scan input counts, not
ANN result counts.
+ _lance_count_metrics = {
+ {"fragments_scanned",
+ ADD_COUNTER(_scanner_profile, "LanceFragmentsScanned",
TUnit::UNIT)},
+ {"ranges_scanned",
+ ADD_COUNTER(_scanner_profile, "LanceRowOffsetRangesScanned",
TUnit::UNIT)},
+ {"rows_scanned", ADD_COUNTER(_scanner_profile, "LanceRowsScanned",
TUnit::UNIT)},
+ {"partitions_ranked",
+ ADD_COUNTER(_scanner_profile, "LanceIVFPartitionsRanked",
TUnit::UNIT)},
+ {"partitions_searched",
+ ADD_COUNTER(_scanner_profile, "LanceIVFPartitionsSearched",
TUnit::UNIT)},
+ {"deltas_searched",
+ ADD_COUNTER(_scanner_profile, "LanceVectorIndexSegmentsSearched",
TUnit::UNIT)},
+ };
+ _lance_time_metrics = {
+ // This is wait time reported by the same Lance scan execution
node described above,
+ // rather than Doris scanner scheduling wait time.
+ {"task_wait_time", ADD_TIMER(_scanner_profile,
"LanceTaskWaitTime")},
+ {"find_partitions_elapsed",
+ ADD_TIMER(_scanner_profile, "LanceIVFPartitionRankingTime")},
+ };
_vector_search = _scan_params->__isset.lance_scan_params &&
lance_scan_params.__isset.external_search_request;
if (_vector_search) {
RETURN_IF_ERROR(_validate_external_search_request());
const auto& request = lance_scan_params.external_search_request;
const auto& vector = request.search_query.vector_search;
- const bool use_index = !request.__isset.vector_search_options ||
-
!request.vector_search_options.__isset.use_index ||
- request.vector_search_options.use_index;
- _scanner_profile->add_info_string("LanceFragmentTopK",
std::to_string(vector.top_k));
- _scanner_profile->add_info_string("LanceFragmentOffset",
std::to_string(vector.offset));
- _scanner_profile->add_info_string("LanceVectorDimension",
-
std::to_string(vector.query_vector.dimension));
- _scanner_profile->add_info_string("LanceUseIndex", use_index ? "true"
: "false");
- _fragment_count = ADD_COUNTER(_scanner_profile, "LanceFragmentCount",
TUnit::UNIT);
+ _scanner_profile->add_info_string("LanceTopK",
std::to_string(vector.top_k));
Review Comment:
[P2] Keep the fragment scope in these profile labels
The production FE does not send the logical values here:
`createFragmentSearchRequest()` rewrites `(top_k, offset)` to `(top_k + offset,
0)` before serialization. Thus a logical `top_k=5, offset=1` is now profiled as
`LanceTopK=6`, `LanceOffset=0`, and `LanceTopKPlusOffset=6`. The unit test
constructs the BE request directly and misses this path. Please retain explicit
fragment/candidate naming (as the removed `LanceFragmentTopK` did), or also
propagate the original logical values under unambiguous names.
##########
thirdparty/vars.sh:
##########
@@ -552,10 +552,10 @@ PUGIXML_SOURCE=pugixml-1.15
PUGIXML_MD5SUM="3b894c29455eb33a40b165c6e2de5895"
# lance-c
-LANCE_C_DOWNLOAD="https://github.com/lance-format/lance-c/archive/refs/tags/v0.1.6.tar.gz";
-LANCE_C_NAME="lance-c-v0.1.6.tar.gz"
-LANCE_C_SOURCE="lance-c-0.1.6"
-LANCE_C_MD5SUM="1599faa2532d9ce963db1188f7435a56"
+LANCE_C_DOWNLOAD="https://github.com/lance-format/lance-c/archive/refs/tags/v0.1.7.tar.gz";
Review Comment:
[P1] Regenerate the shipped Rust crate-license inventory
This bump changes the locked normal/build dependency graph, but
`dist/LICENSE-dist.txt` still identifies lance-c v0.1.2 and
`dist/licenses/LICENSE-lance-c-rust-crates.txt` still says it was generated
from v0.1.2 with the old Lance/DataFusion versions. The exact v0.1.7 graph adds
shipped crates such as `goosefs-sdk`, `opendal-service-goosefs`,
`opendal-service-tos`, `lance-arrow-scalar`, `lance-index-core`,
`lance-select`, and `libsais-rs`, none of which appears in that notice. Please
update the distribution manifest and regenerate the inventory from the exact
patched v0.1.7 lockfile as part of this dependency update.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScanNode.java:
##########
@@ -435,9 +435,6 @@ public String getNodeExplainString(String prefix,
TExplainLevel detailLevel) {
if (isExternalSearch()) {
TVectorSearchParams vector =
externalSearchRequest.getSearchQuery().getVectorSearch();
result.append(prefix).append("externalSearchType=VECTOR\n");
-
result.append(prefix).append("lanceVectorColumn=").append(vector.getColumn()).append("\n");
Review Comment:
[P2] Retain the searched vector field in EXPLAIN
After removing this line, neither the remaining scan-node output nor the new
BE profile identifies which vector field is searched. That cannot always be
inferred from the tuple because `vector_search` allows the searched field to be
unprojected, so searches over different vector columns of the same table become
diagnostically indistinguishable. Please keep a safe `lanceVectorColumn=<name>`
field (while continuing to omit the raw query vector) and retain its regression
assertion.
--
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]
