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)

drivine/cypher/sql/sql_cypher_compiler.py

@@ -2782,6 +2782,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)
             ):
@@ -2791,17 +2792,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)
@@ -5820,6 +5831,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]

tests/integration/test_cypher_compiled.py

@@ -4038,3 +4038,104 @@ 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"]),
+        # 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.
+        (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)

tests/sync_integration/test_cypher_compiled.py

@@ -4005,3 +4005,104 @@ 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"]),
+        # 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.
+        (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)

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)")