feat: support map_contains_key expression #3369
Conversation
peterxcli
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3369 +/- ##
============================================
+ Coverage 56.12% 59.98% +3.85%
- Complexity 976 1475 +499
============================================
Files 119 175 +56
Lines 11743 16172 +4429
Branches 2251 2681 +430
============================================
+ Hits 6591 9701 +3110
- Misses 4012 5119 +1107
- Partials 1140 1352 +212 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| val mapKeysExpr = scalarFunctionExprToProto("map_keys", mapExpr) | ||
|
|
||
| val mapContainsKeyExpr = scalarFunctionExprToProto("array_has", mapKeysExpr, keyExpr) |
| 100, | ||
| schemaGenOptions, | ||
| dataGenOptions) | ||
| } |
There was a problem hiding this comment.
minor : why are we disabling comet here ?
There was a problem hiding this comment.
sorry I didnt read what it means, and I was just copying from "read map[int, int] from parquet" test.
but I think im going to remove this test and write sql test directly as #3369 (comment)
|
|
||
| checkSparkAnswer( | ||
| spark.sql("SELECT map_contains_key(c14, element_at(map_keys(c14), 1)) FROM t1")) | ||
| checkSparkAnswer(spark.sql("SELECT map_contains_key(c14, 999999) FROM t1")) |
There was a problem hiding this comment.
checkSparkAnswerAndOperator checks if all the operators are comet native vs checkSparkAnswer which only validates the result DF. Any reason why we used the latter ?
coderfender
left a comment
coderfender
left a comment
There was a problem hiding this comment.
left test related comments
|
|
||
| // Empty map with string keys | ||
| checkSparkAnswerAndOperator(spark.sql( | ||
| "SELECT map_contains_key(map_from_arrays(CAST(array() AS array<string>), CAST(array() AS array<double>)), 'key') FROM t1")) |
There was a problem hiding this comment.
We might want to add additional tests to verify complex keys , literals and literals ?
There was a problem hiding this comment.
I'm trying my best to replicate the scala test suit in spark:
- apache/spark@04b821c/sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala#L609-L618
- apache/spark@04b821c/sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala#L1663-L1672
but I notice they also have some sql test for this function, let me merge all of them into the slt
|
Thanks @peterxcli! Can we add/migrate the tests to the new framework? https://datafusion.apache.org/comet/contributor-guide/sql-file-tests.html |
5 participants
Which issue does this PR close?
Closes: #3164
Rationale for this change
Comet does not currently support the Spark
map_contains_keyfunction, causing queries using this function to fall back to Spark's JVM execution instead of running natively on DataFusion.The MapContainsKey expression checks whether a given key exists in a map. It is implemented as a runtime-replaceable expression that internally uses ArrayContains on the map's keys to perform the lookup.
Supporting this expression would allow more Spark workloads to benefit from Comet's native acceleration.
What changes are included in this PR?
MapContainsKey->CometMapContainsKeyCometMapContainsKeyto convert the expr toarray_has(map_keys(map), key)map_keys#1788 had implemented themap_keysfunction, so I also mark it as done.How are these changes tested?
map_contains_keyempty map tests in spark: