andygrove commented on PR #4519: URL: https://github.com/apache/datafusion-comet/pull/4519#issuecomment-4855306615
Thanks for the updates. All my earlier points are addressed: the stage-boundary fix, the doc wording, the docstrings, the rule ordering, the dead guard, and the `def` conversion. I traced the input-stage path and confirmed the native shuffle case is safe (its child is always a zero-transition `CometNativeExec`, so the revert never fires there). Two small things: 1. The PR description still lists the old defaults (`enabled=true`, `maxTransitions=2` / min 2) but the code now uses `enabled=false`, `maxTransitions=5` / min 0. Worth updating the description so it matches. 2. It might be nice to add a direct test for the `CometColumnarShuffle` input-stage revert path. The end-to-end tests already exercise the AQE branch, so this is optional. This looks good to me. -- 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]
