github-actions[bot] commented on code in PR #67005:
URL: https://github.com/apache/doris/pull/67005#discussion_r3822895915
##########
be/src/exec/rowid_fetcher.cpp:
##########
@@ -1108,13 +1124,17 @@ Status RowIdStorageReader::read_batch_external_row(
fmt::memory_buffer file_read_times_buffer;
format_to(file_read_times_buffer, "[");
+ std::map<std::string, int64_t> lance_fetch_times_ns;
size_t idx = 0;
for (const auto& [_, scan_info] : scan_rows) {
format_to(file_read_lines_buffer, "{}, ", scan_info.first.size());
- *init_reader_avg_ms = fetch_statistics[idx].init_reader_ms;
+ *init_reader_avg_ms += fetch_statistics[idx].init_reader_ms;
Review Comment:
[P2] Do not reuse an already averaged value across request blocks
`init_reader_avg_ms` points to a value created once in `read_by_rowids()`
and reused for every `request_block_desc`, while this function divides it in
place at the end of each call. After a first block with 10/20 ms groups leaves
15, a second one-group block at 30 ms now reports `(15 + 30) / 1 = 45 ms`. That
corrupts the second block's `InitReaderAvgTime` profile and the final request
log. Please compute the per-block total/average locally, and keep separate raw
totals plus a count if the log needs a request-wide average.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScanNode.java:
##########
@@ -214,7 +217,7 @@ private List<Split> createFragmentSplits(LanceTableMetadata
metadata,
List<Split> splits = new ArrayList<>(visibleFragments.size());
for (LanceFragmentInfo fragment : visibleFragments.values()) {
LanceSplit split =
LanceSplit.forFragment(metadata.getDatasetUri(), metadata.getVersion(),
- fragment.getId(), fragment.getPhysicalRows());
+ fragment.getId(), fragment.getRowCount(),
fragment.getPhysicalRows());
Review Comment:
[P1] Preserve unknown deletion counts before enabling metadata COUNT
`LanceMetadataLoader` obtains this value from the pinned Java SDK's
`FragmentMetadata.getNumRows()`. In 9.1.0-beta.3 that method subtracts
`getNumDeletions()`, but `getNumDeletions()` returns 0 when a deletion file's
`numDeletedRows` is absent
([source](https://github.com/lance-format/lance/blob/v9.1.0-beta.3/java/src/main/java/org/lance/FragmentMetadata.java#L80-L93)).
Lance's canonical Rust metadata contract treats that case as an unknown row
count instead
([source](https://github.com/lance-format/lance/blob/v9.1.0-beta.3/rust/lance-table/src/format/fragment.rs#L538-L551)).
For such an older fragment, this line now puts the physical count into every
split and the BE returns it without opening Lance, so `COUNT(*)` includes
deleted rows. Please preserve whether the logical count is known and serialize
`-1`/scan when it is not; the tests should include a deletion file with a
missing count.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -388,6 +394,10 @@ Status LanceTableReader::get_block(Block* block, bool*
eos) {
DORIS_CHECK(block != nullptr);
DORIS_CHECK(eos != nullptr);
DORIS_CHECK(block->columns() == _projected_columns.size());
+ if (_is_table_level_count_active()) {
Review Comment:
[P2] Honor the asynchronous scanner stop in the metadata path
A running `FileScannerV2` can pass its outer RuntimeState cancellation check
and then be stopped by `ScannerContext`; `try_stop()` sets
`_io_ctx->should_stop`, which is why both generic `TableReader` metadata reads
and the ordinary Lance loop check that flag. This new early return bypasses the
Lance check and can synthesize a full batch with `eos=false` after the scanner
context has stopped. Check `should_stop` before `_read_table_level_count()`
with the same EOF/cleanup behavior, and cover it with a non-null stopped IO
context.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -767,9 +802,16 @@ Status LanceTableReader::_open_scanner(const
TFileRangeDesc& range) {
return _lance_error("enable Lance vector prefilter");
}
RETURN_IF_ERROR(_configure_vector_search(scanner));
- DORIS_CHECK(_fragment_count != nullptr);
- if (lance_params.__isset.fragment_ids) {
- COUNTER_UPDATE(_fragment_count,
static_cast<int64_t>(lance_params.fragment_ids.size()));
+ const int64_t fragment_count =
+ lance_params.__isset.fragment_ids
+ ?
static_cast<int64_t>(lance_params.fragment_ids.size())
+ : 0;
+ if (lance_params.__isset.index_segment_uuids &&
!lance_params.index_segment_uuids.empty()) {
+ COUNTER_UPDATE(_index_segment_count,
+
static_cast<int64_t>(lance_params.index_segment_uuids.size()));
+ COUNTER_UPDATE(_indexed_fragment_count, fragment_count);
+ } else {
+ COUNTER_UPDATE(_flat_knn_fragment_count, fragment_count);
Review Comment:
[P2] Do not infer flat execution from missing segment UUIDs
The FE also emits an unsegmented fragment split when index metadata exists
but fragment coverage is unavailable. With the default request it omits
`use_index`, and lance-c therefore leaves Lance's indexed-search default
enabled; only the segment restriction is absent. This branch then reports those
fragments as `LanceFlatKnnFragmentCount`, even though Lance may use the index,
and this PR removes the old `LanceUseIndex` field that could expose the policy.
Please either force `use_index=false` for true flat fallbacks, or use a neutral
counter/actual execution signal instead of classifying by UUID presence.
--
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]
