andygrove commented on issue #2041: URL: https://github.com/apache/datafusion-ballista/issues/2041#issuecomment-5204801544
Agreed on shipping 54.1.0 with the correctness fixes rather than waiting for DataFusion 55. I went back through everything that has landed on `main` since `branch-54` was cut (133 commits) and triaged the bug fixes. Six of them cherry-pick onto `branch-54` cleanly with no adaptation at all, so I have opened them as draft backport PRs: - #2234 (backport of #2069) enable the executor client pool by default. `client_ttl` defaults to `0`, which disables pooling, so the shuffle-read path opens a new TCP connection per fetch and a single shuffle-heavy query exhausts the host's ephemeral ports. This one only shows up once you have a second executor, which is every real deployment. The SF1 TPC-DS gate went from 11 OK / 77 FAILED to 88 OK / 0 FAILED. - #2235 (backport of #2194) keep GROUP BY-less aggregates when propagating empty stages. Silent wrong answer under AQE, a dropped grand-total row. - #2236 (backport of #2225) bind the gRPC listener before registering with the scheduler. Startup race that gives ECONNREFUSED on registration and kills the executor. - #2237 (backport of #2107) send the shutdown notification before dropping the notifier. Executor hangs on ctrl+c. - #2238 (backport of #2061) spill the sort shuffle writer when the memory pool rejects a reservation grow. The writer was buffering through a rejection and under-reporting usage to the shared pool. - #2239 (backport of #2119) classify task IO errors through `find_root()`. Retryable IO errors wrapped in `DataFusionError::Shared` were being marked non-retryable, so a transient object store blip fails the whole job. All six are unmodified `git cherry-pick -x` of the original commits. `cargo fmt --check` is clean and `cargo check --workspace --all-targets` builds with no warnings on all six applied together. I left the test runs to CI. One thing worth a look before merging: #2234 changes a default value (`--client-ttl` from 0 to 30s). It is the highest impact fix of the six, but it is a behaviour change rather than a pure bug fix, so it is the one I would most expect people to have an opinion on. There are a few more fixes on `main` that I deliberately did **not** propose, because they do not cherry-pick and would need real rework: - #2188 (null-aware anti joins, so distributed `NOT IN` returns wrong results). I confirmed the bug is present on `branch-54`, but the fix is around 800 lines across the static planner, AQE join selection and config. If we want it in 54.1.0 it needs a purpose-built minimal version. - #2124 and #2086 (AQE join selection). These form a chain and collide with `broadcast_sort_merge_join_enabled` from #2130, which `main` has since deleted. - #2212 (fail running jobs when all executors are lost). Adds a new config key and depends on the chaos harness from #2026 that is not on `branch-54`. - #2160 and #2090 (executor memory pool defaults and propagating `memory_limit_per_task_bytes`). Both rebase without much trouble, but #2160 changes default memory behaviour, which felt like more than a patch release should carry. Happy to add them if others disagree. Two things turned out not to be needed: - #2064 (INTERSECT / EXCEPT dropping NULLs). The actual fix was in DataFusion and `branch-54` already ships 54.1.0, so this is fixed there. Only the regression test would port over. - #2186 (every task running every union branch). That is a consequence of multi-partition tasks from #2038, which is `main` only. `branch-54` runs one partition per task and is not exposed. @milenkovicm @phillipleblanc @avantgardnerio does this look like the right set, and is there anything from the second list you would want pulled in before we cut the release? -- 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]
