Remove map cast for element access #22059

feilong-liu · 2024-03-01T19:32:15Z

Rewrite expression from cast(feature as map<bigint, float>)[key] -> feature[cast(key as integer)], where feature is of type map<integer, float> and key is of type bigint, and element_at(cast(feature as map<bigint, float>), key) -> element_at(feature, try_cast(key as integer)), where feature is of type map<integer, float> and key is of type bigint, so as to get rid of map cast function.

Description

Map cast is expensive, this PR tries to get rid of map cast when possible, it currently targets the following cases:

cast(feature as map<bigint, float>)[key] -> feature[cast(key as integer)], where feature is of type map<integer, float> and key is of type bigint
element_at(cast(feature as map<bigint, float>), key) -> element_at(feature, try_cast(key as integer)), where feature is of type map<integer, float> and key is of type bigint

Notice that here when it's accessing the map using subscript function, we use CAST function in index, and when it's element_at function, we use TRY_CAST function, so that

when the key is out of integer range, for feature[key] it will fail both with and without optimization, fail with map key not exists before optimization and with cast failure after optimization
when the key is out of integer range, for element_at(feature, key) it will return NULL both before and after optimization

Motivation and Context

Get rid of the expensive map cast expression.

Impact

Get rid of map cast for cost for these expressions.

Test Plan

Unit tests

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 optimizer rule to get rid of map cast in map access functions when possible

jaystarshot · 2024-03-01T21:12:24Z

presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java

+                        REMOVE_MAP_CAST,
+                        "Remove map cast when possible",
+                        false,
+                        false),


You might need to add this to features config to make this into a config property

You might need to add this to features config to make this into a config property

If this becomes a new feature property, please add documentation for it.

@feilong-liu can you add a configuration property for this as per @jaystarshot's suggestion? Session properties should always have a corresponding config property.

@feilong-liu can you add a configuration property for this as per @jaystarshot's suggestion? Session properties should always have a corresponding config property.

We are trying to move away from that model for optimizations that we know are safe (famous last words lol) to reduce noise. So feature config is only for features and not for optimizations.

Adding a configuration doesn't seem especially noisy to me, but it does allow people to disable the feature for all queries if there ends up being a bug or regression in the optimizer (not everyone has infrastructure set up to easily tack session properties on to all user sessions if needed).

kaikalur · 2024-03-01T22:27:00Z

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveMapCastRule.java

