chore: Revert "chore: [1941-Part1]: Introduce map_sort scalar function (#2…

[comphead]

…262)"

This reverts commit f2baf95.

The function map_sort doesn't exist for Spark, array_sort does but not map_sort. This might be useful in the future but currently it would be more difficult to maintain the code which is not in use.

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

How are these changes tested?

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.66%. Comparing base (f09f8af) to head (98605f1).
⚠️ Report is 491 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2381      +/-   ##
============================================
+ Coverage     56.12%   57.66%   +1.54%     
- Complexity      976     1297     +321     
============================================
  Files           119      147      +28     
  Lines         11743    13512    +1769     
  Branches       2251     2390     +139     
============================================
+ Hits           6591     7792    +1201     
- Misses         4012     4451     +439     
- Partials       1140     1269     +129     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mbutrovich]

LGTM, thanks @comphead. Something to keep in our back pocket if Spark adds it.

[rishvin]

Hi @comphead / @mbutrovich : Spark does have MapSort internal expression. This piece was required to support grouping on map type. Spark applies it internally. Context here: #1941 (comment)

[comphead]

Hi @comphead / @mbutrovich : Spark does have MapSort internal expression. This piece was required to support grouping on map type. Spark applies it internally. Context here: #1941 (comment)

Thanks @rishvin the PR was saying scalar map_sort scalar function but this is MapSort operator to transform DataFrame . I think it would be nice to provide a test case where Spark falls back on MapSort so we can start from there?

[rishvin]

Hi @comphead / @mbutrovich : Spark does have MapSort internal expression. This piece was required to support grouping on map type. Spark applies it internally. Context here: #1941 (comment)

Thanks @rishvin the PR was saying scalar map_sort scalar function but this is MapSort operator to transform DataFrame . I think it would be nice to provide a test case where Spark falls back on MapSort so we can start from there?

Hi @comphead : This PR was split from my first PR: #2221.
The mentioned PR includes integration tests in the file CometAggregationSuite.scala. Without the required changes, tests will fail. Those tests cases verify the physical plan along with results and without required changes they will have spark's HashAggregate instead of CometHashAggregate. We fallback when we see grouping on map type here -

datafusion-comet/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

Line 1439 in 3b29cb9

withInfo(op, "Grouping on map types is not supported")

.

[comphead]

. We fallback when we see grouping on map type here -

datafusion-comet/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

Line 1439 in 3b29cb9

withInfo(op, "Grouping on map types is not supported")

Alright, now that makes a lot of sense. I'm creating an epic to address HashAggregate issues

where mapSort operator will be one of the tasks.

Feel free to include all the context including the test which you already provided in #1941 (comment)

We need to work a little bit how to decompose the mapSort story into manageable isolated PRs, where the end goal is finite, like test failed -> test passed. How does this sound?

#2382

[rishvin]

. We fallback when we see grouping on map type here -

datafusion-comet/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

Line 1439 in 3b29cb9

withInfo(op, "Grouping on map types is not supported")

Alright, now that makes a lot of sense. I'm creating an epic to address HashAggregate issues

where mapSort operator will be one of the tasks.

Feel free to include all the context including the test which you already provided in #1941 (comment)

We need to work a little bit how to decompose the mapSort story into manageable isolated PRs, where the end goal is finite, like test failed -> test passed. How does this sound?

#2382

Thanks @comphead , SG!