viirya commented on code in PR #7094:
URL: https://github.com/apache/datafusion-comet/pull/7094#discussion_r4310902840
##########
docs/source/user-guide/latest/metrics.md:
##########
@@ -230,12 +230,16 @@ execution metrics. Counters accumulate per scan operator;
they do not instrument
| `scan_io_object_store_response_bytes_read` | Response bytes actually
consumed at that API, including bytes fetched between coalesced ranges. Not
HTTP wire bytes.
|
| `scan_io_metadata_cache_hits` | Successful, cache-eligible
metadata opens requiring no storage reads.
|
| `scan_io_metadata_cache_misses` | Successful, cache-eligible
metadata opens requiring storage reads. Failed opens and encrypted opens, which
bypass this shared cache, increment neither cache counter.
|
+| `scan_io_metadata_prefetches` | Metadata opens that used a load
started when the previous file opened
(`spark.comet.parquet.prefetchNextFileMetadata.enabled`). Each also counts as a
cache miss. |
| `row_groups_pruned_dynamic_filter` | Row groups skipped by a runtime
predicate whose value changes during execution, such as a local TopK threshold.
|
Reader-level and object-store bytes are two views of the same reads; do not
add them together.
Likewise, footer bytes are a subset of metadata bytes, not a third
reader-level category. A warm
-metadata-cache hit contributes no new metadata or footer I/O. A valid
plaintext footer followed by
-a page-index failure still contributes footer bytes; an invalid plaintext
footer does not.
+metadata-cache hit contributes no new metadata or footer I/O. A prefetched
load's footer and
+metadata bytes count when the prefetch reads them, so a prefetch that no open
uses, because the
+scan stopped early or pruned the file before opening it, adds bytes without a
matching cache hit.
Review Comment:
Two small leftovers from the old counting. This sentence still says an
unused prefetch "adds bytes without a matching cache hit". A used prefetch now
counts as a miss, so an unused one adds bytes with no matching cache miss and
no `scan_io_metadata_prefetches`. Also, the `scan_io_metadata_prefetches` row
says "Each also counts as a cache miss", but that holds only for loads that
read storage. A prefetched load that the cache served, which will be common
once #7082 shares the cache across a stage, counts as a hit and isn't counted
as a prefetch. Could the row say "opens that used a load which read storage
ahead of them"? The last sentence of the new section in `parquet.md` could use
the same wording.
##########
native/core/src/parquet/parquet_exec.rs:
##########
@@ -1326,6 +1349,64 @@ mod tests {
);
}
+ /// The prefetch is opt-in: a scan of several files loads metadata ahead
only when the session
+ /// sets `prefetch_next_file_metadata`. With it, only the first file's
open reads metadata from
+ /// storage, since each later file's footer and page index were loaded
while the file before
+ /// it was read, and the batches match a scan without it.
+ #[tokio::test]
+ async fn prefetching_the_next_files_metadata_keeps_the_scan_result() {
+ let fixtures: Vec<_> = (0..3).map(|_|
write_scan_io_fixture()).collect();
+ let schema = Arc::clone(&fixtures[0].1);
+ let files: Vec<_> = fixtures
+ .into_iter()
+ .map(|(filename, _)| PartitionedFile::from_path(filename).unwrap())
+ .collect();
+
+ let mut results = vec![];
+ // `None` is a session without the options, which every Comet plan
attaches.
Review Comment:
Keeping `is_some_and` sounds right to me. Plenty of tests build a plain
`SessionConfig` and call `init_datasource_exec`, such as the Delta DV and
dynamic filter tests. A missing extension being an error would make all of them
attach it, and that buys nothing.
--
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]
