[SNOW-1731783] Refactor node query comparison for Repeated subquery elimination

[sfc-gh-yzou]

1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

SNOW-1731783

2. Fill out the following pre-review checklist:

   • I am adding a new automated test(s) to verify correctness of my new code
     • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
   • I am adding new logging messages
   • I am adding a new telemetry message
   • I am adding new credentials
   • I am adding a new dependency
   • If this is a new feature/behavior, I'm adding the Local Testing parity changes.

3. Please describe how your code solves the related issue.

In the previous repeated subquery elimination, we updated the node comparison for SnowflakePlan for Selectable to

    def __eq__(self, other: "SnowflakePlan") -> bool:
        if not isinstance(other, SnowflakePlan):
            return False
        if self._id is not None and other._id is not None:
            return isinstance(other, SnowflakePlan) and self._id == other._id
        else:
            return super().__eq__(other)

    def __hash__(self) -> int:
        return hash(self._id) if self._id else super().__hash__()

where the id is generated based on the query and query parameter, this means two node are treated as the same if they have same type and same query. This make sense when we do repeated subquery elimination, but not expected by other transformations.

Refactor the comparison to make sure that we only use the id comparison for repeated subquery eliminations, not for others.

[sfc-gh-aalam]

This make sense when we do repeated subquery elimination, but not expected by other transformations.

Can you give an example of where this does not make sense?

[sfc-gh-yzou]

@sfc-gh-aalam
"""
Can you give an example of where this does not make sense?
"""
i am not sure for which transformation this could make sense, unless the transformation want to treat two nodes with the same query as the same node, for example, for your large query breakdown, i don't think you want to treat two nodes with the same query as the same node during transformation. Or even cte transformation, during the actual node transformation, we don't want to treat two nodes as the same node during the plan transformation, because the are different nodes. This could cause us to missing apply transformation on some nodes and cause potential problem, because we could incorrectly thought some nodes have been handled. This only make sense when we are doing query comparison
In general, we should never change node comparison unless it is by design, however, for node in plan tree, that should not be the case.

This has been causing problem to our ctc workloads, where we missed some handling of some nodes, I have tried it with our benchmark workloads, the problem has been gone with this refactoring.

[sfc-gh-aalam]

LGTM. a few comments.

[sfc-gh-aalam]

we can do f"{query_id}_{node_type_name}"

[sfc-gh-jdu]

+1

[sfc-gh-yzou]

updated

[sfc-gh-aalam]

can we call this encode_node_id?

[sfc-gh-yzou]

i actually call this encode_node_id_with_query to be more clear of the encoded id to reducing the chance for people to use it incorrectly

[sfc-gh-aalam]

do we want the same node to have the same encode_id. If so, we can rely on id(node) which gives you deterministic result, instead of uuid4()

[sfc-gh-helmeleegy]

+1

[sfc-gh-yzou]

this is used for cached property, so with uuid4 generation it will also be deterministitic, but we can use id also.

[sfc-gh-jdu]

+1

[sfc-gh-jdu]

why do we need node_type_name?

[sfc-gh-yzou]

that is following the previous equivalence check, we require the node type to be the same, for example, a snowflake plan node and selectstatement node with the same query is not counted as the same

[sfc-gh-jdu]

these two properties seem having the same docstring?

[sfc-gh-yzou]

they are slightly different, but i further updated the comment to make it more clear

[sfc-gh-helmeleegy]

This make sense when we do repeated subquery elimination, but not expected by other transformations.

Can you give an example of where this does not make sense?

Also, it sounds like this is a behavioral change. Are there production workloads that can be impacted? Which ones?

[sfc-gh-yzou]

@sfc-gh-helmeleegy there should be no user facing behavior change, this equivalence check is introduced by the cte transformation, and only used internally for plan node comparison for cte transformation, and no others should rely on this fact. If yes, that would be counted as a bug. This is counted as a refactor work before others misuse this property, so no existing workload should be impacted

[sfc-gh-yzou]

@sfc-gh-aalam i am not sure if we are doing the write test here, if two nodes have the same query and same type, there should still be two nodes after deepcopy, because they are two different nodes.