andygrove commented on PR #5453: URL: https://github.com/apache/datafusion-comet/pull/5453#issuecomment-5441327336
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. This is a genuinely useful capability. Being unable to tell "128 bytes of data pages" apart from "a 512 KB coalesced GET" is a real gap when you are debugging a slow scan against object storage, and the layering into reader boundary, object-store boundary, and cache boundary is the right decomposition. The native test coverage is unusually thorough for a metrics change. That said, I have several concerns that I think need resolving before this merges. **`ScanIoObjectStore::get_ranges` replaces the inner store's implementation** For the `ObjectStore` role, `get_ranges` calls `coalesce_ranges(ranges, |range| self.get_range(location, range), OBJECT_STORE_COALESCE_DEFAULT)` rather than delegating to `self.inner.get_ranges`. That is the same body as `object_store`'s default trait method, so for stores that do not override it nothing changes. But any store that does override `get_ranges` loses its own implementation as soon as the wrapper is installed. Comet already has an object-store data cache in flight (#4828), and a caching store is exactly the kind of thing that wants to own `get_ranges`. This turns a metrics change into a change in read behavior, which I do not think we want. I understand why it is written this way, since delegating to the inner store would hide the coalescing that the metrics exist to expose. Is there a way to instrument the coalescing without owning it? If not, this tradeoff should at least be spelled out in a comment at the `get_ranges` site, so that whoever adds a store with a custom `get_ranges` finds out before their implementation is silently bypassed. **`get_opts` collapses `GetResultPayload::File` into an in-memory stream** The `File` branch does `result.bytes().await?` and rewraps the whole thing as a single-chunk stream. That discards the file-handle payload, which is the mechanism that lets callers avoid buffering the whole object. The metadata-role wrapper is installed regardless of `ScanIoSource`, so this applies to local filesystem reads as well. Metadata is bounded so it is probably fine in practice, but "probably fine" plus "silently changes how local reads are serviced" is worth a comment explaining the bound, or a check that the range is small before buffering. **`stop_batch_producer` is unrelated to metrics and looks risky** `Java_org_apache_comet_Native_releasePlan` now aborts the producer task and busy-waits up to 100ms in 1ms sleeps on the calling JNI thread, then gives up and proceeds regardless. Two things I would like to understand. What happens on the give-up path? The context is dropped shortly after, while a task that has been asked to abort but has not yielded may still hold references into it. If that is safe, why is it safe? If it is only safe because 100ms is empirically enough, that is a race, not a bound. Second, why is this in a metrics PR at all? The description says cancellation should not leave background work distorting published metrics, but this is a change to plan teardown semantics and it deserves its own PR and its own review. Blocking a Spark task thread for up to 100ms per plan release is not free either. Could this be split out? **Nine always-on SQL metrics per scan** Every entry in the scan metric map becomes a driver-side accumulator per task and a line in the SQL UI node. Going from one `bytes_scanned` to ten scan-I/O metrics is a real cost on a job with tens of thousands of tasks, and it makes the scan node in the SQL UI considerably busier for every user, including the ones who will never look at read amplification. Was gating these behind a config considered? If they stay always-on, it would help to see a note in the description about the accumulator overhead measured on a job with a large task count. **`docs/source/user-guide/latest/metrics.md` is not updated** That page is hand-maintained and has a `ScanExec` section. Nine new user-facing metrics with names like `scan_io_object_store_response_bytes_read` need an explanation of what they mean, which ones are subsets of which (footer bytes inside metadata bytes), and the warning from the description that reader-level and object-store bytes are alternative views rather than values to add together. That last point in particular is going to be got wrong by users if it lives only in the PR description. **The remote-scheme allowlist will silently under-report** `scan_io_source` matches `s3`, `s3a`, `gs`, `az`, `abfs`, `abfss`, `http`, `https`. That leaves out `gcs`, `azure`, `wasb`, `wasbs`, `s3n`, and anything a user registers through a custom scheme, all of which fall to `OtherObjectStore` and report zeros for the object-store metrics. A user on `wasbs://` sees three zero counters and has no way to tell that from "no coalescing happened". Would it be safer to invert this, so that everything except `file` and HDFS counts as remote? If the allowlist stays, the metrics doc should list which schemes are covered. **Encryption `uri_base` change** `get_options` now builds `uri_base` from `physical_object_store_scheme` plus the host and port slice, instead of `object_store_url.to_string()`. That changes the key the encryption factory is keyed on. It looks deliberate and connected to the `+comet-` scheme suffix, but it is a functional change to encryption in a PR about metrics, and I did not see a test that covers the before and after of the `uri_base` value specifically. Could you either call it out explicitly in the description with a test, or split it? -- 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]
