github-actions[bot] commented on code in PR #65218:
URL: https://github.com/apache/doris/pull/65218#discussion_r3527078599
##########
be/src/format_v2/table_reader.h:
##########
@@ -571,6 +579,18 @@ class TableReader {
return Status::OK();
}
+ void _sync_reader_read_rows_to_io_context() {
+ if (_io_ctx == nullptr || _io_ctx->file_reader_stats == nullptr) {
+ return;
+ }
+ DORIS_CHECK(_data_reader.reader != nullptr);
+ const int64_t read_rows =
_data_reader.reader->reader_statistics().read_rows;
+ DORIS_CHECK(read_rows >= _last_reader_read_rows);
+ const int64_t delta_read_rows = read_rows - _last_reader_read_rows;
+ _io_ctx->file_reader_stats->read_rows +=
cast_set<size_t>(delta_read_rows);
+ _last_reader_read_rows = read_rows;
Review Comment:
This sync still preserves post-filter row semantics for several materialized
readers. `_sync_reader_read_rows_to_io_context()` copies the concrete reader's
private `read_rows`, but CSV/TEXT, JSON, native, and RemoteDoris increment that
field only after `apply_materialized_reader_filters()` or `_apply_filters()`
has already reduced `*rows`. For example, the delimited text reader saves
`rows_before_filter`, applies file-local delete/conjunct filters that mutate
`*rows`, and then adds the filtered `*rows` to `_reader_statistics.read_rows`.
A batch that decodes 4096 rows and has a pushed file-local conjunct reject them
all will therefore sync zero rows here and publish zero `ScanRows`/IOContext
scan rows, while Parquet now uses raw scheduler rows. Please record the
pre-filter/source rows in the affected readers' scan-row stats, keeping
returned rows and `predicate_filtered_rows` separate, and add a test that
drives a real materialized v2 reader with a file-local filter instead of pre-
seeding `FileReaderStats`.
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -770,10 +783,110 @@ void FileScannerV2::update_realtime_counters() {
if (_file_reader_stats == nullptr) {
return;
}
- const int64_t bytes_read = _file_reader_stats->read_bytes;
+ DORIS_CHECK(_file_cache_statistics != nullptr);
+ const int64_t bytes_read =
cast_set<int64_t>(_file_reader_stats->read_bytes);
+ auto* local_state = static_cast<FileScanLocalState*>(_local_state);
+ const auto file_type =
+ _current_range.__isset.file_type
+ ? _current_range.file_type
+ : (_params != nullptr && _params->__isset.file_type ?
_params->file_type
+ :
TFileType::FILE_LOCAL);
+ const auto deltas = _collect_realtime_counter_deltas(
+ *_file_reader_stats, *_file_cache_statistics,
_uncached_reader_bytes_storage(file_type),
Review Comment:
This row source still excludes the supported JNI table-reader paths.
`update_realtime_counters()` now derives `ScanRows`, IOContext scan rows, and
`query_scan_rows` only from `_file_reader_stats->read_rows`, and the new sync
hook that feeds that field runs from the base
`TableReader::get_block()`/`close_current_reader()` path after a concrete
`FileReader` updates its private stats. But `FORMAT_JNI` scans such as JDBC,
MaxCompute, Trino connector, Iceberg sys tables, and JNI Hudi/Paimon splits use
`JniTableReader::get_block()`, which overrides the base method, returns the JNI
batch after `_get_next_jni_block()`/`finalize_jni_block()`, and never adds
those rows to `_io_ctx->file_reader_stats`. Those scans can therefore publish
file bytes from tracing stats while still reporting zero scan rows. Please add
the equivalent row accounting to the JNI path, preferably using the pre-filter
`current_rows` returned by `_get_next_jni_block()` to match the old scanner's
`count_read_rows() == fa
lse` fallback, and cover a JNI-format FileScannerV2 path in the tests.
##########
be/src/format_v2/parquet/parquet_reader.cpp:
##########
@@ -482,13 +490,25 @@ Status ParquetReader::get_block(Block* file_block,
size_t* rows, bool* eof) {
}
const auto predicate_filtered_rows_before =
_state->scheduler.predicate_filtered_rows();
- RETURN_IF_ERROR(_state->scheduler.read_next_batch(_state->file_context,
_state->file_schema,
- *request_snapshot,
file_block, rows, eof));
+ const auto raw_rows_read_before = _state->scheduler.raw_rows_read();
+ Status st = _state->scheduler.read_next_batch(_state->file_context,
_state->file_schema,
+ *request_snapshot,
file_block, rows, eof);
+ if (!st.ok()) {
+ if (_io_ctx != nullptr && _io_ctx->should_stop) {
+ *rows = 0;
+ *eof = true;
+ return Status::OK();
+ }
+ return st;
+ }
_sync_page_cache_profile();
if (_io_ctx != nullptr) {
_io_ctx->predicate_filtered_rows +=
_state->scheduler.predicate_filtered_rows() -
predicate_filtered_rows_before;
}
+ const auto raw_rows_read = _state->scheduler.raw_rows_read();
+ DORIS_CHECK(raw_rows_read >= raw_rows_read_before);
+ _reader_statistics.read_rows += raw_rows_read - raw_rows_read_before;
_eof = *eof;
Review Comment:
Please account the Parquet aggregate path as well as the normal
`get_block()` path. This new raw-row increment only runs after
`scheduler.read_next_batch()`, but aggregate pushdown calls
`ParquetReader::get_aggregate_result()` directly. For `COUNT(complex_col)`,
that method walks the selected ranges and calls
`shape_reader->load_nested_levels_batch(batch_rows)` to read the column levels,
then returns through `TableReader::_try_materialize_aggregate_pushdown_rows()`
and closes the reader. Because that path never increments
`_reader_statistics.read_rows`, the new close-time sync in `TableReader` still
copies zero rows into `_io_ctx->file_reader_stats`, and
`ScanRows`/IOContext/`query_scan_rows` stay zero for a Parquet COUNT(col)
pushdown that actually read the selected rows. Please publish those
aggregate-path `batch_rows` to the same reader stats, or otherwise feed them
into the shared scan-row stats, and add a test for Parquet COUNT(col) aggregate
pushdown.
--
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]
