avamingli commented on PR #1762: URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-5466683932
> ### 3. Sublink-to-Join Conversion for Nested Arithmetic Expressions > tl;dr - let's make an exception for functions - do not transform expressions contains user functions, maybe except IMMUTABLE or STABLE one - for discussion. > > #### The whole idea discussion > The whole idea is simple, I agree with it, why not to unnest the whole expression, why we limit transformation for a simple sentences? > > The search through pg hackers shows that Heike already tried to do the same in PG: > > Heikki Linnakangas, May 2017 [Pulling up more complicated subqueries](https://www.postgresql.org/message-id/[email protected]) > > He uses TPC-DS Q6 as the motivating example: > > ``` > SELECT * FROM foo > WHERE foo.j >= 1.2 * (SELECT avg(bar.j) FROM bar WHERE foo.i = bar.i); > ``` > > Key quote: > > > "The planner can pull up simpler subqueries, converting them to joins, but unfortunately this case is beyond its capabilities." > > He proposes the manual rewrite: > > ``` > SELECT * FROM foo > LEFT JOIN (SELECT avg(bar.j) AS avg, bar.i FROM bar GROUP BY bar.i) AS avg_bar > ON foo.i = avg_bar.i > WHERE foo.j >= 1.2 * avg_bar.avg; > ``` > > And propose a multi-step roadmap. The issue is that never fully landed in core ) > > We could ask Heike why so )) , but he cited Tom Lane from the 2011's thread > > > "Thinking of it as a pull-up or push-down transformation is the wrong approach because those sorts of transformations are done too early to be able to use cost comparisons." > > So the most likely reason is that he's unsure whether you can make transformations without comparing the cost. > > But! We've been already doing it, see [dcdc6c0](https://github.com/apache/cloudberry/commit/dcdc6c0b34bd7184f780f6e2f79f337937435476) - it was commited by Heike ) The solution reminds me Surajit Chaudhuri idea to move aggregation through the join tree https://vldb.org/conf/1994/P354.PDF > > There is one remaining part to move - perform unnesting in order to pull-up. Let's do it ) > > #### Implementation discussion > I've made a test: > > ``` > create table test(id int, price float, category int); > > CREATE OR REPLACE FUNCTION increment(f float) RETURNS float AS $$ > BEGIN > RETURN f + 1; > END; > $$ LANGUAGE plpgsql; > CREATE FUNCTION > > postgres=# explain verbose select * > from test i > where i.price > 12 + 1.2 * > (select increment(avg(j.price)) > from test j > where j.category = i.category); > QUERY PLAN > ------------------------------------------------------------------------------------------------------------------------------ > Gather Motion 3:1 (slice1; segments: 3) (cost=541.17..1269.50 rows=23700 width=16) > Output: i.id, i.price, i.category > -> Hash Join (cost=541.17..953.50 rows=7900 width=16) > Output: i.id, i.price, i.category > Inner Unique: true > Hash Cond: (i.category = "Expr_SUBQUERY".csq_c0) > Join Filter: (i.price > ('12'::double precision + ('1.2'::double precision * "Expr_SUBQUERY".csq_c1))) > -> Seq Scan on public.test i (cost=0.00..271.00 rows=23700 width=16) > Output: i.id, i.price, i.category > -> Hash (cost=528.67..528.67 rows=1000 width=12) > Output: "Expr_SUBQUERY".csq_c1, "Expr_SUBQUERY".csq_c0 > -> Broadcast Motion 3:3 (slice2; segments: 3) (cost=424.50..528.67 rows=1000 width=12) > Output: "Expr_SUBQUERY".csq_c1, "Expr_SUBQUERY".csq_c0 > -> Subquery Scan on "Expr_SUBQUERY" (cost=424.50..515.33 rows=333 width=12) > Output: "Expr_SUBQUERY".csq_c1, "Expr_SUBQUERY".csq_c0 > -> Finalize HashAggregate (cost=424.50..512.00 rows=333 width=12) > Output: j.category, increment(avg(j.price)) > Group Key: j.category > -> Redistribute Motion 3:3 (slice3; segments: 3) (cost=389.50..419.50 rows=1000 width=36) > Output: j.category, (PARTIAL avg(j.price)) > Hash Key: j.category > -> Streaming Partial HashAggregate (cost=389.50..399.50 rows=1000 width=36) > Output: j.category, PARTIAL avg(j.price) > Group Key: j.category > -> Seq Scan on public.test j (cost=0.00..271.00 rows=23700 width=12) > Output: j.id, j.price, j.category > Settings: optimizer = 'off' > Optimizer: Postgres query optimizer > (28 rows) > ``` > > Here you could see that we perform `increment` for the whole dataset and only then join. But we do not know what actually `increment` does, it could contain a cumbersome users logic and depend on the rows processing order. So we cannot move it somewhere and function should be left untouched. Except maybe `IMMUTABLE` or `STABLE` one. My experience tells me they are safe and could be moved. Thanks for the thorough review, and especially for digging up the pg-hackers history and putting the transformation on such solid theoretical footing — that framing is really helpful. One clarification on the prior art before addressing the function-safety point. The nested-arithmetic case wasn't actually implemented before this PR The commit you cited, dcdc6c0b, it does not implement the nested case. Before this PR, convert_EXPR_to_join() looked like: ```c Node *rarg = list_nth(opexp->args, 1); Assert(IsA(rarg, SubLink)); ``` i.e. the SubLink had to be the immediate operand of the outer OpExpr. Any nesting —` price > 1.2 * (SELECT ...), price > 12 + 1.2 * (SELECT ...)` — hit the assertion path and the pull-up was never entered. The intent to handle these was arguably implied in surrounding comments, but the code only supported the flat case. My commit 213d0e4040e is what closes that gap. Empirical verification To confirm, revert 213d0e4040e in-place, rebuilt, and ran the same query on a demo cluster: ```sql CREATE TABLE test (id int, price float, category int) DISTRIBUTED BY (id); INSERT INTO test SELECT g, (random()*100)::float, (g % 10) FROM generate_series(1,10000) g; ANALYZE test; EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) SELECT count(*) FROM test i WHERE i.price > 1.2 * (SELECT avg(j.price) FROM test j WHERE j.category = i.category); Before this PR (commit reverted): Seq Scan on test i (actual rows=1374 loops=1) Filter: (price > ('1.2'::double precision * (SubPlan 1))) SubPlan 1 -> Aggregate (actual rows=1 loops=3385) -- re-executed once per outer row -> Materialize (loops=3385) -> Broadcast Motion -> Seq Scan on test j With this PR: Hash Join (actual rows=1374 loops=1) Hash Cond: (i.category = "Expr_SUBQUERY".csq_c0) Join Filter: (i.price > ('1.2'::double precision * "Expr_SUBQUERY".csq_c1)) -> Seq Scan on test i (actual rows=3385 loops=1) -> Hash -> Broadcast Motion -> Subquery Scan on "Expr_SUBQUERY" -> Finalize GroupAggregate (actual rows=5 loops=1) -- executed once ``` loops=3385 → loops=1 on the aggregation side. Baseline (price > (SELECT ...) with no wrapping arithmetic) produces a Hash Join in both builds, as expected — that's the case dcdc6c0b already covered. On function safety — you're right Fully agreed. The pull-up moves the subquery expression from "evaluated per outer row inside a SubPlan" to "evaluated once per correlation group before the join". For volatile functions this changes semantics; for stable/immutable ones it's safe. The correct fix is to gate the transformation on a contain_volatile_functions() (and possibly contain_mutable_functions() if we want to be conservative like the inline path is) check on the subquery's targetlist and quals before attempting the pull-up. -- 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]
