andygrove commented on PR #5358: URL: https://github.com/apache/datafusion-comet/pull/5358#issuecomment-5608501348
Thanks for the rebase; the branch is on current `main` now and merges cleanly. But your own reply is the thing to settle before any of the mechanical review matters. > I don't think we can drop Spark 3.4 support now, since those efforts will be otherwise wasted. If that is where you have landed, I would rather this sat as a draft, or closed with an issue tracking the removal, than stayed open as a rebase-and-conflict treadmill. It touches the Maven profiles, the shim directories, `dev/diffs/3.4.3.diff`, three CI matrices and `compute-changes.py`, so it will conflict with almost anything that lands, and someone will keep having to rebase it for a decision that has not been made. Which would you prefer? If the answer is "hold", I am happy to convert it to a draft myself. On the substance, since it is worth recording either way: the mechanical work is sound. I went through it in detail and found nothing removed that 3.5, 4.0, 4.1 or 4.2 still needs. `isSpark35Plus` is deleted rather than left as an always-true guard, `CometSpark34AqeDppFallbackRule` is gone with all its call sites and tag fields, the unreachable `classOf[Base64] -> CometBase64` mapping is cleaned up, and only `3.4.3.diff` was touched under `dev/diffs/` with no hand-editing of the surviving diffs. If the project decides to drop 3.4 later, this is the change to pick up rather than redo. The three items @comphead cited as reasons to hold are all resolved now (#4837 closed, #4547 closed, #5314 merged), and their `CHANGES_REQUESTED` from 19 August is still the blocking review. @comphead, whichever way the timing question goes, could you clear or restate that? At the moment the PR is blocked on a review whose stated reasons no longer apply, which makes it hard to tell whether the remaining objection is those specific items or the broader "not yet". My own view on the timing, for the record: I do not think the argument that in-flight 3.4 work would be wasted is a reason to keep 3.4 indefinitely, but it is a reason not to drop it in the middle of that work. So a tracked deferral with a target version seems right, rather than either merging now or closing without a plan. -- 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]
