Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-25 Thread via GitHub 


drexler-sky commented on PR #1923:
URL: 
https://github.com/apache/datafusion-comet/pull/1923#issuecomment-3007078399

   I stepped into the code. The reason Comet falls back to Spark for the 
literal [] is that it goes to 
https://github.com/apache/datafusion-comet/blob/main/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala#L865.
 Maybe we can log a separate issue to address the supported DataType problem 
for complex types.


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-25 Thread via GitHub 


drexler-sky commented on PR #1923:
URL: 
https://github.com/apache/datafusion-comet/pull/1923#issuecomment-3007061357

   > "spark.sql.optimizer.excludedRules" -> 
"org.apache.spark.sql.catalyst.optimizer.ConstantFolding"
   
   I tried this, but it didn't work for me.
   
   


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-25 Thread via GitHub 


comphead commented on PR #1923:
URL: 
https://github.com/apache/datafusion-comet/pull/1923#issuecomment-3006672046

   @drexler-sky what if 
   ```
   "spark.sql.optimizer.excludedRules" -> 
"org.apache.spark.sql.catalyst.optimizer.ConstantFolding",
   ```
   ?


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-25 Thread via GitHub 


drexler-sky commented on PR #1923:
URL: 
https://github.com/apache/datafusion-comet/pull/1923#issuecomment-3006298989

   @andygrove Thanks for the suggestion! I have tried
   ```
  checkSparkAnswerAndOperator(spark.sql("""
   SELECT array_distinct(
 CASE WHEN _2 = _3
 THEN array(_4)
 ELSE array()
 END
   )
   FROM t1
 """))
   ```
   However, Spark still appears to replace the second array_distinct with `[]`.
   ```
   == Physical Plan ==
   *(1) Project [CASE WHEN (cast(_2#1 as smallint) = _3#2) THEN 
array_distinct(array(_4#3)) ELSE [] END AS array_distinct(CASE WHEN (_2 = _3) 
THEN array(_4) ELSE array() END)#44]
   +- *(1) CometColumnarToRow
  +- CometScan parquet [_2#1,_3#2,_4#3] Batched: true, DataFilters: [], 
Format: CometParquet, Location: InMemoryFileIndex(1 
paths)[file:/private/var/folders/2r/znvj4hhd3t1cp22pmw4m3h_4gn/T/spark-39...,
 PartitionFilters: [], PushedFilters: [], ReadSchema: 
struct<_2:tinyint,_3:smallint,_4:int>
   
   ```
   


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-25 Thread via GitHub 


andygrove commented on PR #1923:
URL: 
https://github.com/apache/datafusion-comet/pull/1923#issuecomment-3004790090

   > > ...1 more thing please add tests with empty array.
   > 
   > I tested array_distinct with an empty array.
   > 
   > ```
   > SELECT array_distinct(array()) FROM t1;
   > 
   > == Optimized Logical Plan ==
   > Project [[] AS array_distinct(array())#240]
   > +- Relation 
[_1#121,_2#122,_3#123,_4#124,_5#125L,_6#126,_7#127,_8#128,_9#129,_10#130,_11#131L,_12#132,_13#133,_14#134,_15#135,_16#136,_17#137,_18#138,_19#139,_20#140,_21#141,_id#142]
 parquet
   > 
   > == Physical Plan ==
   > *(1) Project [[] AS array_distinct(array())#240]
   > +- *(1) CometColumnarToRow
   >+- CometScan parquet [] Batched: true, DataFilters: [], Format: 
CometParquet, Location: InMemoryFileIndex(1 
paths)[file:/private/var/folders/2r/znvj4hhd3t1cp22pmw4m3h_4gn/T/spark-f9...,
 PartitionFilters: [], PushedFilters: [], ReadSchema: struct<>
   > ```
   > 
   > Spark uses an alias, `[] AS array_distinct(array())` , so it doesn't reach 
`case _: ArrayDistinct => convert(CometArrayDistinct`
   
   In this case, Spark is replacing the `array_distinct` expression with a 
