Fix CTE reference in analyzer by feilong-liu · Pull Request #22515 · prestodb/presto

feilong-liu · 2024-04-13T05:14:12Z

Description

Currently when creating CTE Reference node, the NamedQuery used in the CTE is used to uniquely identify the CTE, which can lead to conflicts, which can be demonstrated with example in the linked issue.

Instead of trying to build the scope manually, we can simply rely on the underlying Query object linked to each CTE definition and reference, which refers to the same object if are referring to the same CTE.

Another advantage of this approach is that, it's much faster. I've seen query planning timeout due to the recursive query reference visit when dealing with large queries with nested ctes.

Motivation and Context

Fix a bug in CTE materialization

Impact

Bug fix.

Test Plan

Add unit test.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Fix a bug in CTE reference node creation, where different CTEs may be incorrectly considered as the same CTE.

feilong-liu · 2024-04-13T05:14:33Z

presto-hive/src/test/java/com/facebook/presto/hive/TestCteExecution.java

    }

+    @Test
+    public void testNesteCteWithSameName()


Will fail without change here.

feilong-liu · 2024-04-13T05:15:37Z

presto-main/src/main/java/com/facebook/presto/sql/planner/RelationPlanner.java

            else {
                // cte considered for materialization
-                String normalizedCteId = context.getCteInfo().normalize(analysis, namedQuery.getQuery(), cteName);
+                String normalizedCteId = context.getCteInfo().normalize(NodeRef.of(namedQuery.getQuery()), cteName);


Use NodeRef, because we want to use == rather than equals comparison.

feilong-liu · 2024-04-13T05:16:03Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SqlPlannerContext.java

-            public TreeSet<Query> getReferencedQuerySet()
-            {
-                return referencedQuerySet;
+            String identityString = queryNodeRef.hashCode() + delimiter + cteName;


Use hash code to uniquely identify a CTE

I am not sure if repeated runs of the query will have the same hashcode.
If they do then please ignore.
If they don't, is it better to normalize this hashcode by ID too like done previously?
The goal is to make debugging easier in prod since its better for repeated query runs to have the same id of the cte, also the test framework changes might not be required

Good point. Instead of using hash code, now I use an incremental prefix for the mapping purpose, which should be fixed now, and get rid of the massive change of test framework.

presto-main/src/main/java/com/facebook/presto/sql/planner/SqlPlannerContext.java

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/CteConsumerMatcher.java

feilong-liu · 2024-04-13T05:17:47Z

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/CteConsumerMatcher.java

+        String cteName = ((CteConsumerNode) node).getCteId();

-        return match();
+        if (cteNameMapping.containsKey(expectedCteName) && cteNameMapping.inverse().containsKey(cteName) && cteNameMapping.get(expectedCteName).equals(cteName)) {


One to one mapping exists and match.

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/CteConsumerMatcher.java

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/Matcher.java

jaystarshot

Nice catch, just added couple of comments

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/CteConsumerMatcher.java

jaystarshot · 2024-04-13T06:52:31Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SqlPlannerContext.java

-            public TreeSet<Query> getReferencedQuerySet()
-            {
-                return referencedQuerySet;
+            String identityString = queryNodeRef.hashCode() + delimiter + cteName;


I am not sure if repeated runs of the query will have the same hashcode.
If they do then please ignore.
If they don't, is it better to normalize this hashcode by ID too like done previously?
The goal is to make debugging easier in prod since its better for repeated query runs to have the same id of the cte, also the test framework changes might not be required

presto-hive/src/test/java/com/facebook/presto/hive/TestCteExecution.java

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/Matcher.java

jaystarshot · 2024-04-14T03:53:37Z

LGTM, however I only have codeowner access for presto main so you will need other reviews.

feilong-liu requested review from a team and jaystarshot as code owners April 13, 2024 05:14

feilong-liu requested a review from presto-oss April 13, 2024 05:14

feilong-liu commented Apr 13, 2024

View reviewed changes

feilong-liu force-pushed the fix_cte_ref branch from 2221e6d to 71801c9 Compare April 13, 2024 05:21

feilong-liu requested review from mlyublena, pranjalssh and rschlussel April 13, 2024 05:23

feilong-liu commented Apr 13, 2024

View reviewed changes

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/Matcher.java Outdated Show resolved Hide resolved

feilong-liu force-pushed the fix_cte_ref branch 5 times, most recently from 42f07d3 to 172098f Compare April 13, 2024 05:57

feilong-liu requested a review from hantangwangd as a code owner April 13, 2024 05:57

jaystarshot previously approved these changes Apr 13, 2024

View reviewed changes

elharo reviewed Apr 13, 2024

View reviewed changes

presto-hive/src/test/java/com/facebook/presto/hive/TestCteExecution.java Outdated Show resolved Hide resolved

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/Matcher.java Outdated Show resolved Hide resolved

feilong-liu dismissed jaystarshot’s stale review via af9495a April 14, 2024 01:03

feilong-liu force-pushed the fix_cte_ref branch from 172098f to af9495a Compare April 14, 2024 01:03

feilong-liu requested a review from jaystarshot April 14, 2024 01:07

jaystarshot previously approved these changes Apr 14, 2024

View reviewed changes

feilong-liu dismissed jaystarshot’s stale review via cdb69bd April 15, 2024 04:54

feilong-liu force-pushed the fix_cte_ref branch from af9495a to cdb69bd Compare April 15, 2024 04:54

rschlussel approved these changes Apr 15, 2024

View reviewed changes

mlyublena approved these changes Apr 15, 2024

View reviewed changes

Fix CTE reference in analyzer

408cb8f

feilong-liu force-pushed the fix_cte_ref branch from cdb69bd to 408cb8f Compare April 16, 2024 16:54

feilong-liu merged commit 29fd544 into prestodb:master Apr 16, 2024

feilong-liu deleted the fix_cte_ref branch April 16, 2024 18:48

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

Conversation

feilong-liu commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

feilong-liu Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

feilong-liu Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

feilong-liu Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

jaystarshot Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

feilong-liu Apr 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

feilong-liu Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaystarshot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaystarshot Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaystarshot commented Apr 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feilong-liu commented Apr 13, 2024 •

edited

Loading