Transform in values to in filter by jackychen718 · Pull Request #23161 · prestodb/presto

jackychen718 · 2024-07-10T06:02:23Z

Description

IN (VALUES ...) should be translated into simple IN LIST

Motivation and Context

The issue comes from #22728 IN (VALUES ..) should be translated as simple IN LIST
If VALUES table only has one column, transformed it into IN LIST

fixes #22728

Impact

Add another optimization rule, TransformInValuesToInFilter:
If filteringSource of SemiJoinNode comes from a ValueNode with only one column, transform it into a ProjectNode with outputVariable of the SemiJoinNode assigned to the IN LIST predicate. The outputVariable will be filtered in a later stage.
This rule has to be used together with another rule, InlineProjectionsOnValues on #23245

Test Plan

Test the rule fires on in the scenario and returns correct query plan.

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

== RELEASE NOTES ==

General Changes
* Add session property ``transform_in_values_to_in_filter`` and configuration property ``optimizer.transform_in_values_to_in_filter`` to transform key IN (VALUES ...) form into IN LIST form when VALUES table only has one column :pr:'23161'.
* Add session property ``inline_projections_on_values`` and configuration property ``optimizer.inline-projections-on-values`` to evaluate project node on values node :pr:`23245`.

elharo

Does this resolve #22728?

jackychen718 · 2024-07-10T13:51:08Z

Yes. It resolves #22728.

elharo

If you say

fixes #22728

in the PR description then Github autocloses the bug when the PR is merged.

jackychen718 · 2024-07-10T14:22:32Z

Thanks for your advice.

steveburnett · 2024-07-10T15:10:06Z

(Edit to acknowledge a release note entry has been added - please ignore this comment.)

If this doesn't need a release note - and I don't believe it does - please add this so this PR isn't in the "Missing Release Notes" section of the release notes process (see #23079 for an example).

Release Notes

== NO RELEASE NOTE ==

feilong-liu · 2024-07-10T17:41:23Z

...rc/main/java/com/facebook/presto/sql/planner/iterative/rule/TransformInValuesToInFilter.java

+        implements Rule<SemiJoinNode>
+{
+    private static final Pattern<SemiJoinNode> PATTERN = semiJoin().with(filteringSource()
+            .matching(project().with(source()


It's possible that there are multiple projects between values and semi join nodes

yes. I see that, in previous version, it has more than one projects. However, in current version, it has only one project there.Do you have some ideas how can I match multiple levels to catch the ValuesNode?

Not sure about the pattern but values->project* should still be constant? Maybe we have a separate inline constant values rule that'll be generally useful and this rule. So if you have something like:

VALUES('abcd')->Project(subtr(field_0, 1, 2))

you can rewrite that as:

VALUES('ab')

Using the RowExpressionInterpreter?

feilong-liu · 2024-07-10T17:42:35Z

...rc/main/java/com/facebook/presto/sql/planner/iterative/rule/TransformInValuesToInFilter.java

+    public Result apply(SemiJoinNode semiJoinNode, Captures captures, Context context)
+    {
+        PlanNode source = semiJoinNode.getSource();
+        return context.getLookup().resolveGroup(semiJoinNode.getFilteringSource()).findFirst()


Having the whole logic within the single return statement may not be a good idea.

Yeah makes it hard to read

feilong-liu · 2024-07-10T17:45:17Z

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/Patterns.java

                Optional.empty());
    }

+    public static Property<SemiJoinNode, PlanNode> filteringSource()


Put it in a statics class "SemiJoin" like other node specific properties below

kaikalur

Also add an e2e test

kaikalur · 2024-07-10T17:56:31Z

...rc/main/java/com/facebook/presto/sql/planner/iterative/rule/TransformInValuesToInFilter.java

+    public Result apply(SemiJoinNode semiJoinNode, Captures captures, Context context)
+    {
+        PlanNode source = semiJoinNode.getSource();
+        return context.getLookup().resolveGroup(semiJoinNode.getFilteringSource()).findFirst()


Yeah makes it hard to read

kaikalur · 2024-07-10T17:56:51Z

...rc/main/java/com/facebook/presto/sql/planner/iterative/rule/TransformInValuesToInFilter.java

+                .flatMap(projectNode -> context.getLookup().resolveGroup(projectNode.getSources().get(0)).findFirst())
+                .map(ValuesNode.class::cast)
+                .map(ValuesNode::getRows)
+                // check that all values are only a single row expression (no struct/row types)


Hmm why not struct/row? How about array types?

In fact the apply function works for struct/row and array. The only problem is how to match the pattern of variable projects between valuesNode and SemiJoinNode.

steveburnett · 2024-07-11T18:45:38Z

Nit of formatting the release note entry to use ` and not ', and add a space between the session property and the PR number.

== RELEASE NOTES ==

General Changes
* Add an optimization rule that transform key IN (VALUES ...) form into IN LIST form if VALUES table only has one column. The optimization is controlled by session property `transform_in_values_to_in_filter` :pr:`23161`

jackychen718 requested review from a team, feilong-liu and jaystarshot as code owners July 10, 2024 06:02

jackychen718 requested a review from presto-oss July 10, 2024 06:02

elharo reviewed Jul 10, 2024

View reviewed changes

elharo previously approved these changes Jul 10, 2024

View reviewed changes

feilong-liu reviewed Jul 10, 2024

View reviewed changes

kaikalur requested changes Jul 10, 2024

View reviewed changes

Transform in values to in filter

95fdd1d

jackychen718 dismissed elharo’s stale review via 95fdd1d September 17, 2024 05:00

jackychen718 force-pushed the InValues-to-InFilter branch from ee31b0a to 95fdd1d Compare September 17, 2024 05:00

jackychen718 requested a review from vivek-bharathan as a code owner September 17, 2024 05:00

jackychen718 requested review from elharo, feilong-liu and kaikalur September 17, 2024 12:44

Conversation

jackychen718 commented Jul 10, 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!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

jackychen718 commented Jul 10, 2024

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

jackychen718 commented Jul 10, 2024

Uh oh!

steveburnett commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaikalur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaikalur Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackychen718 Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveburnett commented Jul 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jackychen718 commented Jul 10, 2024 •

edited

Loading

steveburnett commented Jul 10, 2024 •

edited

Loading

kaikalur Jul 10, 2024 •

edited

Loading

jackychen718 Jul 12, 2024 •

edited

Loading