sunchao commented on PR #5453: URL: https://github.com/apache/datafusion-comet/pull/5453#issuecomment-5443138079
Updated in [130ee02bb](https://github.com/apache/datafusion-comet/commit/130ee02bb4d3bd127c154a20cca3070ea5979644). The two reviews are addressed in this revision as follows: 1. **Split and issue references.** This PR now contains the scan metrics, documentation, and their tests, closing #5508. Deterministic object-store registration and encryption URI normalization are in #5503, closing #5502. Producer cancellation and waiting are in #5505, closing #5504. **The metrics PR depends on #5503**: the backend identity returned during construction must also be part of the cache identity, or an old cache collision could return the wrong kind of store. The shutdown change is independent. 2. **`bytes_scanned`, task bytes, and efficiency.** Keeping the existing meaning is deliberate. I restored an explanation beside the reader-factory setup, added the same qualification at the byte-read method, and corrected the filesystem-level claim in the guide. Footer/page-index loads bypass `bytes_scanned`, and coalescing can fetch more than its requested logical ranges. Task `bytesRead` and `scan_efficiency_ratio` still inherit those limitations; the new metrics do not silently redefine them. 3. **The nine metrics and overlap.** The metrics guide now defines every counter, explains footer bytes as a subset of metadata bytes, and says explicitly not to sum reader and object-store byte totals. It also covers cache eligibility, encrypted opens, metadata-only scans with no data denominator, backend scope, and incomplete cancellation snapshots. 4. **Registration order and encryption URIs.** #5503 derives isolated registration URLs deterministically from canonical backend/configuration identity from the first registration. Its tests use real distinct store contents and check both registration orders, configuration separation, cloud/HDFS alias routing, and ordinary/isolated encryption URI equivalence, including ports and custom schemes. Native `s3a` is canonicalized to `s3`. The previous direct URI test was preserved and strengthened there. `CometFileKeyUnwrapper` already normalizes the S3 aliases, so I am not describing the original spelling difference as a demonstrated failure of that implementation. 5. **Shutdown latency, ownership, and test runtime.** Those concerns are covered in #5505. The helper returns an explicit finished/timeout result, logs an incomplete snapshot on timeout, and tests use dedicated Tokio runtimes. A producer owns its stream and sender, not the raw execution-context pointer; that does not guarantee that all external task state remains usable after timeout. The 100 ms budget bounds the requested wait, not memory safety or arbitrary scheduler delays. Production release comes from the Spark executor thread; the exhausted-budget case is a synthetic helper test, not evidence that the JNI entry point normally runs on a Tokio worker. Local helper measurements show finished producers returning in microseconds, cooperative pending producers around one millisecond, and blocked work reaching the budget with incomplete counters. The separate PR reports the numbers and their limits; this metrics PR no longer claims complete accounting after cancellation. 6. **Backend classification and duplicate parsing.** `prepare_object_store_with_configs` now returns the selected backend classification with its URL/path. Both callers use it. The classification uses the pinned object-store parser, covering `azure` and `adl`, while explicitly retaining local, memory, and HDFS/custom categories. The pinned parser does not support native `gcs`, `wasb`, `wasbs`, or `s3n`; blanket inversion would also incorrectly label memory stores as remote. Tests check the accepted aliases and HDFS routing, and the guide lists the scope. 7. **Footer protocol and corrupt encryption.** Comments now explain the tail/payload protocol, when the plaintext decoder can advance to indexes, and why a later index failure does not erase a footer read. They also explain that a complete encrypted payload is counted before key retrieval/authentication. A new regression corrupts the encrypted footer's authentication tag and verifies that its completed I/O is still counted when authentication fails. The description no longer claims every malformed encrypted footer is excluded. 8. **Data versus metadata methods.** The code documents the pinned Parquet contract: `get_bytes` for Bloom filters, `get_byte_ranges` for data pages, and separate metadata loading for footers/indexes. Existing projection/pruning and Bloom-filter assertions already catch a broad reclassification; the dependency is now explicit rather than relying only on those tests. I did not replace it with an unvalidated offset classifier. 9. **Custom `get_ranges` behavior.** New tests perform actual reads through a recording store, checking returned bytes and which methods were called. Local/custom paths retain delegation. The native remote case intentionally exercises default coalescing and confirms the inner override is bypassed. A comment at that branch and the guide call out the composition constraint for a future data cache. Simply forwarding the call would hide the physical API requests this metric is intended to observe. 10. **The `File` payload branch.** That branch can buffer `GetResult.range`, not necessarily a whole file. In the pinned local metadata path, `get_ranges` delegates to `LocalFileSystem.get_ranges` and does not reach the wrapper's `get_opts` branch; local data reads also bypass the remote-role wrapper. The new delegation tests protect that path. I did not add an arbitrary range limit without a reachable local-scan regression. 11. **Accumulator overhead and a config.** Driver metric identities are created per scan operator; task copies and reporting still scale with task count. The new benchmark runs actual 10,000-task Spark jobs with zero/nine extra SQL accumulators, reverses the case order in a second JVM, and verifies all final values. Mean time increased from 7.410 to 7.930 seconds in one pass and 7.357 to 7.771 seconds in the other: about 41–52 microseconds per task, or 5.6–7.0% on this deliberately tiny-task `local[1]` fixture. This includes task serialization and scheduler updates, but excludes native counters, JNI, distributed networking, storage, and UI rendering. The description includes standard deviations, the separate driver-creation measurement, and reproduction commands. The metrics remain enabled alongside the other scan metrics; I am not claiming they are free or that this percentage predicts scan-query overhead. Validation for the follow-up: **100 native Parquet tests and 12 Spark 4.1.3 tests passed**, including all ten task-metrics tests, with the newly built metrics native library. The full JVM reactor, ScalaStyle/Spotless, native library/test Clippy with warnings denied, Rust formatting, and metrics-page formatting passed. Broader cross-version results are explicitly labeled as earlier validation rather than reruns of this revision. -- 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]
