Extend remove map cast rule to cover map_subset function#25395
Merged
feilong-liu merged 1 commit intoprestodb:masterfrom Jun 26, 2025
Merged
Extend remove map cast rule to cover map_subset function#25395feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu merged 1 commit intoprestodb:masterfrom
Conversation
feilong-liu
requested review from
a team,
elharo,
jaystarshot and
vivek-bharathan
as code owners
| .setSystemProperty(REMOVE_MAP_CAST, "false") | ||
| .build(); | ||
| assertQueryWithSameQueryRunner(enableOptimization, "select map_subset(feature, array[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]), 4)) t(feature, key)", disableOptimization); | ||
| assertQueryWithSameQueryRunner(enableOptimization, "select map_subset(feature, array[1, 3]) 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]), 4)) t(feature, key)", disableOptimization); |
Member
There was a problem hiding this comment.
As I see, this query wouldn't trigger the new optimization logic, since array[1, 3] would be set as type array<int>, so there is no CAST for feature. Is this intentional, or should we use map_subset(feature, array[1, cast(3 as bigint)]) in order to trigger the optimization logic?
Contributor
Author
There was a problem hiding this comment.
Yeah, you are right. Thanks for pointing out, just fixed it.
By the way, there is another PR #25394 which optimizes queries with map_subset functions, greatly appreciate if you could take a look when you have time. Thanks!
feilong-liu
force-pushed
the
map_cast_mapsubset
branch
from
f4dd52c to
0552999
Compare
Contributor
|
Thanks for the release note! Suggest adding a link to the doc in the release note, like this: |
feilong-liu
force-pushed
the
map_cast_mapsubset
branch
2 times, most recently
from
97ea4c5 to
b657961
Compare
6 tasks
rschlussel
previously approved these changes
Jun 26, 2025
Contributor
rschlussel
left a comment
rschlussel
left a comment
There was a problem hiding this comment.
Looks good. just recommend changing the description and variable names from using "index" to using "key" to be clearer.
| return call(functionAndTypeManager, "element_at", node.getType(), castInput, newIndex); | ||
| } | ||
| else if (functionResolution.isMapSubSetFunction(node.getFunctionHandle())) { | ||
| RowExpression newIndex = null; |
| Type arrayElementType = ((ArrayType) constantArray.getType()).getElementType(); | ||
| ImmutableList.Builder<RowExpression> arguments = ImmutableList.builder(); | ||
| for (int i = 0; i < arrayValue.getPositionCount(); ++i) { | ||
| ConstantExpression mapIndex = constant(readNativeValue(arrayElementType, arrayValue, i), arrayElementType); |
| return null; | ||
| } | ||
|
|
||
| private static boolean canRemoveMapCast(Type fromKeyType, Type fromValueType, Type toKeyType, Type toValueType, Type indexType) |
Contributor
There was a problem hiding this comment.
indexType -> subsetKeysType?
feilong-liu
force-pushed
the
map_cast_mapsubset
branch
2 times, most recently
from
6a1fb85 to
087427e
Compare
feilong-liu
force-pushed
the
map_cast_mapsubset
branch
from
087427e to
dd0e2d3
Compare
hantangwangd
approved these changes
Jun 26, 2025
This was referenced Jul 4, 2025
This was referenced Jul 24, 2025
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Rewrite to remove map cast in map_subset function.
map_subset(cast(feature as map<bigint, double>), array[k1, k2])where feature is of type map<integer, double> and key is of type arraycast(map_subset(feature, array[try_cast(k1 as integer), try_cast(k2 as integer)]) as map<bigint, double>)When k1, or k2 is out of integer range, try_cast will return NULL, where map_subset will not return values for this key, which is the same behavior for both input and output
This together with #25394 will enable subfield pushdown for feature map accessed in
map_subset(cast(feature as map<bigint, double>), array[k1, k2]).Also cast a subset of the map is cheaper than casting the whole map.
Motivation and Context
As in description
Impact
Optimization of map_subset function which is often used in feature extraction in ML workload
Test Plan
Add unit tests
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.