literal at planning time. To test with an empty array you would need to force 
this to happen at query execution time. You can do this using a `CASE WHEN` 
expression, similar to other tests in this PR.


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-24 Thread via GitHub 


drexler-sky commented on PR #1923:
URL: 
https://github.com/apache/datafusion-comet/pull/1923#issuecomment-3002328767

   > ...1 more thing please add tests with empty array.
   
   I tested array_distinct with an empty array.
   ```
   SELECT array_distinct(array()) FROM t1;
   
   == Optimized Logical Plan ==
   Project [[] AS array_distinct(array())#240]
   +- Relation 
[_1#121,_2#122,_3#123,_4#124,_5#125L,_6#126,_7#127,_8#128,_9#129,_10#130,_11#131L,_12#132,_13#133,_14#134,_15#135,_16#136,_17#137,_18#138,_19#139,_20#140,_21#141,_id#142]
 parquet
   
   == Physical Plan ==
   *(1) Project [[] AS array_distinct(array())#240]
   +- *(1) CometColumnarToRow
  +- CometScan parquet [] Batched: true, DataFilters: [], Format: 
CometParquet, Location: InMemoryFileIndex(1 
paths)[file:/private/var/folders/2r/znvj4hhd3t1cp22pmw4m3h_4gn/T/spark-f9...,
 PartitionFilters: [], PushedFilters: [], ReadSchema: struct<>
   ```
   Spark uses an alias, `[] AS array_distinct(array())` , so it doesn't reach 
`case _: ArrayDistinct => convert(CometArrayDistinct`
   


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-24 Thread via GitHub 


drexler-sky commented on code in PR #1923:
URL: https://github.com/apache/datafusion-comet/pull/1923#discussion_r2165266799


##
spark/src/test/scala/org/apache/comet/CometArrayExpressionSuite.scala:
##
@@ -232,24 +232,42 @@ class CometArrayExpressionSuite extends CometTestBase 
with AdaptiveSparkPlanHelp
 }
   }
 
+  test("array_distinct") {

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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-24 Thread via GitHub 


andygrove commented on code in PR #1923:
URL: https://github.com/apache/datafusion-comet/pull/1923#discussion_r2164099829


##
spark/src/test/scala/org/apache/comet/CometArrayExpressionSuite.scala:
##
@@ -232,24 +232,42 @@ class CometArrayExpressionSuite extends CometTestBase 
with AdaptiveSparkPlanHelp
 }
   }
 
