TEST: Test propagate_map_queries pass

[mtsokol]

Hi @willow-ahrens,

I migrated propagate_map_queries test from my "draft autoscheduler" PR.

To make sure the test passes I had to adjust def make_term for Plan, Query, and MapJoin.

My impression is that x.make_term(x.head(), ...) kind of duplicates information - x.head() is more less a cls of x, right?

I propose:

def make_term(cls, *args):
    return cls(*args)

instead and override it (e.g. Plan) to make passing arguments more convenient.

[willow-ahrens]

Let's leave make_term as-is. I'd like to be able to change the representation of the term under the hood, so that if we need to make the head an integer/enum later we still can. Could we instead change the tests here?

[mtsokol]

Let's leave make_term as-is. I'd like to be able to change the representation of the term under the hood, so that if we need to make the head an integer/enum later we still can. Could we instead change the tests here?

Sure! I reverted the changes. Can we keep two overrides of make_term for Plan and MapJoin? They still follow cls, head, *args convention but allow to have Plan([a, b, c]) instead of Plan(([a, b, c],)).

[willow-ahrens]

Okay, I understand what's going on with make_term.

It should hold for all terms that
x == x.make_term(x.head(), *x.children())

Currently, this is not true for any of the terms that hold tuples of arguments.

You needed to override make_term to make this work for Plan and Mapjoin since those are the terms that got tested which hold tuples of arguments.

Could you override make_term so that the above invariant holds for all terms?

[mtsokol]

The whole problem originates from the fact that the only thing missing from Python dataclasses is variable arguments for the constructor. Therefore number of arguments must be always known at class definition level. This requires us to keep variable length arguments as a tuple for a few Logic nodes:

class Plan(LogicNode):
    bodies: tuple[LogicNode] = ()

and I think this is sufficient for our case, it's just something to keep in mind when rewriting Julia code.
But the current def make_term() implementation is broken due to this reason:

    @classmethod
    def make_term(cls, head, *args):
        """Creates a term with the given head and arguments."""
        return head(*args)

When constructing a plan.make_term(plan.head(), plan.arguments()) the *args causes the tuple to be applied as multiple parameters instead of one bodies tuple. Therefore I overridden those make_term that have iterable as an argument and it doesn't break OOP principles - it makes it compatible with tree-like nodes.

[mtsokol]

It should hold for all terms that x == x.make_term(x.head(), *x.children())

The current overrides for tree-like nodes (or expr-like nodes here) make sure that this is valid, e.g.:

@dataclass(eq=True, frozen=True)
class MapJoin(LogicNode):
    @classmethod
    def make_term(cls, head, op, *args):
        return head(op, args)

I can add a test for this!

[willow-ahrens]

Thanks!

[mtsokol]

I'll go ahead and merge it now, I'll add make_term test in #7.