refactor(clickhouse): move `translate_val` to toposorted compiler loop #7209

cpcloud · 2023-09-24T22:40:33Z

This PR moves translate_val to function the same way that translate_rel
does: inputs are compiled and then passed into translate_val instead of
translation rules having to compile their own inputs.

A number interesting things come out of this:

Recursion is entirely removed from the compiler. This makes the logic it
much easier to follow IMO.
Most rules are greatly simplified: no need to call translate_val
recursively, inputs are compiled and then passed to functions.
A few features "just work" after this refactor because the translation rule
for the op is simpler (window functions)

Additionally, I was able to remove most occurrences of dialect="clickhouse"
and convert the clickhouse translate rules to use sqlglot functions or or other
constructs (SQLStringView and Joins need the dialect parameter).

Not counting the newly generated SQL, the net lines of code are reduced as well:

❯ git diff --shortstat upstream/master HEAD~
 24 files changed, 975 insertions(+), 1258 deletions(-)

ibis/backends/clickhouse/compiler/relations.py

cpcloud · 2023-09-25T12:09:45Z

ibis/backends/clickhouse/compiler/core.py

+    op = op.replace(
+        p.WindowFunction(p.Cumulative, ...)
+        >> (lambda op, _: cumulative_to_window(op.func, op.frame))
+    )


The pattern system is pretty darn sweet.

You "should" be able to do the replacements in one pass by combining them into an AnyOf pattern:

params = {param.op(): value for param, value in params.items()} scalar_rule = p.ScalarParameter >> ( lambda op, _: ops.Literal(value=params[op], dtype=op.dtype) ) window_rule = p.WindowFunction(p.Cumulative, ...) >> ( lambda op, _: cumulative_to_window(op.func, op.frame) ) op = op.replace(scalar_rule | window_rule)

We should provide a way to spell the replacement rule as a decorated function:

@replace(ops.ScalarParameter) def substitute_param(op, ctx): return ops.Literal(value=params[op], dtype=op.dtype)

Possibly worth a low-priority ticket.

Let's avoid building that until we have some more use cases as input

Single-pass replacement worked 🎉

Note that this will work until the replacement rules have distinct matchers because the first matching pattern gets applied.

ibis/backends/clickhouse/compiler/relations.py

kszucs · 2023-09-25T12:23:09Z

ibis/backends/clickhouse/compiler/core.py

+    return new_expr.op()
+
+
+def _translate_node(node, *args, **kwargs):


Can you please remove _translate_node, translate_rel and translate_val in favor of a single flat overloaded function, e.g. translate()?

I can do that in a follow up.

Actually I think this adds unnecessary implicit behavior:

I now have to deal with import cycles

I now have to ensure that both values and relations translate is imported so that all the rules are registered.

I don't really see what we gain by making these into a single rule.

Put all the translate implementations into a single file?

I like that they are separate now, it makes it easier to find things IMO.

kszucs · 2023-09-25T12:25:04Z

ibis/backends/clickhouse/compiler/core.py

+    )
+
+    # apply translate rules in topological order
+    results = op.map(fn, filter=(ops.TableNode, ops.Value))


Restricting the traversal is required or just performance optimization?

I haven't tried it without. I'd rather keep it as is, if only for readability. It's easier to see that the code should only work with these node types than if there were no filter, which would suggest it handles generic Node instances.

kszucs · 2023-09-25T12:47:59Z

ibis/backends/clickhouse/compiler/relations.py

-        *map(partial(translate_val, **kw), op.values), dialect="clickhouse"
-    )
+@translate_rel.register
+def _dummy(op: ops.DummyTable, *, values, **_):


Can you remove the keyword-only marker * from the functions? I usually find it more distracting than useful.

I kind of like it as an indicator of compiled inputs. I'd rather keep it if that's okay.

It also further enforces the op structure during compilation, you can't have any rogue arguments.

The error messages are also much more readable with this when an argument is missing. With positional-or-keyword you don't get the name of the argument that was missing when you fail to pass it in whereas with keyword-only you do.

I think it's better than having it positional-or-keyword, even if the * is a bit distracting.

ibis/backends/clickhouse/compiler/values.py

kszucs · 2023-09-25T13:12:45Z

