Move WindowNode and JoinNode variations to the SPI #23976

rschlussel · 2024-11-07T17:10:19Z

Description

Move WindowNode, JoinNode, SemiJoinNode, MergeJoinNode, and SpatialJoinNode to the SPI

Motivation and Context

This change is required for using these nodes in plugin PlanCheckers and ConnectorPlanOptimizers. See e.g. discussion here #23596 (comment).

Impact

WindowNode, JoinNode, SemiJoinNode, MergeJoinNode, and SpatialJoinNode are added to the SPI. ConnectorJoinNode is removed from the SPI

Test Plan

CI

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 ==

SPI Changes
* Add WindowNode, JoinNode, SemiJoinNode, MergeJoinNode, and SpatialJoinNode to the SPI. :pr:`23976`
* Remove ConnectorJoinNode from the SPI. JoinNode can now be used instead. :pr:`23976`

steveburnett · 2024-11-07T22:37:17Z

Thanks for the release note entry! Just a nit.

== RELEASE NOTES ==

SPI Changes
* Add WindowNode, JoinNode, SemiJoinNode, and SpatialJoinNode to the SPI. :pr:`23976`

rschlussel · 2024-11-08T14:24:40Z

sorry don't review yet. I changed the protocol files directly, and I need to generate them instead.

rschlussel · 2024-11-08T20:30:14Z

this should be ready for review now. thanks!

aditi-pandit

Thanks @rschlussel for these changes.

presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.yml

feilong-liu

Overall lgtm!
Just for usage of ImmutableMap/ImmutableSet I am not sure if we are relying on the order preserving property here, if yes, it will be better if we change it to LinkedHashMap/LinkedHashSet.

feilong-liu · 2024-11-11T16:48:27Z

presto-spi/src/main/java/com/facebook/presto/spi/plan/JoinNode.java

        this.filter = filter;
        this.leftHashVariable = leftHashVariable;
        this.rightHashVariable = rightHashVariable;
        this.distributionType = distributionType;
-        this.dynamicFilters = ImmutableMap.copyOf(dynamicFilters);
+        this.dynamicFilters = unmodifiableMap(new HashMap<>(dynamicFilters));


Should it be using LinkedHashMap to preserve order, which was guaranteed by ImmutableMap?

feilong-liu · 2024-11-11T16:52:00Z

presto-spi/src/main/java/com/facebook/presto/spi/plan/SpatialJoinNode.java

-                .addAll(right.getOutputVariables())
-                .build();
-
+        Set<VariableReferenceExpression> inputSymbols = new HashSet<>();


LinkedHashSet to keep original order?

feilong-liu · 2024-11-11T16:52:23Z

presto-spi/src/main/java/com/facebook/presto/spi/plan/WindowNode.java


        this.source = source;
-        this.prePartitionedInputs = ImmutableSet.copyOf(prePartitionedInputs);
+        this.prePartitionedInputs = unmodifiableSet(new HashSet<>(prePartitionedInputs));


LinkedHashSet?

feilong-liu · 2024-11-11T16:52:32Z

presto-spi/src/main/java/com/facebook/presto/spi/plan/WindowNode.java

        this.specification = specification;
-        this.windowFunctions = ImmutableMap.copyOf(windowFunctions);
+        this.windowFunctions = unmodifiableMap(new HashMap<>(windowFunctions));


LinkedHashMap?

feilong-liu · 2024-11-11T16:53:02Z

