From 298ac48f58d9493513c280950926eff68afc58bd Mon Sep 17 00:00:00 2001 From: jasper blues Date: Fri, 30 Oct 2026 09:24:41 +1000 Subject: [PATCH 1/2] Read a grouping key in an order term over an aggregate as the GROUP BY reads it, so Oracle sorts by me.age + count(you.age) On Oracle, ORDER BY me.age + count(you.age) beside me.age AS age failed with ORA-00979. The projection groups by the key read through its text (item_property), and the order term read me.age again from the properties. Where rows are grouped, an order term whose every aggregate and key is an item is now sorted over the items' columns, after the query. Any other term over an aggregate reads each key item as its column value, the same expression the GROUP BY holds (_keys_read). Port of drivine4j#429. Fixes #256 --- drivine/cypher/sql/sql_cypher_compiler.py | 66 ++++++++++++- tests/integration/test_cypher_compiled.py | 95 +++++++++++++++++++ .../sync_integration/test_cypher_compiled.py | 95 +++++++++++++++++++ tests/unit/test_oracle_item_property.py | 8 ++ 4 files changed, 260 insertions(+), 4 deletions(-) diff --git a/drivine/cypher/sql/sql_cypher_compiler.py b/drivine/cypher/sql/sql_cypher_compiler.py index ae378e5..15b13d4 100644 --- a/drivine/cypher/sql/sql_cypher_compiler.py +++ b/drivine/cypher/sql/sql_cypher_compiler.py @@ -2739,6 +2739,7 @@ def carried(column: str, value: Callable[[], Value], aggregates: Expression) -> column = f"s{i + 1}" # An order term that is an item, as written, is that item: so it may be ordered by where rows are compared. item = next((j for j, it in enumerate(items) if it.expression == sort.expression), -1) + over_items = _over_items(sort.expression, items, item_columns) if aggregating else None if contains(sort.expression, lambda it: isinstance(it, Variable) and it.name in anew) and all( it in item_columns for it in variables_in(sort.expression) ): @@ -2748,17 +2749,27 @@ def carried(column: str, value: Callable[[], Value], aggregates: Expression) -> carried(column, lambda item=item: compiled[item][1], sort.expression) elif compared and not contains(sort.expression, _aggregates): after[column] = self._sortable(self._expression(sort.expression, item_columns)) + elif over_items is not None: + # Where rows are grouped, so is one whose every aggregate and key is an item, each such part read by its + # item's name, as _aggregates_first reads it: worked out in the query, each key is read again as the + # GROUP BY reads it (_keys_read), on Oracle through its text (item_property), at each level of an + # operation over it, and `me.age + count(you.age)` compiled to twice what it did before + # (liberation-data/drivine4j#413). + after[column] = self._sortable(self._expression(over_items, item_columns)) else: # One that aggregates reads the rows it aggregates, as an item does; so does one that reads an aggregate # _aggregates_first worked out first, which no item is. Beside a variable-length walk it binds no value # in a subquery: carried out of a WITH's query, one so bound closes Oracle's (23.26) session # (DPY-4011), as `WITH b ORDER BY toString(coalesce(b.name, b.title))` did, its argument long enough to # be bound. Elsewhere it does, so operations nested in it compile to a statement that grows with them, - # not exponentially. - scope = {**select.scope, **dict(compiled)} - term = sort.expression + # not exponentially. Where rows are grouped, a part of it that is an item keying the groups reads that + # item's column value, as the GROUP BY does: Oracle refuses another read of the key (ORA-00979), as + # `me.age` read from the properties beside `me.age AS age` read through its text (item_property) was + # (liberation-data/drivine4j#413). + read, keys = _keys_read(sort.expression, items, columns) if aggregating else (sort.expression, {}) + scope = {**select.scope, **dict(compiled), **keys} carry = functools.partial( - carried, column, lambda term=term, scope=scope: self._expression(term, scope), term + carried, column, lambda read=read, scope=scope: self._expression(read, scope), sort.expression ) if select.walks: self._unshared(carry) @@ -5723,6 +5734,53 @@ def _aggregates(expression: Expression) -> bool: return is_aggregate(expression) or (isinstance(expression, Variable) and expression.name.startswith("#aggregate")) +def _over_items(term: Expression, items: list[ProjectionItem], item_columns: dict[str, Value]) -> Expression | None: + """ + The term, an order term of a grouping projection, with each part of it that is one of the items read by that + item's name, where so it reads no aggregate, no subquery and nothing but the items, `item_columns`; None where it + does. A variable an item is named after is that item, not the variable of that name before it. + """ + + def by_name(part: Expression) -> Expression | None: + if isinstance(part, Variable) and part.name in item_columns: + return part + return next((Variable(it.name) for it in items if it.expression == part), None) + + read = replaced(term, by_name) + if holds_subquery(read) or contains(read, is_aggregate) or not all(it in item_columns for it in variables_in(read)): + return None + return read + + +def _keys_read( + term: Expression, items: list[ProjectionItem], columns: list[tuple[str, Value]] +) -> tuple[Expression, dict[str, Value]]: + """ + The term, each part of it that is an item of a grouping projection keying the groups, one of the items, read by a + name no variable has, and what each such name is bound to: the item's value among the columns, as it is grouped + by. `replaced` reads no comprehension's, quantifier's or reduce's own expressions, so a part it reads is never one + whose variables such a one binds again: a key is read beside `[me IN [1] | me]` as anywhere else. + """ + # The last of two items alike, as drivine4j's `associate` keeps it. + keys = [ + (i, item.expression) + for i, item in reversed(list(enumerate(items))) + if not contains(item.expression, is_aggregate) and variables_in(item.expression) + ] + bound: dict[str, Value] = {} + + def key(part: Expression) -> Expression | None: + i = next((i for i, expression in keys if expression == part), None) + if i is None: + return None + # Named as no variable is, unless quoted so. + name = f"#key {i + 1}" + bound[name] = columns[i][1] + return Variable(name) + + return replaced(term, key), bound + + def children(expression: Expression) -> list[Expression]: if isinstance(expression, (Property, HasLabels)): return [expression.subject] diff --git a/tests/integration/test_cypher_compiled.py b/tests/integration/test_cypher_compiled.py index 6ca50e3..5028d0d 100644 --- a/tests/integration/test_cypher_compiled.py +++ b/tests/integration/test_cypher_compiled.py @@ -3698,3 +3698,98 @@ async def rows(cypher: str) -> list[Any]: for cypher, expected in writes: assert await rows(cypher) == expected, cypher await clear(cypher_pm) + + +async def test_an_order_term_over_an_aggregate_that_reads_a_grouping_key_is_sorted_by_the_key_as_it_is_grouped_by( + cypher_pm: PersistenceManager, +): + # On Oracle `me.age` in the term read the properties, where the key is grouped by as read through its text, and + # Oracle refused it (ORA-00979): #256, liberation-data/drivine4j#413. drivine4j's CypherQueryContract. + async def rows(cypher: str) -> list[Any]: + return await cypher_pm.query(spec(cypher)) + + await clear(cypher_pm) + # Sorted by the key alone, A, B, C, D; by the key and the count, B, A, C, D. + await rows( + "CREATE (a:CyKey {name: 'A', age: 10, born: date('2000-01-01')}), " + "(b:CyKey {name: 'B', age: 11, born: date('2000-01-05')}), " + "(c:CyKey {name: 'C', age: 20, born: date('2000-01-20')}), " + "(d:CyKey {name: 'D', age: 21, born: date('2000-01-25')}), " + "(a)-[:K]->(b), (a)-[:K]->(c), (a)-[:K]->(d)" + ) + match = "MATCH (me:CyKey)--(you:CyKey)" + by_key = [[10, 3], [11, 1], [20, 1], [21, 1]] + by_both = [[11, 1], [10, 3], [20, 1], [21, 1]] + cases: list[tuple[str, list[Any]]] = [ + (f"{match} RETURN me.age AS age, count(you.age) AS cnt ORDER BY me.age + count(you.age)", by_both), + ( + f"{match} WITH me.age AS age, count(you.age) AS cnt ORDER BY me.age + count(you.age) RETURN age, cnt", + by_both, + ), + (f"{match} RETURN DISTINCT me.age AS age, count(you.age) AS cnt ORDER BY me.age + count(you.age)", by_both), + (f"{match} RETURN me.age AS age, count(you.age) AS cnt ORDER BY age + count(you.age)", by_both), + # Sorted, skipped and limited before the WITH's WHERE, as Cypher reads it; a node keying the groups, read by + # property. + ( + f"{match} WITH me.age AS age, count(you.age) AS cnt ORDER BY me.age + count(you.age) SKIP 1 LIMIT 2 " + "WHERE cnt = 1 RETURN age", + [20], + ), + (f"{match} WITH me, count(you) AS cnt ORDER BY me.age * count(you) RETURN me.name", ["B", "C", "D", "A"]), + # An aggregate that is no item; a comprehension that names its own variable as the key's; the key in a function. + # On Oracle the comprehension's term compiles to 4.7 MB, which Oracle is still parsing minutes later: + over an + # aggregate's result is written out at every level until #235 is fixed. + (f"{match} RETURN me.age AS age, count(*) AS c ORDER BY me.age + count(you.age)", by_both), + *( + [] + if cypher_pm.type is DatabaseType.ORACLE + else [ + ( + f"{match} RETURN me.age AS age, count(you.age) AS cnt " + "ORDER BY me.age + size([me IN [1] | me]) + count(you.age)", + by_both, + ) + ] + ), + (f"{match} RETURN me.age AS age, count(*) AS cnt ORDER BY toString(me.age) + count(*)", by_key), + # Two keys, one a part of the other. + ( + f"{match} RETURN me.age AS a, me.age + 1 AS b, count(*) AS cnt " + "ORDER BY me.age + 1 + count(*), me.age * 2 - count(*)", + [[11, 12, 1], [10, 11, 3], [20, 21, 1], [21, 22, 1]], + ), + # Beside a variable-length walk, where the key is read as elsewhere. + ( + "MATCH (me:CyKey)-[:K*1..1]-(you:CyKey) RETURN me.age AS age, count(you) AS cnt " + "ORDER BY me.age * count(you)", + [[11, 1], [20, 1], [21, 1], [10, 3]], + ), + (f"{match} RETURN me.age AS age, count(you.age) AS cnt ORDER BY me.age", by_key), + # A key that is an expression, and several keys. + ( + f"{match} RETURN me.age + 1 AS k, count(*) AS cnt ORDER BY me.age + 1 + count(*)", + [[12, 1], [11, 3], [21, 1], [22, 1]], + ), + ( + f"{match} RETURN me.name AS name, me.age AS age, count(*) AS cnt ORDER BY me.age * count(*) DESC, me.name", + [["A", 10, 3], ["D", 21, 1], ["C", 20, 1], ["B", 11, 1]], + ), + ] + # Each that differs, so a store's every difference shows at once. + differing: list[str] = [] + for cypher, expected in cases: + try: + given = await rows(cypher) + except Exception as e: # noqa: BLE001 + differing.append(f"{cypher}: {str(e).splitlines()[0]}") + continue + if given != expected: + differing.append(f"{cypher}: {given}") + assert differing == [] + # A temporal key: A's 30 days are past C's 10. + temporal = ( + f"{match} RETURN me.name AS name, me.born AS born, count(*) AS cnt " + "ORDER BY me.born + duration({days: 10 * count(*)})" + ) + assert [it[0] for it in await rows(temporal)] == ["B", "C", "A", "D"], temporal + await clear(cypher_pm) diff --git a/tests/sync_integration/test_cypher_compiled.py b/tests/sync_integration/test_cypher_compiled.py index 16356b3..c6fa418 100644 --- a/tests/sync_integration/test_cypher_compiled.py +++ b/tests/sync_integration/test_cypher_compiled.py @@ -3668,3 +3668,98 @@ def rows(cypher: str) -> list[Any]: for cypher, expected in writes: assert rows(cypher) == expected, cypher clear(cypher_pm) + + +def test_an_order_term_over_an_aggregate_that_reads_a_grouping_key_is_sorted_by_the_key_as_it_is_grouped_by( + cypher_pm: PersistenceManager, +): + # On Oracle `me.age` in the term read the properties, where the key is grouped by as read through its text, and + # Oracle refused it (ORA-00979): #256, liberation-data/drivine4j#413. drivine4j's CypherQueryContract. + def rows(cypher: str) -> list[Any]: + return cypher_pm.query(spec(cypher)) + + clear(cypher_pm) + # Sorted by the key alone, A, B, C, D; by the key and the count, B, A, C, D. + rows( + "CREATE (a:CyKey {name: 'A', age: 10, born: date('2000-01-01')}), " + "(b:CyKey {name: 'B', age: 11, born: date('2000-01-05')}), " + "(c:CyKey {name: 'C', age: 20, born: date('2000-01-20')}), " + "(d:CyKey {name: 'D', age: 21, born: date('2000-01-25')}), " + "(a)-[:K]->(b), (a)-[:K]->(c), (a)-[:K]->(d)" + ) + match = "MATCH (me:CyKey)--(you:CyKey)" + by_key = [[10, 3], [11, 1], [20, 1], [21, 1]] + by_both = [[11, 1], [10, 3], [20, 1], [21, 1]] + cases: list[tuple[str, list[Any]]] = [ + (f"{match} RETURN me.age AS age, count(you.age) AS cnt ORDER BY me.age + count(you.age)", by_both), + ( + f"{match} WITH me.age AS age, count(you.age) AS cnt ORDER BY me.age + count(you.age) RETURN age, cnt", + by_both, + ), + (f"{match} RETURN DISTINCT me.age AS age, count(you.age) AS cnt ORDER BY me.age + count(you.age)", by_both), + (f"{match} RETURN me.age AS age, count(you.age) AS cnt ORDER BY age + count(you.age)", by_both), + # Sorted, skipped and limited before the WITH's WHERE, as Cypher reads it; a node keying the groups, read by + # property. + ( + f"{match} WITH me.age AS age, count(you.age) AS cnt ORDER BY me.age + count(you.age) SKIP 1 LIMIT 2 " + "WHERE cnt = 1 RETURN age", + [20], + ), + (f"{match} WITH me, count(you) AS cnt ORDER BY me.age * count(you) RETURN me.name", ["B", "C", "D", "A"]), + # An aggregate that is no item; a comprehension that names its own variable as the key's; the key in a function. + # On Oracle the comprehension's term compiles to 4.7 MB, which Oracle is still parsing minutes later: + over an + # aggregate's result is written out at every level until #235 is fixed. + (f"{match} RETURN me.age AS age, count(*) AS c ORDER BY me.age + count(you.age)", by_both), + *( + [] + if cypher_pm.type is DatabaseType.ORACLE + else [ + ( + f"{match} RETURN me.age AS age, count(you.age) AS cnt " + "ORDER BY me.age + size([me IN [1] | me]) + count(you.age)", + by_both, + ) + ] + ), + (f"{match} RETURN me.age AS age, count(*) AS cnt ORDER BY toString(me.age) + count(*)", by_key), + # Two keys, one a part of the other. + ( + f"{match} RETURN me.age AS a, me.age + 1 AS b, count(*) AS cnt " + "ORDER BY me.age + 1 + count(*), me.age * 2 - count(*)", + [[11, 12, 1], [10, 11, 3], [20, 21, 1], [21, 22, 1]], + ), + # Beside a variable-length walk, where the key is read as elsewhere. + ( + "MATCH (me:CyKey)-[:K*1..1]-(you:CyKey) RETURN me.age AS age, count(you) AS cnt " + "ORDER BY me.age * count(you)", + [[11, 1], [20, 1], [21, 1], [10, 3]], + ), + (f"{match} RETURN me.age AS age, count(you.age) AS cnt ORDER BY me.age", by_key), + # A key that is an expression, and several keys. + ( + f"{match} RETURN me.age + 1 AS k, count(*) AS cnt ORDER BY me.age + 1 + count(*)", + [[12, 1], [11, 3], [21, 1], [22, 1]], + ), + ( + f"{match} RETURN me.name AS name, me.age AS age, count(*) AS cnt ORDER BY me.age * count(*) DESC, me.name", + [["A", 10, 3], ["D", 21, 1], ["C", 20, 1], ["B", 11, 1]], + ), + ] + # Each that differs, so a store's every difference shows at once. + differing: list[str] = [] + for cypher, expected in cases: + try: + given = rows(cypher) + except Exception as e: # noqa: BLE001 + differing.append(f"{cypher}: {str(e).splitlines()[0]}") + continue + if given != expected: + differing.append(f"{cypher}: {given}") + assert differing == [] + # A temporal key: A's 30 days are past C's 10. + temporal = ( + f"{match} RETURN me.name AS name, me.born AS born, count(*) AS cnt " + "ORDER BY me.born + duration({days: 10 * count(*)})" + ) + assert [it[0] for it in rows(temporal)] == ["B", "C", "A", "D"], temporal + clear(cypher_pm) diff --git a/tests/unit/test_oracle_item_property.py b/tests/unit/test_oracle_item_property.py index 506f2b5..4d32df5 100644 --- a/tests/unit/test_oracle_item_property.py +++ b/tests/unit/test_oracle_item_property.py @@ -318,6 +318,14 @@ def read(cypher: str) -> str: assert "JSON_VALUE(JSON_QUERY(n1.props, '$.m' RETURNING JSON), '$.a.size()' RETURNING NUMBER)" in nested, nested +def test_a_grouping_key_an_order_term_over_the_items_reads_is_read_through_its_text_once_for_item_and_group_by(): + # liberation-data/drivine4j#413: the term read the key from the properties again, `JSON_VALUE(n1.props, '$.age' …)`, + # which isn't what the GROUP BY holds, and Oracle refused it (ORA-00979). It reads the items' columns now. + sql = oracle("MATCH (me:P)--(you:P) RETURN me.age AS age, count(you.age) AS cnt ORDER BY me.age + count(you.age)") + assert sql.count("JSON_SERIALIZE") == 2, sql + assert "JSON_VALUE(n1.props, '$.age" not in sql, sql + + def test_a_map_whose_key_holds_a_parenthesis_is_read_by_its_path_and_two_values_read_together_are_left_as_they_are(): # The parenthesis is in a literal, so the map taken out of the properties is still the one argument. sql = oracle("MATCH (n:Doc) RETURN size(n.`a)`.b)") From d372ddc170b7124837ac56adc2d115877d6f71ca Mon Sep 17 00:00:00 2001 From: jasper blues Date: Fri, 30 Oct 2026 13:01:43 +1000 Subject: [PATCH 2/2] Test an order term the items can't carry, because it holds a subquery --- tests/integration/test_cypher_compiled.py | 6 ++++++ tests/sync_integration/test_cypher_compiled.py | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/tests/integration/test_cypher_compiled.py b/tests/integration/test_cypher_compiled.py index 5028d0d..3ae8581 100644 --- a/tests/integration/test_cypher_compiled.py +++ b/tests/integration/test_cypher_compiled.py @@ -3736,6 +3736,12 @@ async def rows(cypher: str) -> list[Any]: [20], ), (f"{match} WITH me, count(you) AS cnt ORDER BY me.age * count(you) RETURN me.name", ["B", "C", "D", "A"]), + # A subquery in a term made of items, which _over_items leaves to be worked out in the query that groups. + ( + f"{match} WITH me, count(you) AS cnt ORDER BY COUNT {{ (me)--() }} * count(you) * 100 - me.age " + "RETURN me.name", + ["D", "C", "B", "A"], + ), # An aggregate that is no item; a comprehension that names its own variable as the key's; the key in a function. # On Oracle the comprehension's term compiles to 4.7 MB, which Oracle is still parsing minutes later: + over an # aggregate's result is written out at every level until #235 is fixed. diff --git a/tests/sync_integration/test_cypher_compiled.py b/tests/sync_integration/test_cypher_compiled.py index c6fa418..ffdeb3d 100644 --- a/tests/sync_integration/test_cypher_compiled.py +++ b/tests/sync_integration/test_cypher_compiled.py @@ -3706,6 +3706,12 @@ def rows(cypher: str) -> list[Any]: [20], ), (f"{match} WITH me, count(you) AS cnt ORDER BY me.age * count(you) RETURN me.name", ["B", "C", "D", "A"]), + # A subquery in a term made of items, which _over_items leaves to be worked out in the query that groups. + ( + f"{match} WITH me, count(you) AS cnt ORDER BY COUNT {{ (me)--() }} * count(you) * 100 - me.age " + "RETURN me.name", + ["D", "C", "B", "A"], + ), # An aggregate that is no item; a comprehension that names its own variable as the key's; the key in a function. # On Oracle the comprehension's term compiles to 4.7 MB, which Oracle is still parsing minutes later: + over an # aggregate's result is written out at every level until #235 is fixed.