Remove redundant cast to varchar in join clause #21050

feilong-liu · 2023-10-05T22:22:55Z

Description

Rewrite query such as select select * from orders o join customer c on cast(o.custkey as varchar) = cast(c.custkey as varchar) to select select * from orders o join customer c on o.custkey = c.custkey

Motivation and Context

Varchar is expensive to hash, and join with varchar keys can be expensive. For the cases above, removing the cast can reduce the cost of join operation.

Impact

Test in production queries show it decrease both CPU and latency by half.

Test Plan

Unit tests and verifier run

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
* Add an optimization, which removes the redundant cast to varchar expression in join condition. The optimizer is controlled by session property `remove_redundant_cast_to_varchar_in_join` and enabled by default.

feilong-liu · 2023-10-06T22:27:41Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java

@@ -1253,7 +1257,7 @@ public void testPayloadJoins()
                "SELECT l.* FROM (select orderkey+1 as ok1, * from lineitem_map) l left join orders o on (l.ok1 = o.orderkey+1) left join part p on (l.partkey=p.partkey)",
                "SELECT l.* FROM (select *, cast(orderkey as int) as ok from lineitem_map) l left join (select *, cast(orderkey as int) as ok from orders) o on (l.ok = o.ok) left join (select *, cast(partkey as int) as pk from part) p on (l.ok=p.pk)",
                "with lm as (select quantity + 1 as q1, * from lineitem_map) SELECT l.* FROM (select * from lm) l left join orders o on (l.orderkey = o.orderkey) left join part p on (l.partkey=p.partkey)",
-
+                // This is the query which fail when REMOVE_REDUNDANT_CAST_TO_VARCHAR_IN_JOIN set to true
                "SELECT l.* FROM lineitem_map l left join orders o on (cast(l.orderkey as varchar) = cast(o.orderkey as varchar)) left join part p on (l.partkey=p.partkey)",


@mlyublena The optimization added surfaced a bug in payload join optimizer, i.e. when payload join optimization is not enabled, this query runs successfully with the optimization in this PR, but when payload join optimization is enabled, this query will fail with the optimization in this PR.

There is an outstanding PR that fixes a lot of bugs in payload join optimizer: #19904
My guess is that the case you found is already fixed there

feilong-liu · 2023-10-06T22:28:23Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

+                .build();
+        assertUpdate("create table lineitem_map as select *, map(ARRAY[1,3], ARRAY[2,4]) as m1, map(ARRAY[1,3], ARRAY[2,4]) as m2 from lineitem", 60175);
+        assertUpdate("create table part_map as select *, map(ARRAY[1,3], ARRAY[2,4]) as m3, map(ARRAY[1,3], ARRAY[2,4]) as m4 from part", 2000);
+        assertQueryWithSameQueryRunner(session, "SELECT l.* FROM lineitem_map l left join orders o on (cast(l.orderkey as varchar) = cast(o.orderkey as varchar)) left join part p on (l.partkey=p.partkey)", disabled);


Add the query found in payload join test here, make sure that it succeeds with this optimization, with payload join disabled.

...com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantCastToVarcharInJoinClause.java

mlyublena · 2023-10-10T20:52:40Z

...com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantCastToVarcharInJoinClause.java

+ * </pre>
+ * We will rely on optimizations later to remove unnecessary cast (if not used) and identity projection here.
+ *
+ * Notice that we do not apply similar optimizations to queries with similar join condition like `cast(bigint as varchar) = varchar`. In general it can be converted to


should we allow this transformation for all non-lossy/deterministic casts?

I applied for cast(bigint/integer as varchar) as varchar has high cost for hash operation. Other cases may not show as big improvement here.

mlyublena · 2023-10-10T20:53:46Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

@@ -282,6 +282,8 @@ public class FeaturesConfig

    private boolean preProcessMetadataCalls;

+    private boolean removeRedundantCastToVarcharInJoin = true;


why do we need a session property? isn't this always beneficial?

Just in case we need to revert it if any bugs found later.

...com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantCastToVarcharInJoinClause.java

mlyublena · 2023-10-11T22:34:36Z

...com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantCastToVarcharInJoinClause.java

+    {
+        PlanNode leftInput = context.getLookup().resolve(node.getLeft());
+        PlanNode rightInput = context.getLookup().resolve(node.getRight());
+        if (!(leftInput instanceof ProjectNode) || !(rightInput instanceof ProjectNode)) {


so that transformation won't work if the projection happens some levels down the tree?
for example

(select * from (select *, cast(key as varchar) as newkey) where ...) join T on newkey=cast(... as varchar)

Correct, it will not work. I tried to do it, but it will complicated this rule as we not only need to extract this pattern traversing the tree, but also need to propagate the variables used in the cast expression (it not propagated already) to the join node. Since this rule already covered the query which motivated this optimization, I decided to do it in a simplified way for now.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

mlyublena · 2023-10-11T22:40:24Z

...com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantCastToVarcharInJoinClause.java

+            RowExpression rightAssignment = ((CallExpression) rightProjectAssignment).getArguments().get(0);
+
+            if (!leftAssignment.getType().equals(rightAssignment.getType())) {
+                leftAssignment = castToBigInt(functionAndTypeManager, leftAssignment);


we don't need to do this if the expressions are already bigints

This is checked in castToBigInt function, with

if (rowExpression.getType().equals(BIGINT)) { return rowExpression; }

mlyublena

LGTM

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

github-actions · 2023-10-12T04:53:19Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff c4df72a...47c9b35.

No notifications.

feilong-liu requested a review from a team as a code owner October 5, 2023 22:22

feilong-liu requested a review from presto-oss October 5, 2023 22:22

feilong-liu marked this pull request as draft October 5, 2023 22:23

feilong-liu force-pushed the join_cast_varchar branch 2 times, most recently from 4d20653 to a9ba850 Compare October 6, 2023 22:24

feilong-liu marked this pull request as ready for review October 6, 2023 22:24

feilong-liu requested review from kaikalur, pranjalssh and mlyublena October 6, 2023 22:25

feilong-liu commented Oct 6, 2023

View reviewed changes

pranjalssh approved these changes Oct 10, 2023

View reviewed changes

...com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantCastToVarcharInJoinClause.java Outdated Show resolved Hide resolved

mlyublena reviewed Oct 10, 2023

View reviewed changes

feilong-liu force-pushed the join_cast_varchar branch 4 times, most recently from ebffe7d to a558373 Compare October 11, 2023 22:11