Thanks for v4, Tom! > This was ugly enough that it motivated me to go look at exactly why > an outer aggregate doesn't work here, in hopes of removing the > inconsistent restriction. I found it: replace_property_refs_mutator, > which increments Vars' varlevelsup to account for the fact that > they're being pushed into a subquery,
You are right. This is not something I considered. No reason why an outer aggregate can't be used inside the GRAPH_TABLE. The restriction only applies to the same-level aggregate, which there's no machinery for. I tested v4 with a mixed same-level and outer-level aggregate, and this combination is not rejected as it should be. Using the same EXISTS test from v4, none of these produce the clean parse-time rejection we expect. The COLUMNS case is worse, it returns a row instead of erroring. Under EXISTS the aggregate's value is never required, so the incorrect same-level aggregate is not evaluated and the bad query is accepted. ``` postgres=# SELECT EXISTS(SELECT num FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (count(c.customer_id + o.customer_id) AS num)) t) FROM customers o; exists -------- t (1 row) postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE count(c.customer_id + o.customer_id) > 0) COLUMNS (c.name AS nm)) t) FROM customers o; ERROR: Upper-level Var found where not expected postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE GROUPING(c.customer_id, o.customer_id) = 1) COLUMNS (c.name AS nm)) t) FROM customers o GROUP BY customer_id; ERROR: arguments to GROUPING must be grouping expressions of the associated query level ``` So, based on your explanation earlier, this led me to find that the leveling is still wrong. Specifically, min_varlevel > 0, since the level-zero GraphPropertyRef is not accounted for in check_agg_arguments_walker. In this case min_varlevel should be 0. The fix is to teach check_agg_arguments_walker that a GraphPropertyRef carries a level, the GRAPH_TABLE's own level. This is safe because a GraphPropertyRef can only ever refer to its own GRAPH_TABLE's level and can never appear inside a sub-select, so treating it as a level-zero reference is always correct. Such an aggregate then resolves to the GRAPH_TABLE's own level and the existing same-level rejection fires. With that in place all of the cases above are rejected cleanly at parse time. ``` postgres=# SELECT EXISTS(SELECT num FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (count(c.customer_id + o.customer_id) AS num)) t) FROM customers o; ERROR: aggregate functions are not allowed in GRAPH_TABLE COLUMNS postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE count(c.customer_id + o.customer_id) > 0) COLUMNS (c.name AS nm)) t) FROM customers o; ERROR: aggregate functions are not allowed in GRAPH_TABLE WHERE postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE GROUPING(c.customer_id, o.customer_id) = 1) COLUMNS (c.name AS nm)) t) FROM customers o GROUP BY customer_id; ERROR: grouping operations are not allowed in GRAPH_TABLE WHERE ``` v5 attached fixes the leveling problem, folded into your v4, with a mixed-level test cases added. What do you think? -- Sami Imseih Amazon Web Services (AWS)
v5-0001-Rework-GRAPH_TABLE-aggregate-window-SRF-rejection.patch
Description: Binary data
