You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stefankandic (via GitHub)" <gi...@apache.org> on 2024/11/01 22:14:36 UTC
Re: [PR] [SPARK-50206][SQL] Added separate collation id for UTF8_BINARY and non-collated strings [spark]
stefankandic commented on code in PR #48737:
URL: https://github.com/apache/spark/pull/48737#discussion_r1826332674
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -394,7 +395,11 @@ private static Collation fetchCollation(int collationId) {
// instance.
assert (collationId >= 0 && getDefinitionOrigin(collationId)
== DefinitionOrigin.PREDEFINED);
- if (collationId == UTF8_BINARY_COLLATION_ID) {
+ if (collationId == NON_COLLATED_STRING_COLLATION_ID) {
+ // Skip cache.
+ return CollationSpecUTF8.NON_COLLATED_STRING_COLLATION;
+ }
+ else if (collationId == UTF8_BINARY_COLLATION_ID) {
Review Comment:
Do we even need the new collation spec? Why can't we just return the utf8 binary one? The idea is that these two should behave identically anyways
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -394,7 +395,11 @@ private static Collation fetchCollation(int collationId) {
// instance.
assert (collationId >= 0 && getDefinitionOrigin(collationId)
== DefinitionOrigin.PREDEFINED);
- if (collationId == UTF8_BINARY_COLLATION_ID) {
Review Comment:
can we call these it `IMPLICIT_UTF8_BINARY_COLLATION_ID` and `EXPLICIT_UTF8_BINARY_COLLATION_ID`?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/StringType.scala:
##########
@@ -74,7 +77,7 @@ class StringType private (val collationId: Int) extends AtomicType with Serializ
* `string` due to backwards compatibility.
*/
override def typeName: String =
- if (isUTF8BinaryCollation) "string"
+ if (isNonCollatedString) "string"
Review Comment:
i don't think we should change this
--
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