andygrove commented on PR #5345: URL: https://github.com/apache/datafusion-comet/pull/5345#issuecomment-5441646951
> **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 thing to offer. "How much of my workload would Comet accelerate" is a question people ask before they are willing to turn it on, and answering it without running anything natively is the right shape for that. The implementation is admirably small, and I checked that reverting `CometScanExec` and `CometBatchScanExec` is sufficient, since the native scan execs are only constructed inside the discarded `newPlan`. Some things to sort out. **Naming and description are inconsistent** The PR title says "trail mode", the config is `spark.comet.trial.enabled`, and the description says plans are annotated `Comet[Trail]`. "Trail" appears to be a typo for "Trial" and should be fixed everywhere. More importantly, the description says the annotation exists and the diff does not add one, and it also says "comet is only printing the plan while the execution is still native", which is the opposite of what the feature does. Could you rewrite the description to match the code? **This overlaps with `spark.comet.explain.planOnly.enabled`** #5394 adds a config with a very similar purpose. Two configs that both mean "analyze but do not execute natively" is going to confuse users, and whichever lands second will need to explain how it differs. Worth working out with @andygrove whether these should be one feature. **`logWarning` for expected output** Trial mode logs the full extended explain plus coverage stats at warning level for every query. Warning is for things that are wrong, and this is the feature working as intended. On a session that runs hundreds of queries the driver log becomes unusable. Could this be `logInfo`, or better, surfaced through the existing extended-explain mechanism so users can query it rather than grep for it? **No user-facing documentation** `configs.md` is generated so the config itself will show up, but a feature whose whole point is to be discovered and used by evaluators needs a paragraph somewhere a person would find it. `docs/source/user-guide/latest/tuning.md` or `understanding-comet-plans.md` would be reasonable homes. What does the output look like, and how should someone interpret the coverage number? **Test coverage** The test covers one aggregate query. Since the correctness of this feature is entirely about the revert being complete, it would be worth covering the shapes where the revert has more to undo: a V2 scan with runtime filters (the `CometBatchScanExec` branch has a `copy(runtimeFilters = ...)` that the aggregate query never exercises), and a query with a shuffle, to confirm no `CometShuffleExchangeExec` survives. A test that asserts "no `CometPlan` anywhere" across a handful of representative TPC-DS-ish queries would give much more confidence than one query. -- 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]