+  test("array_distinct") {

Review Comment:
   Could you add `array_distinct` to the list of supported array expressions in 
`docs/source/user-guide/expressions.md` and add a note about the compatibility 
issue.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-23 Thread via GitHub 


drexler-sky commented on PR #1923:
URL: 
https://github.com/apache/datafusion-comet/pull/1923#issuecomment-2998998589

   @andygrove @parthchandra @comphead Could you please take another look? The 
CI failure doesn't seem to be related to this PR.


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-23 Thread via GitHub 


drexler-sky commented on code in PR #1923:
URL: https://github.com/apache/datafusion-comet/pull/1923#discussion_r2162749959


##
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##
@@ -171,9 +184,9 @@ object CometArrayMax extends CometExpressionSerde {
   binding: Boolean): Option[ExprOuterClass.Expr] = {
 val arrayExprProto = exprToProto(expr.children.head, inputs, binding)
 
-val arrayContainsScalarExpr =
+val arrayMaxScalarExpr =

Review Comment:
   yes



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-23 Thread via GitHub 


comphead commented on code in PR #1923:
URL: https://github.com/apache/datafusion-comet/pull/1923#discussion_r2162734270


##
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##
@@ -171,9 +184,9 @@ object CometArrayMax extends CometExpressionSerde {
   binding: Boolean): Option[ExprOuterClass.Expr] = {
 val arrayExprProto = exprToProto(expr.children.head, inputs, binding)
 
-val arrayContainsScalarExpr =
+val arrayMaxScalarExpr =

Review Comment:
   is it a clean up?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-23 Thread via GitHub 


drexler-sky commented on code in PR #1923:
URL: https://github.com/apache/datafusion-comet/pull/1923#discussion_r2162673921


##
spark/src/test/scala/org/apache/comet/CometArrayExpressionSuite.scala:
##
@@ -232,24 +232,42 @@ class CometArrayExpressionSuite extends CometTestBase 
with AdaptiveSparkPlanHelp
 }
   }
 
+  test("array_distinct") {

Review Comment:
   Thanks for the comment. While I was testing the nulls, I found out that 
datafusion's array_distinct doesn't behave the same as spark's array_distinct. 
This is because datafusion first 
[sorts](https://github.com/apache/datafusion/blob/main/datafusion/functions-nested/src/set_ops.rs#L541)
 then removes duplicates while spark preserves the original order. Therefore I 
changed the code to implement `IncompatExpr`. 



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-23 Thread via GitHub 


parthchandra commented on code in PR #1923:
URL: https://github.com/apache/datafusion-comet/pull/1923#discussion_r2162015639


##
spark/src/test/scala/org/apache/comet/CometArrayExpressionSuite.scala:
##
@@ -232,24 +232,42 @@ class CometArrayExpressionSuite extends CometTestBase 
with AdaptiveSparkPlanHelp
 }
   }
 
+  test("array_distinct") {

Review Comment:
   Can we add a case with nulls(more than one) in the array. 



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-23 Thread via GitHub 


andygrove commented on PR #1923:
URL: 
https://github.com/apache/datafusion-comet/pull/1923#issuecomment-2997098005

   The CI test failure is unrelated to changes in this PR and is now fixed in 
main branch


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat: supports array_distinct [datafusion-comet]

2025-06-22 Thread via GitHub 


codecov-commenter commented on PR #1923:
URL: 
https://github.com/apache/datafusion-comet/pull/1923#issuecomment-2995032965

   ## 
[Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/1923?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 Report
   Attention: Patch coverage is `0%` with `6 lines` in your changes missing 
coverage. Please review.
   > Project coverage is 32.76%. 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 
[(`98fb791`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/98fb7910a8434f771ae19dc8daae9dced55f9f09?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 275 commits behind head on main.
   
   | [Files with missing 
lines](https://app.codecov.io/gh/apache/datafusion-comet/pull/1923?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Patch % | Lines |
   |---|---|---|
   | 
[...src/main/scala/org/apache/comet/serde/arrays.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/1923?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2Fserde%2Farrays.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9zZXJkZS9hcnJheXMuc2NhbGE=)
 | 0.00% | [5 Missing :warning: 
](https://app.codecov.io/gh/apache/datafusion-comet/pull/1923?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 |
   | 
[.../scala/org/apache/comet/serde/QueryPlanSerde.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/1923?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2Fserde%2FQueryPlanSerde.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9zZXJkZS9RdWVyeVBsYW5TZXJkZS5zY2FsYQ==)
 | 0.00% | [0 Missing and 1 partial :warning: 
](https://app.codecov.io/gh/apache/datafusion-comet/pull/1923?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 |
   
   Additional details and impacted files
   
   
   ```diff
   @@  Coverage Diff  @@
   ##   main#1923   +/-   ##
   =
   - Coverage 56.12%   32.76%   -23.36% 
   + Complexity  976  786  -190 
   =
 Files   119  130   +11 
 Lines 1174312756 +1013 
 Branches   2251 2407  +156 
   =
   - Hits   6591 4180 -2411 
   - Misses 4012 7608 +3596 
   + Partials   1140  968  -172 
   ```
   
   
   
   [:umbrella: View full report in Codecov by 
Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/1923?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).
   
:rocket: New features to boost your workflow: 
   
   - :snowflake: [Test 
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, 
report on failures, and find test suite problems.
   


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]