presto-spi/src/main/java/com/facebook/presto/spi/plan/WindowNode.java

    }

    public Set<VariableReferenceExpression> getCreatedVariable()
    {
-        return windowFunctions.keySet();
+        return unmodifiableSet(windowFunctions.keySet());


Why we need change here?

you're right, not needed. I wasn't sure if the set view of the unmodifiable map would also be unmodifiable, but it is.

aditi-pandit

Thanks @rschlussel. LGTM for native changes. Have a minor nit in the rest of the code.

aditi-pandit · 2024-11-12T04:49:29Z

presto-spi/src/main/java/com/facebook/presto/spi/plan/AbstractJoinNode.java

@@ -11,18 +11,30 @@
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
-package com.facebook.presto.sql.planner.plan;
+


The license is repeated twice. Please delete it.

thanks! fixed

Move the above plan nodes to the SPI so that they can be used by plugin PlanCheckers.

With JoinNode moved to the SPI, ConnectorJoinNode is no longer necessary. Remove it and replace references with JoinNode.

aditi-pandit

Thanks @rschlussel LGTM for native changes.

rschlussel force-pushed the window-join-spi branch 6 times, most recently from 1ce43fa to bcaa7c9 Compare November 7, 2024 22:08

rschlussel force-pushed the window-join-spi branch from bcaa7c9 to ad8fd08 Compare November 8, 2024 00:11

rschlussel marked this pull request as ready for review November 8, 2024 14:10

rschlussel requested review from elharo, a team, jaystarshot, hantangwangd, ZacBlanco, shrinidhijoshi, feilong-liu and ClarenceThreepwood as code owners November 8, 2024 14:10

rschlussel requested a review from presto-oss November 8, 2024 14:10

rschlussel force-pushed the window-join-spi branch 2 times, most recently from ecd0ab5 to 9b61627 Compare November 8, 2024 16:16

aditi-pandit reviewed Nov 11, 2024

View reviewed changes

presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.yml Show resolved Hide resolved

feilong-liu reviewed Nov 11, 2024

View reviewed changes

rschlussel force-pushed the window-join-spi branch 2 times, most recently from 904e3c0 to 622e0aa Compare November 11, 2024 17:13

feilong-liu previously approved these changes Nov 11, 2024

View reviewed changes

rschlussel requested a review from aditi-pandit November 11, 2024 20:04

aditi-pandit previously approved these changes Nov 12, 2024

View reviewed changes

rschlussel added 2 commits November 12, 2024 13:25

Move Window, Join, SemiJoin, SpatialJoin, MergeJoin to SPI

593eaa0

Move the above plan nodes to the SPI so that they can be used by plugin PlanCheckers.

Remove ConnectorJoinNode from the SPI

3206477

With JoinNode moved to the SPI, ConnectorJoinNode is no longer necessary. Remove it and replace references with JoinNode.

rschlussel dismissed stale reviews from aditi-pandit and feilong-liu via 3206477 November 12, 2024 18:25

rschlussel force-pushed the window-join-spi branch from 622e0aa to 3206477 Compare November 12, 2024 18:25

aditi-pandit approved these changes Nov 12, 2024

View reviewed changes

feilong-liu approved these changes Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move WindowNode and JoinNode variations to the SPI #23976

Move WindowNode and JoinNode variations to the SPI #23976

rschlussel commented Nov 7, 2024 •

edited

Loading

steveburnett commented Nov 7, 2024

rschlussel commented Nov 8, 2024

rschlussel commented Nov 8, 2024

aditi-pandit left a comment •

edited

Loading

feilong-liu left a comment

feilong-liu Nov 11, 2024

feilong-liu Nov 11, 2024

feilong-liu Nov 11, 2024

feilong-liu Nov 11, 2024

feilong-liu Nov 11, 2024

rschlussel Nov 11, 2024

aditi-pandit left a comment

aditi-pandit Nov 12, 2024

rschlussel Nov 12, 2024

aditi-pandit left a comment

Move WindowNode and JoinNode variations to the SPI #23976

Are you sure you want to change the base?

Move WindowNode and JoinNode variations to the SPI #23976

Conversation

rschlussel commented Nov 7, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

steveburnett commented Nov 7, 2024

rschlussel commented Nov 8, 2024

rschlussel commented Nov 8, 2024

aditi-pandit left a comment • edited Loading

Choose a reason for hiding this comment

feilong-liu left a 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

aditi-pandit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

rschlussel commented Nov 7, 2024 •

edited

Loading

aditi-pandit left a comment •

edited

Loading