fix: Avoid unnecessary input repartitioning with ScalarSubqueryExec#21986
fix: Avoid unnecessary input repartitioning with ScalarSubqueryExec#21986Dandandan merged 2 commits intoapache:mainfrom
ScalarSubqueryExec#21986Conversation
ScalarSubqueryExec
ScalarSubqueryExecScalarSubqueryExec
|
run benchmark tpch tpcds |
comphead
left a comment
There was a problem hiding this comment.
Thanks @neilconway, waiting for benches, but the PR makes a lot of sense, the extra repartition is waste of cycles.
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/fix-subquery-repartition (a59ee8d) to ba038e9 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/fix-subquery-repartition (a59ee8d) to ba038e9 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
It's a little odd we don't see any benchmark changes as a result of this PR ... the new plans are clearly better, so I think it's fine to land, but I'm mildly skeptical of these benchmark results. |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
I think it's quite obvious there is no difference: the output is a single row, so there is nothing to repartition (cost is negligible). |
Which issue does this PR close?
Rationale for this change
ScalarSubqueryExecmarked its main input (the parent query of the subquery) as benefitting from partitioning, but that was incorrect:ScalarSubqueryExecis just a passthrough node, so it doesn't benefit from input partitioning itself. This bug resulted in inserting unnecessaryRepartitionExecnodes underneathScalarSubqueryExec, notably in TPC-H Q22.What changes are included in this PR?
ScalarSubqueryExecAre these changes tested?
Yes.
Are there any user-facing changes?
Yes, some query plans will change (and improve).