+                Type toKeyType = ((MapType) castExpression.getType()).getKeyType();
+                Type toValueType = ((MapType) castExpression.getType()).getValueType();
+
+                if (toValueType.equals(fromValueType) && fromKeyType.equals(INTEGER) && toKeyType.equals(BIGINT) && node.getArguments().get(1).getType().equals(BIGINT)) {


Put it in a separate function

kaikalur · 2024-03-01T22:27:59Z

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

+        assertQuery(enableOptimization, "select element_at(feature, key) from (values (map(array[cast(1 as integer), 2, 3, 4], array[0.3, 0.5, 0.9, 0.1]), cast(2 as bigint)), (map(array[cast(1 as integer), 2, 3, 4], array[0.3, 0.5, 0.9, 0.1]), 400000000000)) t(feature, key)",
+                "values 0.5, null");
+        assertQueryFails(enableOptimization, "select feature[key] from (values (map(array[cast(1 as integer), 2, 3, 4], array[0.3, 0.5, 0.9, 0.1]), cast(2 as bigint)), (map(array[cast(1 as integer), 2, 3, 4], array[0.3, 0.5, 0.9, 0.1]), 400000000000)) t(feature, key)",
+                ".*Out of range for integer.*");


Try some unrelated ones where this rule should not trigger - like varchar, map<varchar,float> etc

mlyublena · 2024-03-01T23:02:36Z

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveMapCastRule.java

+
+/**
+ * Remove cast on map if possible. Currently it only supports subscript and element_at function, and only works when map key is of type integer and index is bigint. For example:
+ * cast(feature as map<bigint, float>)[key] -> feature[cast(key as integer)], where feature is of type map<integer, float> and key is of type bigint


nit: can you put the before and after on separate lines for readability? something like this:

For arguments feature: map<integer, float>, key: bigint Input: cast(feature as map<bigint, float>)[key] Output: feature[cast(key as integer)]

mlyublena · 2024-03-01T23:03:04Z

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveMapCastRule.java

+/**
+ * Remove cast on map if possible. Currently it only supports subscript and element_at function, and only works when map key is of type integer and index is bigint. For example:
+ * cast(feature as map<bigint, float>)[key] -> feature[cast(key as integer)], where feature is of type map<integer, float> and key is of type bigint
+ * element_at(cast(feature as map<bigint, float>), key) -> element_at(feature, try_cast(key as integer)), where feature is of type map<integer, float> and key is of type bigint


where is the cast added in the first place? parser/analyzer?

Type promotion - ideally we should fix the type promo. For example:

element_at(map(int, real), bigint)

promotes map to map(bigint, real) instead we should coerce bigint to int? But I think our function resolution logic maybe inadequate for that.

So for now, we just decided to drop these unnecessary casts. This helps a lot in our current prod workloads as there is a lot of mixing of int and bigint in keys and map

mlyublena · 2024-03-01T23:04:10Z

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveMapCastRule.java

+                Type toKeyType = ((MapType) castExpression.getType()).getKeyType();
+                Type toValueType = ((MapType) castExpression.getType()).getValueType();
+
+                if (toValueType.equals(fromValueType) && fromKeyType.equals(INTEGER) && toKeyType.equals(BIGINT) && node.getArguments().get(1).getType().equals(BIGINT)) {


why do we special case the int/bigint pair? shouldn't this work for any cast that is not lossy?

Non-exact types as keys is dubious (and also not common) so we don't want to do that.

sounds good, if we extract this in a function as suggested above we can also extend this in the future to handle other pairs if needed

Rewrite expression from cast(feature as map<bigint, float>)[key] -> feature[cast(key as integer)], where feature is of type map<integer, float> and key is of type bigint, and element_at(cast(feature as map<bigint, float>), key) -> element_at(feature, try_cast(key as integer)), where feature is of type map<integer, float> and key is of type bigint, so as to get rid of map cast function.

feilong-liu requested a review from a team as a code owner March 1, 2024 19:32

feilong-liu requested a review from presto-oss March 1, 2024 19:32

jaystarshot reviewed Mar 1, 2024

View reviewed changes

feilong-liu requested review from kaikalur, pranjalssh, rschlussel and mlyublena March 1, 2024 21:55

kaikalur previously approved these changes Mar 1, 2024

View reviewed changes

mlyublena reviewed Mar 1, 2024

View reviewed changes

feilong-liu dismissed kaikalur’s stale review via cf0b50c March 4, 2024 19:17

feilong-liu force-pushed the skip_map_cast_int_bigint branch from 0983047 to cf0b50c Compare March 4, 2024 19:17

feilong-liu requested a review from mlyublena March 4, 2024 19:25

mlyublena approved these changes Mar 4, 2024

View reviewed changes

pranjalssh approved these changes Mar 6, 2024

View reviewed changes

feilong-liu force-pushed the skip_map_cast_int_bigint branch from cf0b50c to 91aa1b1 Compare March 6, 2024 20:49

feilong-liu merged commit 4629ca5 into prestodb:master Mar 7, 2024
56 checks passed

feilong-liu deleted the skip_map_cast_int_bigint branch March 7, 2024 23:49

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove map cast for element access #22059

Remove map cast for element access #22059

feilong-liu commented Mar 1, 2024

jaystarshot Mar 1, 2024

steveburnett Mar 4, 2024

rschlussel Mar 14, 2024

kaikalur Mar 14, 2024

rschlussel Mar 14, 2024

kaikalur Mar 1, 2024

mlyublena Mar 1, 2024

kaikalur Mar 1, 2024

mlyublena Mar 1, 2024

mlyublena Mar 1, 2024

kaikalur Mar 1, 2024

mlyublena Mar 1, 2024

kaikalur Mar 1, 2024

mlyublena Mar 1, 2024

Remove map cast for element access #22059

Remove map cast for element access #22059

Conversation

feilong-liu commented Mar 1, 2024

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

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

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