You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "GideonPotok (via GitHub)" <gi...@apache.org> on 2024/05/06 15:40:46 UTC
[PR] SPARK-47353: Enable collation support for the Mode expression in Spark. [spark]
GideonPotok opened a new pull request, #46404:
URL: https://github.com/apache/spark/pull/46404
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592834974
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -17,28 +17,36 @@
package org.apache.spark.sql.catalyst.expressions.aggregate
+import scala.collection.mutable.TreeMap
+
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.analysis.{ExpressionBuilder, UnresolvedWithinGroup}
import org.apache.spark.sql.catalyst.expressions.{Ascending, Descending, Expression, ExpressionDescription, ImplicitCastInputTypes, SortOrder}
import org.apache.spark.sql.catalyst.trees.UnaryLike
import org.apache.spark.sql.catalyst.types.PhysicalDataType
-import org.apache.spark.sql.catalyst.util.GenericArrayData
+import org.apache.spark.sql.catalyst.util.{CollationFactory, GenericArrayData}
import org.apache.spark.sql.errors.QueryCompilationErrors
-import org.apache.spark.sql.types.{AbstractDataType, AnyDataType, ArrayType, BooleanType, DataType}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.{AbstractDataType, AnyDataType, ArrayType, BooleanType, DataType, StringType}
Review Comment:
```suggestion
import org.apache.spark.sql.types._
```
scalastyle suggests that we may use wildcard import in this case, which I think we should practice in order to keep the code clean and also minimize merge conflicts
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1594765070
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
I am happy to try additional alternative approaches, BTW.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1595553952
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
@uros-db
I also added a third approach to the experimental pr https://github.com/apache/spark/pull/46488
What kinds of cardinalities are expected? Eg even if the data frame has 1 Trillion Rows, would it not be highly rare use case to have 1 Trillion unique values? That might be worth thinking about when it comes to the range of what to benchmark.
And what percent of values in the data frames to be benchmarked should be duplicate values, within the same collation? It is really just one very specific case we are benchmarking right now, and at least in the experimental branch, it could be useful to play around with that while we evaluate different algorithms.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592822930
##########
sql/core/src/test/scala/org/apache/spark/sql/CollationStringExpressionsSuite.scala:
##########
@@ -800,6 +804,61 @@ class CollationStringExpressionsSuite
assert(collationMismatch.getErrorClass === "COLLATION_MISMATCH.EXPLICIT")
}
+ test("Support mode for string expression with collation") {
+ val query = "SELECT mode(collate('abc', 'utf8_binary'))"
+ checkAnswer(sql(query), Row("abc"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("utf8_binary")))
+ }
+
+ test("Support mode for string expression with collation ID on table") {
+ withTable("t") {
+ sql("CREATE TABLE t(i STRING) USING parquet")
+ sql("INSERT INTO t VALUES " +
+ "('a'), ('a'), ('a'), ('a'), ('a'), " +
+ "('b'), ('b'), ('b'), " +
+ "('B'), ('B'), ('B'), ('B')")
+ val query = "SELECT mode(collate(i, 'UTF8_BINARY_LCASE')) FROM t"
Review Comment:
```suggestion
val query = s"SELECT mode(collate(i, '${t.collationName}')) FROM t"
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592912017
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
@uros-db see above comments -- I forgot to tag you
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1594753431
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
@uros-db I basically implemented a proof of concept [here](https://github.com/apache/spark/pull/46488).
I believe the benchmarking I did there (see the PR description) maybe shows 28x slowdown rather then a 89x slowdown. (I know we had a 130x slowdown before, but I modified the benchmarks slightly and now it is the following
Here is for the originally proposed implementation (This PR) which was the Red-black tree / TreeMap
```
[info] collation unit benchmarks - mode - 10000 elements: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ---------------------------------------------------------------------------------------------------------------------------------
[info] UTF8_BINARY_LCASE - mode - 10000 elements 13 15 5 7.6 130.9 1.0X
[info] UNICODE - mode - 10000 elements 39 41 2 2.6 392.0 0.3X
[info] UTF8_BINARY - mode - 10000 elements 1 1 0 129.0 7.7 16.9X
[info] UNICODE_CI - mode - 10000 elements 39 41 2 2.6 391.4 0.3X
[info] Numerical Type - mode - 10000 elements 0 1 0 204.5 4.9 26.8X
[info] OpenJDK 64-Bit Server VM 21.0.2+13-LTS on Mac OS X 14.4.1
[info] Apple M3 Max
[info] collation unit benchmarks - mode - 5000 elements: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] --------------------------------------------------------------------------------------------------------------------------------
[info] UTF8_BINARY_LCASE - mode - 5000 elements 4 5 0 11.1 89.7 1.0X
[info] UNICODE - mode - 5000 elements 13 14 1 3.8 266.2 0.3X
[info] UTF8_BINARY - mode - 5000 elements 0 0 0 140.5 7.1 12.6X
[info] UNICODE_CI - mode - 5000 elements 13 14 1 3.8 265.1 0.3X
[info] Numerical Type - mode - 5000 elements 0 0 0 251.4 4.0 22.5X
```
And here are results for the OpenHashMap implementation seen [here](https://github.com/apache/spark/pull/46488)
```
collation unit benchmarks - mode - 10000 elements: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ---------------------------------------------------------------------------------------------------------------------------------
[info] UTF8_BINARY_LCASE - mode - 10000 elements 8 9 1 12.6 79.5 1.0X
[info] UNICODE - mode - 10000 elements 13 14 1 7.4 135.0 0.6X
[info] UTF8_BINARY - mode - 10000 elements 1 1 0 120.0 8.3 9.5X
[info] UNICODE_CI - mode - 10000 elements 14 14 1 7.3 137.9 0.6X
[info] Numerical Type - mode - 10000 elements 0 1 0 218.0 4.6 17.3X
[info] OpenJDK 64-Bit Server VM 21.0.2+13-LTS on Mac OS X 14.4.1
[info] Apple M3 Max
[info] collation unit benchmarks - mode - 5000 elements: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] --------------------------------------------------------------------------------------------------------------------------------
[info] UTF8_BINARY_LCASE - mode - 5000 elements 3 3 0 18.6 53.6 1.0X
[info] UNICODE - mode - 5000 elements 5 6 0 9.5 104.9 0.5X
[info] UTF8_BINARY - mode - 5000 elements 0 0 0 148.4 6.7 8.0X
[info] UNICODE_CI - mode - 5000 elements 5 5 0 11.1 90.3 0.6X
[info] Numerical Type - mode - 5000 elements 0 0 0 267.6 3.7 14.3X
```
The thing I notice though is the slowdown is really happening most accutely for the unicode collations. Is there some underlying reason they are so much less performance then either UTF8_BINARY Collation? Is it because it can't compare byte by byte? I get why that would affect the TreeMap implementation, but if it is affecting the prototype for use of OpenHashMap with different hashing function, maybe it is from the conversion to UTF8String by both implementations. Which is not related to choice of data structures.
Please let me know what you think.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression using a scala TreeMap (RB Tree) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1598048572
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,10 +76,30 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
Review Comment:
Some changes may be unavoidable, but since we can choose - let's opt for keeping the method signature intact
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #46404:
URL: https://github.com/apache/spark/pull/46404#issuecomment-2097319430
Can you fill the PR description please
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592822463
##########
sql/core/src/test/scala/org/apache/spark/sql/CollationStringExpressionsSuite.scala:
##########
@@ -800,6 +804,61 @@ class CollationStringExpressionsSuite
assert(collationMismatch.getErrorClass === "COLLATION_MISMATCH.EXPLICIT")
}
+ test("Support mode for string expression with collation") {
+ val query = "SELECT mode(collate('abc', 'utf8_binary'))"
+ checkAnswer(sql(query), Row("abc"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("utf8_binary")))
+ }
+
+ test("Support mode for string expression with collation ID on table") {
+ withTable("t") {
+ sql("CREATE TABLE t(i STRING) USING parquet")
+ sql("INSERT INTO t VALUES " +
+ "('a'), ('a'), ('a'), ('a'), ('a'), " +
+ "('b'), ('b'), ('b'), " +
+ "('B'), ('B'), ('B'), ('B')")
+ val query = "SELECT mode(collate(i, 'UTF8_BINARY_LCASE')) FROM t"
+ checkAnswer(sql(query), Row("b"))
+ }
+ }
Review Comment:
here would be a good idea to test multiple collation types, which are expected to return different results
consider using something like: `Seq("UTF8_BINARY_LCASE", "UNICODE", "UNICODE_CI").foreach(collationName =>`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592822463
##########
sql/core/src/test/scala/org/apache/spark/sql/CollationStringExpressionsSuite.scala:
##########
@@ -800,6 +804,61 @@ class CollationStringExpressionsSuite
assert(collationMismatch.getErrorClass === "COLLATION_MISMATCH.EXPLICIT")
}
+ test("Support mode for string expression with collation") {
+ val query = "SELECT mode(collate('abc', 'utf8_binary'))"
+ checkAnswer(sql(query), Row("abc"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("utf8_binary")))
+ }
+
+ test("Support mode for string expression with collation ID on table") {
+ withTable("t") {
+ sql("CREATE TABLE t(i STRING) USING parquet")
+ sql("INSERT INTO t VALUES " +
+ "('a'), ('a'), ('a'), ('a'), ('a'), " +
+ "('b'), ('b'), ('b'), " +
+ "('B'), ('B'), ('B'), ('B')")
+ val query = "SELECT mode(collate(i, 'UTF8_BINARY_LCASE')) FROM t"
+ checkAnswer(sql(query), Row("b"))
+ }
+ }
Review Comment:
here would be a good idea to test multiple collation types, which are expected to return different results
consider using something like: testCases -> foreach (like other 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592824395
##########
sql/core/src/test/scala/org/apache/spark/sql/CollationStringExpressionsSuite.scala:
##########
@@ -800,6 +804,61 @@ class CollationStringExpressionsSuite
assert(collationMismatch.getErrorClass === "COLLATION_MISMATCH.EXPLICIT")
}
+ test("Support mode for string expression with collation") {
+ val query = "SELECT mode(collate('abc', 'utf8_binary'))"
+ checkAnswer(sql(query), Row("abc"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("utf8_binary")))
+ }
+
+ test("Support mode for string expression with collation ID on table") {
+ withTable("t") {
+ sql("CREATE TABLE t(i STRING) USING parquet")
+ sql("INSERT INTO t VALUES " +
+ "('a'), ('a'), ('a'), ('a'), ('a'), " +
+ "('b'), ('b'), ('b'), " +
+ "('B'), ('B'), ('B'), ('B')")
+ val query = "SELECT mode(collate(i, 'UTF8_BINARY_LCASE')) FROM t"
+ checkAnswer(sql(query), Row("b"))
Review Comment:
```suggestion
checkAnswer(sql(query), Row(t.expectedResult))
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on PR #46404:
URL: https://github.com/apache/spark/pull/46404#issuecomment-2098391117
> Can you fill the PR description please
@HyukjinKwon 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592889564
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
+ )))
+ {
+ case (map, (key: String, count)) =>
+ map(org.apache.spark.unsafe.types.UTF8String.fromString(key)) =
+ map.getOrElse(org.apache.spark.unsafe.types.UTF8String.fromString(key), 0L) + count
+ map
+ case (map, (key: UTF8String, count)) =>
+ map(key) = map.getOrElse(key, 0L) + count
+ map
+ case (_, _) =>
+ throw new IllegalArgumentException("Mode expects non-null string type.")
Review Comment:
@uros-db Yep.
The buffer should never contain a key that is null, because it is not added to the map during Mode.update(). The behavior this supports is that, according to the [docs](https://spark.apache.org/docs/latest/api/sql/index.html): "NULL values are ignored. If all the values are NULL, or there are 0 rows, returns NULL."
The subtleties of all this are fresh in my mind from looking into it recently -- So just let me know if there is an aspect of this you need more details about and I am happy to explain.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1595049540
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,10 +76,30 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (isCollatedString(child)) {
+ val modeMap = buff.foldLeft(
Review Comment:
place `val collationId: Int = child.dataType.asInstanceOf[StringType].collationId` here so that avoid lazy.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,10 +76,30 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
Review Comment:
Could we avoid the change ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592911161
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
I will be running it soon in GHA and will then add the `...results.txt` report to the code change.
Here is a sneak Peek.
Note that once I make your other suggested changes, the UTF8_BINARY case will become the "Collation Not Enabled" Case, once I remove the collationEnabled flag from the Mode class signature...
```
[info] OpenJDK 64-Bit Server VM 21.0.2+13-LTS on Mac OS X 14.4.1
[info] Apple M3 Max
[info] collation unit benchmarks - mode: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] UTF8_BINARY_LCASE 1027 1198 241 0.1 10272.2 1.0X
[info] UNICODE 3249 3262 19 0.0 32485.8 0.3X
[info] UTF8_BINARY 805 806 1 0.1 8050.6 1.3X
[info] UNICODE_CI 3623 3632 13 0.0 36233.6 0.3X
[info] Collation Not Enabled 28 30 1 3.5 284.6 36.1X
[info] Numerical Type 28 29 1 3.6 277.5 37.0X
```
As you can see, the performance is impacted!
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
Here is my plan:
1. Get your thoughts on these performance results...
2. Analyze the computation complexity in theory. Being very standard java Collection classes, the TreeMap vs OpenHashMap documentation will surely will state what their complexity class/big-o is known to be. After that, I will provide this analysis in the PR description.
3. I can analyze the computation complexity empirically: I can either add to the benchmark or do an ad-hoc benchmark to determine how it grows as input grows the time grows .
Do you have any thoughts on what else I might want to look into?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592920580
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
were you able to define a custom hashing strategy for OpenHashMap in Scala? (instead of using TreeMap)
if that's possible, we could theoretically invoke hashCode from CollationFactory.Collation.hashFunction (instead of ordering)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1593161711
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
@uros-db `org.apache.spark.util.collection.OpenHashMap` is actually a spark data structure, so I could try to modify it in a second candidate PR and then see about having `Mode` rely on a `OpenHashMap` configured to be collation aware..
The way I could do it would be to modify `org.apache.spark.util.collection.OpenHashSet.Hasher.hash()` , specifically the override of hash within a class we would create `Hasher[String with Collation...]`, to branch to an alternative hashing method that is collation sensitive.
(I would not be modifying `org.apache.spark.util.collection.OpenHashSet#hashcode` , which distributes the hashes more evenly and is called on values returned by `Hasher.hash`)
This should work because `OpenHashMap` relies internally on `OpenHashSet` which uses `org.apache.spark.util.collection.OpenHashSet.Hasher`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1595561154
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,10 +76,30 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (isCollatedString(child)) {
+ val modeMap = buff.foldLeft(
Review Comment:
I am copying a pattern I have seen in other Expression implementations of collation. But I am fine making the suggested change.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,10 +76,30 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (isCollatedString(child)) {
+ val modeMap = buff.foldLeft(
Review Comment:
I am copying a pattern I have seen in other Expression implementations of collation. But I am fine with making the suggested change.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592826648
##########
sql/core/src/test/scala/org/apache/spark/sql/CollationStringExpressionsSuite.scala:
##########
@@ -800,6 +804,61 @@ class CollationStringExpressionsSuite
assert(collationMismatch.getErrorClass === "COLLATION_MISMATCH.EXPLICIT")
}
+ test("Support mode for string expression with collation") {
+ val query = "SELECT mode(collate('abc', 'utf8_binary'))"
+ checkAnswer(sql(query), Row("abc"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("utf8_binary")))
+ }
+
+ test("Support mode for string expression with collation ID on table") {
+ withTable("t") {
+ sql("CREATE TABLE t(i STRING) USING parquet")
+ sql("INSERT INTO t VALUES " +
+ "('a'), ('a'), ('a'), ('a'), ('a'), " +
+ "('b'), ('b'), ('b'), " +
+ "('B'), ('B'), ('B'), ('B')")
+ val query = "SELECT mode(collate(i, 'UTF8_BINARY_LCASE')) FROM t"
+ checkAnswer(sql(query), Row("b"))
+ }
+ }
+
+ test("Support mode for string expression with collation ID") {
+ val query = "SELECT mode(collate('lorem epsum', 'UTF8_BINARY_LCASE'))"
+ checkAnswer(sql(query), Row("lorem epsum"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("UTF8_BINARY_LCASE")))
+ }
+
+ test("Support Mode.eval(buffer)") {
+ val myMode = Mode(child = Literal("some_column_name"), collationEnabled = true)
+ val buffer = new OpenHashMap[AnyRef, Long](5)
+ buffer.update("b", 1L)
+ buffer.update("B", 1L)
+ buffer.update("c", 1L)
+ buffer.update("d", 1L)
+ buffer.update("a", 2L)
+ assert(myMode.eval(buffer).toString == "a")
+ val buffer2 = new OpenHashMap[AnyRef, Long](5)
+ buffer2.update(UTF8String.fromString("b"), 1L)
+ buffer2.update(UTF8String.fromString("B"), 1L)
+ buffer2.update(UTF8String.fromString("c"), 1L)
+ buffer2.update(UTF8String.fromString("d"), 1L)
+ buffer2.update(UTF8String.fromString("a"), 2L)
+ assert(myMode.eval(buffer2).toString == "a")
+ }
+
+ test("Support Mode.eval(buffer) with non-default collation") {
+ val myMode = Mode(
+ child = Literal.create("some_column_name", StringType("utf8_binary_lcase")),
+ collationEnabled = true)
+ val buffer = new OpenHashMap[AnyRef, Long](11)
+ buffer.update("b", 2L)
+ buffer.update("B", 2L)
+ buffer.update("c", 2L)
+ buffer.update("d", 2L)
+ buffer.update("a", 3L)
+ assert(myMode.eval(buffer).toString == "B")
+ }
Review Comment:
these two tests can also be combined using testCases -> foreach...
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on PR #46404:
URL: https://github.com/apache/spark/pull/46404#issuecomment-2099064413
just another quick note based on your previous PRs - no need to force push and disrupt the commit history, in general I think it's easier to follow changes in this PR as we comment & resolve conversations, and look at commits as they come
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1595558939
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,10 +76,30 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
Review Comment:
Either I am renaming here or line 83. If I change line 83 instead, I will have to change all of the references to buffer on L85 and L86. Do you have a particular preference and if so, why?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1594756311
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
Actually, the Numerical benchmark case should be ignored. Thus, For this implementation, the slowdown ranges from 16.9x to 56x. And for the prototype, it ranges from 9.5x to 15x
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592918394
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
so I think we can agree that a 130x slowdown isn't exactly great... perhaps we should look into other approaches - I would suggest that you open another PR for that tho
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1594756311
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
Actually, the Numerical benchmark case should be ignored for this analysis. The case we can compare the various cases against is the UTF8_BINARY case. Thus, For this implementation, the slowdown ranges from 16.9x to 56x. And for the prototype, it ranges from 9.5x to 15x
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592830626
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -162,10 +196,10 @@ object ModeBuilder extends ExpressionBuilder {
override def build(funcName: String, expressions: Seq[Expression]): Expression = {
val numArgs = expressions.length
if (numArgs == 0) {
- Mode(UnresolvedWithinGroup)
+ Mode(UnresolvedWithinGroup, collationEnabled = SQLConf.get.collationEnabled)
} else if (numArgs == 1) {
// For compatibility with function calls without WITHIN GROUP.
- Mode(expressions(0))
+ Mode(expressions(0), collationEnabled = SQLConf.get.collationEnabled)
Review Comment:
I think we generally don't branch expression instantiation based on the "collationEnabled" flag
the idea is usually to take the collationId from the string arguments (children), and use something like `supportsBinaryEquality` to branch execution in order to preserve original performance for UTF8_BINARY
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592820188
##########
sql/core/src/test/scala/org/apache/spark/sql/CollationStringExpressionsSuite.scala:
##########
@@ -800,6 +804,61 @@ class CollationStringExpressionsSuite
assert(collationMismatch.getErrorClass === "COLLATION_MISMATCH.EXPLICIT")
}
+ test("Support mode for string expression with collation") {
+ val query = "SELECT mode(collate('abc', 'utf8_binary'))"
+ checkAnswer(sql(query), Row("abc"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("utf8_binary")))
+ }
Review Comment:
you can remove this test, as it doesn't seem to do anything - i.e. we already know that mode supports regular strings (UTF8_BINARY)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on PR #46404:
URL: https://github.com/apache/spark/pull/46404#issuecomment-2099074984
> kudos for your effort on this ticket! I left some comments
>
> also, please try to ensure that your code doesn't introduce any breaking changes (for example, I see that `SQLQueryTestSuite` has a failing test for `Mode`) before requesting thorough review
>
> however, it's also good to verify the approach as soon as possible - so I would advise making these fixes and committing the benchmark results
@uros-db Thanks for the review! I hear you that I should have waited before I brought in the other reviewers.
Thanks for initial feedback!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression using a scala TreeMap (RB Tree) [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok closed pull request #46404: [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression using a scala TreeMap (RB Tree)
URL: https://github.com/apache/spark/pull/46404
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592833471
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
+ )))
+ {
+ case (map, (key: String, count)) =>
+ map(org.apache.spark.unsafe.types.UTF8String.fromString(key)) =
+ map.getOrElse(org.apache.spark.unsafe.types.UTF8String.fromString(key), 0L) + count
+ map
+ case (map, (key: UTF8String, count)) =>
+ map(key) = map.getOrElse(key, 0L) + count
+ map
+ case (_, _) =>
+ throw new IllegalArgumentException("Mode expects non-null string type.")
Review Comment:
just checking - is this how `Mode` handles null values currently in Spark (without the collation changes in this PR)
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
+ )))
+ {
+ case (map, (key: String, count)) =>
+ map(org.apache.spark.unsafe.types.UTF8String.fromString(key)) =
+ map.getOrElse(org.apache.spark.unsafe.types.UTF8String.fromString(key), 0L) + count
+ map
+ case (map, (key: UTF8String, count)) =>
+ map(key) = map.getOrElse(key, 0L) + count
+ map
+ case (_, _) =>
+ throw new IllegalArgumentException("Mode expects non-null string type.")
Review Comment:
just checking - is this how `Mode` handles null values currently in Spark?
(without the collation changes 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592832622
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,20 +78,46 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (child.dataType.isInstanceOf[StringType] && collationEnabled) {
+ val modeMap = buff.foldLeft(
+ new TreeMap[org.apache.spark.unsafe.types.UTF8String, Long]()(Ordering.comparatorToOrdering(
+ CollationFactory.fetchCollation(collationId).comparator
Review Comment:
I like the general approach of using TreeMap with custom comparator - however, I do worry about the performance
it's good that you added the benchmark, so we can take a look at the overall results?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592837929
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -17,28 +17,36 @@
package org.apache.spark.sql.catalyst.expressions.aggregate
+import scala.collection.mutable.TreeMap
+
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.analysis.{ExpressionBuilder, UnresolvedWithinGroup}
import org.apache.spark.sql.catalyst.expressions.{Ascending, Descending, Expression, ExpressionDescription, ImplicitCastInputTypes, SortOrder}
import org.apache.spark.sql.catalyst.trees.UnaryLike
import org.apache.spark.sql.catalyst.types.PhysicalDataType
-import org.apache.spark.sql.catalyst.util.GenericArrayData
+import org.apache.spark.sql.catalyst.util.{CollationFactory, GenericArrayData}
import org.apache.spark.sql.errors.QueryCompilationErrors
-import org.apache.spark.sql.types.{AbstractDataType, AnyDataType, ArrayType, BooleanType, DataType}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.{AbstractDataType, AnyDataType, ArrayType, BooleanType, DataType, StringType}
+import org.apache.spark.unsafe.types.UTF8String
import org.apache.spark.util.collection.OpenHashMap
+
case class Mode(
child: Expression,
mutableAggBufferOffset: Int = 0,
inputAggBufferOffset: Int = 0,
- reverseOpt: Option[Boolean] = None)
+ reverseOpt: Option[Boolean] = None,
+ collationEnabled: Boolean = false)
Review Comment:
I don't think we need the collationEnabled flag
instead, I think `collationId` should be enough to dictate whether to use the default (currently existing) implementation for UTF8_BINARY, or your (new, collation-aware) implementation
try using collationId to branch `eval` accordingly
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression using a scala TreeMap (RB Tree) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1598038412
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -70,10 +76,30 @@ case class Mode(
buffer
}
- override def eval(buffer: OpenHashMap[AnyRef, Long]): Any = {
- if (buffer.isEmpty) {
+ override def eval(buff: OpenHashMap[AnyRef, Long]): Any = {
+ if (buff.isEmpty) {
return null
}
+ val buffer = if (isCollatedString(child)) {
+ val modeMap = buff.foldLeft(
Review Comment:
I think this is fine either way, I think we should generally aim to include `collationId` as part of the expression field (as we've done for other expressions), and there should be no harm since it's a lazy val
on the other hand, collationId is used only in one place here, but still I'd keep it as it is
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on PR #46404:
URL: https://github.com/apache/spark/pull/46404#issuecomment-2103059407
> just another quick note based on your previous PRs - no need to force push and disrupt the commit history, in general I think it's easier to follow changes in this PR as we comment & resolve conversations, and look at commits as they come
@uros-db The issue I run into is then I can't rebase with master realistically, as there will be a large number of rounds of conflict resolution, instead of one round before and after the squashed commit. Do you mind if I go back to continuously squashing all my commits, for this reason?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] SPARK-47353: Enable collation support for the Mode expression in Spark. [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on PR #46404:
URL: https://github.com/apache/spark/pull/46404#issuecomment-2096348487
@uros-db This is ready for the initial round of review. Let me know what you think!
I left two todos, that I will add to this branch in the next day or two. They are:
1. finish the function to benchmark performance impact
2. since null is not a valid key for tree map, I have to add support for mode to be null, even when mode is on a string column, in which case the hash map gets copied over to treetop). I will then modify the test for this, accordingly.
This feature has been very interesting. Thanks for letting me claim it!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592822930
##########
sql/core/src/test/scala/org/apache/spark/sql/CollationStringExpressionsSuite.scala:
##########
@@ -800,6 +804,61 @@ class CollationStringExpressionsSuite
assert(collationMismatch.getErrorClass === "COLLATION_MISMATCH.EXPLICIT")
}
+ test("Support mode for string expression with collation") {
+ val query = "SELECT mode(collate('abc', 'utf8_binary'))"
+ checkAnswer(sql(query), Row("abc"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("utf8_binary")))
+ }
+
+ test("Support mode for string expression with collation ID on table") {
+ withTable("t") {
+ sql("CREATE TABLE t(i STRING) USING parquet")
+ sql("INSERT INTO t VALUES " +
+ "('a'), ('a'), ('a'), ('a'), ('a'), " +
+ "('b'), ('b'), ('b'), " +
+ "('B'), ('B'), ('B'), ('B')")
+ val query = "SELECT mode(collate(i, 'UTF8_BINARY_LCASE')) FROM t"
Review Comment:
```suggestion
val query = s"SELECT mode(collate(i, '$collationName')) FROM t"
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592825050
##########
sql/core/src/test/scala/org/apache/spark/sql/CollationStringExpressionsSuite.scala:
##########
@@ -800,6 +804,61 @@ class CollationStringExpressionsSuite
assert(collationMismatch.getErrorClass === "COLLATION_MISMATCH.EXPLICIT")
}
+ test("Support mode for string expression with collation") {
+ val query = "SELECT mode(collate('abc', 'utf8_binary'))"
+ checkAnswer(sql(query), Row("abc"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("utf8_binary")))
+ }
+
+ test("Support mode for string expression with collation ID on table") {
+ withTable("t") {
+ sql("CREATE TABLE t(i STRING) USING parquet")
+ sql("INSERT INTO t VALUES " +
+ "('a'), ('a'), ('a'), ('a'), ('a'), " +
+ "('b'), ('b'), ('b'), " +
+ "('B'), ('B'), ('B'), ('B')")
+ val query = "SELECT mode(collate(i, 'UTF8_BINARY_LCASE')) FROM t"
+ checkAnswer(sql(query), Row("b"))
+ }
+ }
+
+ test("Support mode for string expression with collation ID") {
+ val query = "SELECT mode(collate('lorem epsum', 'UTF8_BINARY_LCASE'))"
+ checkAnswer(sql(query), Row("lorem epsum"))
+ assert(sql(query).schema.fields.head.dataType.sameType(StringType("UTF8_BINARY_LCASE")))
Review Comment:
dataType check can be incorporated into the previous test, so this test can then be removed
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592827444
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/CollationBenchmark.scala:
##########
Review Comment:
did you run this benchmark? I wonder how this approach is performing
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on PR #46404:
URL: https://github.com/apache/spark/pull/46404#issuecomment-2098916903
@cloud-fan @MaxGekk @dbatomic just letting y'all know this is ready for first round of review. Thanks!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SPARK-47353][SQL] Enable collation support for the Mode expression [spark]
Posted by "GideonPotok (via GitHub)" <gi...@apache.org>.
GideonPotok commented on code in PR #46404:
URL: https://github.com/apache/spark/pull/46404#discussion_r1592899719
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -162,10 +196,10 @@ object ModeBuilder extends ExpressionBuilder {
override def build(funcName: String, expressions: Seq[Expression]): Expression = {
val numArgs = expressions.length
if (numArgs == 0) {
- Mode(UnresolvedWithinGroup)
+ Mode(UnresolvedWithinGroup, collationEnabled = SQLConf.get.collationEnabled)
} else if (numArgs == 1) {
// For compatibility with function calls without WITHIN GROUP.
- Mode(expressions(0))
+ Mode(expressions(0), collationEnabled = SQLConf.get.collationEnabled)
Review Comment:
will do!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org