andygrove commented on PR #5265: URL: https://github.com/apache/datafusion-comet/pull/5265#issuecomment-5441647260
> **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. The diagnosis is exactly right, and matching on `CometLeafExec` rather than `CometNativeScanExec` is the minimal fix. The comment explaining why the self-filtering in `reportScanInputMetrics` makes it safe for leaves that do not track `bytes_scanned` is the part I wanted to see, and it is there. Three things. **Is there a regression test for the Parquet case?** The change widens the condition, so the risk is double counting rather than under counting: a leaf that both reports its own input metrics in `doExecuteColumnar` and now also gets reported through `CometNativeExec.executeColumnarWithContext`. The new test covers the fused Iceberg shape. Is there an existing test that pins the non-fused Parquet shape, so we would notice if a leaf started being counted twice? If not, an assertion that `bytesRead` equals roughly the file size rather than twice it would be worth adding. **CSV and contributed scans** The comment names `CometCsvNativeScanExec` and contrib scans as beneficiaries. Do they actually populate `bytes_scanned`? If they do not today, the change is a no-op for them and that is fine, but the comment reads as though they are fixed by this too. Worth being precise about which scans this actually turns on. **The test is 237 lines for a one-line fix** That is not a criticism of the test, which is careful and well commented, especially the `fusingParents` assertion that stops it silently degrading into a duplicate of the `SELECT *` test. But `CometIcebergNativeSuite` only runs when Iceberg is on the classpath and it is already a long suite. Could the same property be asserted more cheaply against a Parquet scan plus a projection, with the Iceberg case reduced to confirming the scan type is recognized? If the Iceberg-specific setup is load-bearing then keep it as is, but it is worth asking whether the coverage has to cost this much wall clock. -- 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]
