You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "comphead (via GitHub)" <gi...@apache.org> on 2025/05/24 17:39:56 UTC
[PR] fix: support map_values, map_keys [datafusion-comet]
comphead opened a new pull request, #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #1781.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support `map_keys` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788#discussion_r2110170865
##########
native/core/src/execution/planner.rs:
##########
@@ -3232,4 +3232,113 @@ mod tests {
Ok(())
}
+
+ /*
+ Testing a nested types scenario map[struct, struct]
+
+ select map_keys(m).b from (
+ select map(named_struct('a', 1, 'b', 'n', 'c', 'x'), named_struct('a', 1, 'b', 'n', 'c', 'x')) m
+ */
+ #[tokio::test]
+ async fn test_nested_types_map_keys_by_index() -> Result<(), DataFusionError> {
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support `map_keys` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788#discussion_r2110170636
##########
spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala:
##########
@@ -303,4 +303,28 @@ class CometNativeReaderSuite extends CometTestBase with AdaptiveSparkPlanHelper
|""".stripMargin,
"select c0[0].a, c0[0].a from tbl")
}
+
+ test("native reader - select nested field from a complex map key using map_keys") {
+ testSingleLineQuery(
+ """
+ | select map(str0, str0) c0 from
+ | (
+ | select named_struct('a', cast(1 as long), 'b', cast(2 as long), 'c', cast(3 as long)) str0 union all
+ | select named_struct('a', cast(3 as long), 'b', cast(4 as long), 'c', cast(5 as long)) str0
+ | )
+ |""".stripMargin,
+ "select map_keys(c0).b from tbl")
+ }
+
+// test("native reader - select nested field from a complex map key using map_values") {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support `map_keys` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support `map_keys` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788#discussion_r2109770363
##########
spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala:
##########
@@ -303,4 +303,28 @@ class CometNativeReaderSuite extends CometTestBase with AdaptiveSparkPlanHelper
|""".stripMargin,
"select c0[0].a, c0[0].a from tbl")
}
+
+ test("native reader - select nested field from a complex map key using map_keys") {
+ testSingleLineQuery(
+ """
+ | select map(str0, str0) c0 from
+ | (
+ | select named_struct('a', cast(1 as long), 'b', cast(2 as long), 'c', cast(3 as long)) str0 union all
+ | select named_struct('a', cast(3 as long), 'b', cast(4 as long), 'c', cast(5 as long)) str0
+ | )
+ |""".stripMargin,
+ "select map_keys(c0).b from tbl")
+ }
+
+// test("native reader - select nested field from a complex map key using map_values") {
Review Comment:
Should this test be ignored with a link to an issue rather than commented out?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support `map_keys` [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788#discussion_r2109745284
##########
native/core/src/execution/planner.rs:
##########
@@ -3232,4 +3232,113 @@ mod tests {
Ok(())
}
+
+ /*
+ Testing a nested types scenario map[struct, struct]
+
+ select map_keys(m).b from (
+ select map(named_struct('a', 1, 'b', 'n', 'c', 'x'), named_struct('a', 1, 'b', 'n', 'c', 'x')) m
+ */
+ #[tokio::test]
+ async fn test_nested_types_map_keys_by_index() -> Result<(), DataFusionError> {
Review Comment:
nit: what does the by_index part of the name refer to?
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -862,7 +862,8 @@ abstract class CometTestBase
testName: String = "test",
tableName: String = "tbl",
sqlConf: Seq[(String, String)] = Seq.empty,
- debugCometDF: DataFrame => Unit = _ => ()): Unit = {
+ debugCometDF: DataFrame => Unit = _ => (),
+ checkCometOperator: Boolean = true): Unit = {
Review Comment:
Is this related?
##########
native/core/src/execution/planner.rs:
##########
@@ -3232,4 +3232,113 @@ mod tests {
Ok(())
}
+
+ /*
+ Testing a nested types scenario map[struct, struct]
+
+ select map_keys(m).b from (
+ select map(named_struct('a', 1, 'b', 'n', 'c', 'x'), named_struct('a', 1, 'b', 'n', 'c', 'x')) m
+ */
+ #[tokio::test]
+ async fn test_nested_types_map_keys_by_index() -> Result<(), DataFusionError> {
+ let session_ctx = SessionContext::new();
+
+ // generate test data in the temp folder
+ let test_data = "select map([named_struct('a', 1, 'b', 'n', 'c', 'x')], [named_struct('a', 1, 'b', 'n', 'c', 'x')]) c0";
Review Comment:
minor: can we use a different struct for the key and the value, just to make it clear we are reading the correct fields.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support `map_keys` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788#issuecomment-2907578088
`map_values` will be addressed in following PR. It got a correctness issue
```
- read map[struct, struct] from parquet *** FAILED *** (1 second, 478 milliseconds)
Results do not match for query:
Timezone: sun.util.calendar.ZoneInfo[id="America/Los_Angeles",offset=-28800000,dstSavings=3600000,useDaylight=true,transitions=185,lastRule=java.util.SimpleTimeZone[id=America/Los_Angeles,offset=-28800000,dstSavings=3600000,useDaylight=true,startYear=0,startMode=3,startMonth=2,startDay=8,startDayOfWeek=1,startTime=7200000,startTimeMode=0,endMode=3,endMonth=10,endDay=1,endDayOfWeek=1,endTime=7200000,endTimeMode=0]]
Timezone Env:
== Parsed Logical Plan ==
Project [map_values(map1#508970).id2 AS map_values(map1).id2#509026]
+- SubqueryAlias tbl
+- View (`tbl`, [map1#508970])
+- Relation [map1#508970] parquet
== Analyzed Logical Plan ==
map_values(map1).id2: array<bigint>
Project [map_values(map1#508970).id2 AS map_values(map1).id2#509026]
+- SubqueryAlias tbl
+- View (`tbl`, [map1#508970])
+- Relation [map1#508970] parquet
== Optimized Logical Plan ==
Project [map_values(map1#508970).id2 AS map_values(map1).id2#509026]
+- Relation [map1#508970] parquet
== Physical Plan ==
*(1) CometColumnarToRow
+- CometProject [map_values(map1).id2#509026], [map_values(map1#508970).id2 AS map_values(map1).id2#509026]
+- CometScan parquet [map1#508970] Batched: true, DataFilters: [], Format: CometParquet, Location: InMemoryFileIndex(1 paths)[file:/__w/datafusion-comet/datafusion-comet/spark/target/tmp/spark-062..., PartitionFilters: [], PushedFilters: [], ReadSchema: struct<map1:map<struct<id:bigint,id2:bigint,id3:bigint>,struct<id2:bigint>>>
== Results ==
== Results ==
!== Correct Answer - 5 == == Spark Answer - 5 ==
struct<map_values(map1).id2:array<bigint>> struct<map_values(map1).id2:array<bigint>>
![ArrayBuffer(3)] [ArrayBuffer(0)]
![ArrayBuffer(4)] [ArrayBuffer(3)]
![ArrayBuffer(null)] [ArrayBuffer(4)]
[null] [null]
[null] [null] (QueryTest.scala:244)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support `map_keys` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788#discussion_r2110176644
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -862,7 +862,8 @@ abstract class CometTestBase
testName: String = "test",
tableName: String = "tbl",
sqlConf: Seq[(String, String)] = Seq.empty,
- debugCometDF: DataFrame => Unit = _ => ()): Unit = {
+ debugCometDF: DataFrame => Unit = _ => (),
+ checkCometOperator: Boolean = true): Unit = {
Review Comment:
does not related directly, but this extends testing method functionality little bit to support tests which doesn't require all native comet operators. Used this extension to find that map_keys projection is not supported
##########
native/core/src/execution/planner.rs:
##########
@@ -3232,4 +3232,113 @@ mod tests {
Ok(())
}
+
+ /*
+ Testing a nested types scenario map[struct, struct]
+
+ select map_keys(m).b from (
+ select map(named_struct('a', 1, 'b', 'n', 'c', 'x'), named_struct('a', 1, 'b', 'n', 'c', 'x')) m
+ */
+ #[tokio::test]
+ async fn test_nested_types_map_keys_by_index() -> Result<(), DataFusionError> {
+ let session_ctx = SessionContext::new();
+
+ // generate test data in the temp folder
+ let test_data = "select map([named_struct('a', 1, 'b', 'n', 'c', 'x')], [named_struct('a', 1, 'b', 'n', 'c', 'x')]) c0";
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support `map_keys` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788#issuecomment-2914466138
Thanks everyone
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support `map_values`, `map_keys` [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1788:
URL: https://github.com/apache/datafusion-comet/pull/1788#issuecomment-2906975530
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/1788?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 57.20%. Comparing base [(`f09f8af`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`a9a359f`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/a9a359fe5b392b2bf1dc53ef31da2b7abb71dcd7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 214 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1788 +/- ##
============================================
+ Coverage 56.12% 57.20% +1.07%
- Complexity 976 1076 +100
============================================
Files 119 130 +11
Lines 11743 12634 +891
Branches 2251 2364 +113
============================================
+ Hits 6591 7227 +636
- Misses 4012 4198 +186
- Partials 1140 1209 +69
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/1788?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org