sunchao commented on PR #5453: URL: https://github.com/apache/datafusion-comet/pull/5453#issuecomment-5429340530
Thanks, @andygrove ! splitting this makes sense. I’ll move the object-store isolation fix and producer-shutdown change into separate PRs with their own tracking issues, and keep this PR focused on scan I/O metrics. Leaving `bytes_scanned` unchanged was deliberate, to preserve its existing semantics. I’ll explain that where the `TODO` was removed, correct the filesystem-level claim in `metrics.md`, and document all nine metrics, including which measurements overlap and must not be summed. You’re right about the encrypted-footer wording. That path records a complete footer payload before decryption and validation, so the description overstates the guarantee. I’ll clarify the semantics and add coverage for corrupt encrypted footers. I’ll also document the footer protocol and read-method assumptions, consolidate backend classification, and strengthen the HDFS test so it checks actual range-read delegation. A few details from checking the implementation: - The `s3`/`s3a` URI inconsistency is real, although `CometFileKeyUnwrapper` already normalizes both before key lookup. I still agree that normalized, deterministic registration would be cleaner. - The pinned `object_store` recognizes `azure` and `adl`, but not `wasb`/`wasbs`. Comet’s native Azure integration supports `abfs`/`abfss`. I’ll keep classification aligned with backend construction; simply treating every non-file, non-HDFS store as remote would also include in-memory stores. - The existing projection/pruning tests would catch a broad change that classified data reads as metadata, but I agree the dependency on upstream call patterns should be explicit. For producer shutdown, the wait can improve the final metrics snapshot, but it does not guarantee complete accounting of in-flight work. I agree the latency tradeoff needs separate evidence. I’ll address that, along with the shared-runtime test concern, in the separate PR. -- 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]
