andygrove commented on PR #3783: URL: https://github.com/apache/datafusion-comet/pull/3783#issuecomment-5441788095
> **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. Adding a Spark-plus-Iceberg baseline engine so Comet's native Iceberg scan can be compared against the JVM Iceberg path is clearly useful, and per-query plan capture is the kind of thing you want the first time a benchmark result looks surprising. A few things. **The PR has been open a long time with an unaddressed review** There is review feedback on this from May. If those comments have been handled, a reply saying so would let it move; if they have not, that is the blocker rather than anything below. **Plan capture output** `tpcbench.py` gains per-query plan capture. Where do the plans go, how large do they get for TPC-DS at scale, and is the capture on by default or behind a flag? Twenty lines of diff is small, but writing a physical plan per query per engine can produce a lot of files, and a benchmark harness that quietly fills a directory is unpleasant. If it is opt-in, the description should say which flag. **Table creation accepting both shapes** Accepting `table.parquet` and `table/` is a two-line change and clearly convenient. Is there a case where both exist and the choice is ambiguous? If so, which wins, and should it warn? **Documentation** The description contains a good runbook: download Spark, fetch the Iceberg runtime jar, and so on. That is exactly the kind of thing that gets lost in a PR body. Is there a `benchmarks/` README where those steps belong, so someone who finds `spark-iceberg.toml` in six months knows how to use 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]