ibis/backends/clickhouse/compiler/values.py

-    ops.Multiply: "*",
-    ops.Divide: "/",
-    ops.Modulus: "%",
+    ops.Add: operator.add,


We repeat these mappings (along with their string infix representation) at multiple places, possible we should set them directly on the operation classes. Should create a follow-up ticket for this.

…on for easier non-recursive compilation

…tion

kszucs · 2023-09-26T13:35:52Z

ibis/expr/analysis.py

+def cumulative_to_window(
+    func: ops.Cumulative, frame: ops.WindowFrame
+) -> ops.WindowFunction:
+    klass = getattr(ops, func.__class__.__name__.replace("Cumulative", ""))


It looks a bit fragile to me but fine for now.

We haven't changed the name of cumulative operations ever ... maybe?

kszucs · 2023-09-26T13:36:28Z

ibis/backends/tests/test_window.py

@@ -952,9 +949,6 @@ def agg(df):


 @pytest.mark.notimpl(["datafusion", "polars"], raises=com.OperationNotDefinedError)
-@pytest.mark.broken(
-    ["clickhouse"], reason="clickhouse returns incorrect results", raises=AssertionError


kszucs · 2023-09-26T13:40:31Z

ibis/backends/clickhouse/tests/test_functions.py

+        alltypes[alltypes.int_col == 1]
+        .limit(n)
+        .int_col.collect()
+        .map(lambda x: my_add(x, 1))


Pretty nice!

kszucs · 2023-09-26T13:45:20Z

ibis/backends/clickhouse/tests/conftest.py

-
-
-@pytest.fixture
-def translate():


Nice to see this gone!

kszucs · 2023-09-26T13:46:12Z

ibis/backends/clickhouse/compiler/core.py

+    # rewrite cumulative functions to window functions, so that we don't have
+    # to think about handling them in the compiler, we need only compile window
+    # functions
+    replace_cumulative_ops = p.WindowFunction(


kszucs · 2023-09-26T13:46:58Z

ibis/backends/clickhouse/compiler/core.py

+            op.value,
+            ops.TableArrayView(
+                ops.Selection(
+                    table=an.find_first_base_table(op.options), selections=(op.options,)


We should switch to Topmost pattern to locate the base table, but in a follow-up.

Something like BaseTableOf = Topmost(ops.Relation)

kszucs · 2023-09-26T13:48:11Z

ibis/backends/clickhouse/compiler/core.py

+        c.ExistsSubquery(x)
+    )
+
+    op = op.replace(


Starting to have a ruleset here, great to see it in action!

kszucs

Great improvement, thanks @cpcloud!

cpcloud added refactor Issues or PRs related to refactoring the codebase clickhouse The ClickHouse backend labels Sep 24, 2023

cpcloud force-pushed the clickhouse-translate-val-map branch from 26cb0e2 to 58449f6 Compare September 24, 2023 22:41

cpcloud commented Sep 24, 2023

View reviewed changes

ibis/backends/clickhouse/compiler/relations.py Outdated Show resolved Hide resolved

cpcloud commented Sep 24, 2023

View reviewed changes

ibis/backends/clickhouse/compiler/relations.py Show resolved Hide resolved

cpcloud commented Sep 24, 2023

View reviewed changes

ibis/backends/clickhouse/compiler/relations.py Outdated Show resolved Hide resolved

cpcloud force-pushed the clickhouse-translate-val-map branch 9 times, most recently from 034819a to d771fd4 Compare September 25, 2023 12:02

cpcloud commented Sep 25, 2023

View reviewed changes

ibis/backends/clickhouse/compiler/relations.py Outdated Show resolved Hide resolved

cpcloud commented Sep 25, 2023

View reviewed changes

ibis/backends/clickhouse/compiler/relations.py Outdated Show resolved Hide resolved

cpcloud commented Sep 25, 2023

View reviewed changes

ibis/backends/clickhouse/compiler/relations.py Outdated Show resolved Hide resolved

kszucs reviewed Sep 25, 2023

View reviewed changes

cpcloud force-pushed the clickhouse-translate-val-map branch 3 times, most recently from a320152 to a2b4eaf Compare September 25, 2023 12:34

kszucs reviewed Sep 25, 2023

View reviewed changes

ibis/backends/clickhouse/compiler/values.py Outdated Show resolved Hide resolved

kszucs reviewed Sep 25, 2023

View reviewed changes

cpcloud force-pushed the clickhouse-translate-val-map branch 3 times, most recently from 9ec68ae to 85a56b2 Compare September 25, 2023 16:15

cpcloud force-pushed the clickhouse-translate-val-map branch 4 times, most recently from 18dff95 to 3c4ca70 Compare September 26, 2023 10:54

cpcloud added this to the 7.0 milestone Sep 26, 2023

cpcloud force-pushed the clickhouse-translate-val-map branch from 3c4ca70 to 9d09f8e Compare September 26, 2023 11:05

cpcloud mentioned this pull request Sep 26, 2023

revert: feat(patterns): support building sequences in replacement patterns #7221

Merged

cpcloud force-pushed the clickhouse-translate-val-map branch 2 times, most recently from 2c19f21 to 3bccfb1 Compare September 26, 2023 11:31

cpcloud added 5 commits September 26, 2023 07:47

chore: add sqlglot helpers

63d705d

refactor(array-apply): adjust array map and array filter representati…

880c401

…on for easier non-recursive compilation

refactor(backends): adjust backends to work with new array representa…

45c586e

…tion

refactor(clickhouse): remove recursion from the compiler

f81a44e

test(sql): generate snapshots

e2b4c61

cpcloud force-pushed the clickhouse-translate-val-map branch from 3bccfb1 to e2b4c61 Compare September 26, 2023 11:51

kszucs reviewed Sep 26, 2023

View reviewed changes

ibis/backends/clickhouse/tests/conftest.py

@pytest.fixture

def translate():

Copy link

Member

kszucs Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see this gone!

kszucs reviewed Sep 26, 2023

View reviewed changes

kszucs approved these changes Sep 26, 2023

View reviewed changes

kszucs merged commit 7f1852e into ibis-project:master Sep 26, 2023
86 checks passed

cpcloud deleted the clickhouse-translate-val-map branch September 26, 2023 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(clickhouse): move `translate_val` to toposorted compiler loop #7209

refactor(clickhouse): move `translate_val` to toposorted compiler loop #7209

cpcloud commented Sep 24, 2023 •

edited

Loading

cpcloud Sep 25, 2023

kszucs Sep 25, 2023

kszucs Sep 25, 2023 •

edited

Loading

cpcloud Sep 25, 2023

cpcloud Sep 25, 2023

kszucs Sep 25, 2023 •

edited

Loading

kszucs Sep 25, 2023

cpcloud Sep 25, 2023

cpcloud Sep 25, 2023

kszucs Sep 25, 2023

cpcloud Sep 25, 2023 •

edited

Loading

kszucs Sep 25, 2023

cpcloud Sep 25, 2023

kszucs Sep 25, 2023

cpcloud Sep 25, 2023

cpcloud Sep 25, 2023 •

edited

Loading

kszucs Sep 25, 2023

kszucs Sep 26, 2023

cpcloud Sep 26, 2023

kszucs Sep 26, 2023

kszucs Sep 26, 2023

kszucs Sep 26, 2023

kszucs Sep 26, 2023

kszucs Sep 26, 2023

kszucs Sep 26, 2023

kszucs Sep 26, 2023

kszucs left a comment

		return new_expr.op()


		def _translate_node(node, args, *kwargs):

refactor(clickhouse): move translate_val to toposorted compiler loop #7209

refactor(clickhouse): move translate_val to toposorted compiler loop #7209

Conversation

cpcloud commented Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kszucs Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kszucs Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcloud Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcloud Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kszucs left a comment

Choose a reason for hiding this comment

refactor(clickhouse): move `translate_val` to toposorted compiler loop #7209

refactor(clickhouse): move `translate_val` to toposorted compiler loop #7209

cpcloud commented Sep 24, 2023 •

edited

Loading

kszucs Sep 25, 2023 •

edited

Loading

kszucs Sep 25, 2023 •

edited

Loading

cpcloud Sep 25, 2023 •

edited

Loading

cpcloud Sep 25, 2023 •

edited

Loading