You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "uros-db (via GitHub)" <gi...@apache.org> on 2025/07/16 21:10:07 UTC
[PR] [SPARK-52828][SQL] Make hashing for collated strings collation agnostic [spark]
uros-db opened a new pull request, #51521:
URL: https://github.com/apache/spark/pull/51521
<!--
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?
We change the behavior of the `Murmur3Hash` and `XxHash64` catalyst expressions to be collation agnostic (i.e. collation-unaware). Also, we introduce two new internal catalyst expressions: `CollationAwareMurmur3Hash` and `CollationAwareXxHash64`, which are collation aware and take the collation of the string into consideration when hashing collated strings.
Furthermore, we replace `Murmur3Hash` and `XxHash64` in expressions where the hash expressions should be collation aware with `CollationAwareMurmur3Hash` and `CollationAwareXxHash64`. This is necessary for example when we do hash partitioning. Moreover, we change the way hashing is done for collated strings for the internal HiveHash expression to be consistent with the rest of the hashing expressions (the HiveHash expression is meant to always be collation-aware).
Finally, we add a kill switch (the SQL config is `COLLATION_AGNOSTIC_HASHING_ENABLED`) that allows to recover the previous behavior of `Murmur3Hash` and `XxHash64` as user-facing expressions. The kill switch has no effect on the new collation aware hashing expressions, or the HiveHash expression, which are internal and need to follow the new collation aware behavior.
### Why are the changes needed?
The `Murmur3Hash` and `XxHash64` catalyst expressions, when applied to collated strings, currently always take into consideration the collation of the string, that is they are collation aware. This is not the correct behavior, and these expressions should be collation agnostic by default instead.
### Does this PR introduce _any_ user-facing change?
Yes, see the detailed explanation above.
### How was this patch tested?
Updated existing tests in relevant suites: CollationFactorySuite, DistributionSuite, and HashExpressionsSuite. Also verified that the CollationSuite suite passes.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on PR #51521:
URL: https://github.com/apache/spark/pull/51521#issuecomment-3094758487
@cloud-fan Please re-review.
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2216473392
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -421,6 +424,9 @@ abstract class HashExpression[E] extends Expression {
s"$result = $hasherClassName.hashInt($input.months, $microsecondsHash);"
}
+ private def legacyCollationAwareHashing: Boolean =
Review Comment:
this is called once per input, can we make it a val to save cost?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -545,11 +580,32 @@ abstract class InterpretedHashFunction {
protected def hashUnsafeBytes(base: AnyRef, offset: Long, length: Int, seed: Long): Long
+ private def legacyCollationAwareHashing: Boolean =
Review Comment:
ditto
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on PR #51521:
URL: https://github.com/apache/spark/pull/51521#issuecomment-3080808321
@mkaravel @cloud-fan Please review.
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2212582726
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -421,6 +423,9 @@ abstract class HashExpression[E] extends Expression {
s"$result = $hasherClassName.hashInt($input.months, $microsecondsHash);"
}
+ private def isAlwaysCollationAwareBug: Boolean =
Review Comment:
shall we name it `isCollationAgnostic`?
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #51521:
URL: https://github.com/apache/spark/pull/51521#issuecomment-3163099390
The docker test failure is unrelated, thanks, merging to master!
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2258566199
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/CollationBenchmark.scala:
##########
@@ -92,7 +94,7 @@ abstract class CollationBenchmarkBase extends BenchmarkBase {
sublistStrings.foreach { _ =>
utf8Strings.foreach { s =>
(0 to 3).foreach { _ =>
- collation.hashFunction.applyAsLong(s)
+ Murmur3HashFunction.hash(s, StringType(collationType), 42L, true).toInt
Review Comment:
```suggestion
Murmur3HashFunction.hash(s, StringType(collationType), 42L, true, false).toInt
```
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2213610161
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -429,14 +434,43 @@ abstract class HashExpression[E] extends Expression {
val numBytes = s"$input.numBytes()"
s"$result = $hasherClassName.hashUnsafeBytes($baseObject, $baseOffset, $numBytes, $result);"
} else {
- val stringHash = ctx.freshName("stringHash")
- s"""
- long $stringHash = CollationFactory.fetchCollation(${stringType.collationId})
- .hashFunction.applyAsLong($input);
- $result = $hasherClassName.hashLong($stringHash, $result);
- """
+ if (isAlwaysCollationAwareBug && !isCollationAware) {
Review Comment:
Updated.
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2213552377
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -982,6 +982,14 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ lazy val COLLATION_AGNOSTIC_HASHING_ENABLED =
Review Comment:
Updated.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -982,6 +982,14 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ lazy val COLLATION_AGNOSTIC_HASHING_ENABLED =
+ buildConf("spark.sql.collation.collationAgnosticHashing.enabled")
Review Comment:
Updated.
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2212600082
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -421,6 +423,9 @@ abstract class HashExpression[E] extends Expression {
s"$result = $hasherClassName.hashInt($input.months, $microsecondsHash);"
}
+ private def isAlwaysCollationAwareBug: Boolean =
Review Comment:
The original issue was that hashing was unintentionally collation aware, instead of collation agnostic. This is why `isAlwaysCollationAwareBug` may be a better fit here.
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2217227528
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -545,11 +580,32 @@ abstract class InterpretedHashFunction {
protected def hashUnsafeBytes(base: AnyRef, offset: Long, length: Int, seed: Long): Long
+ private lazy val legacyCollationAwareHashing: Boolean =
+ SQLConf.get.getConf(SQLConf.COLLATION_AWARE_HASHING_ENABLED)
+
/**
- * Computes hash of a given `value` of type `dataType`. The caller needs to check the validity
- * of input `value`.
+ * This method is intended for callers using the old hash API and preserves compatibility for
+ * supported data types. It must only be used for data types that do not include collated strings
+ * or complex types (e.g., structs, arrays, maps) that may contain collated strings.
+ *
+ * The caller is responsible for ensuring that `dataType` does not involve collation-aware fields.
+ * This is validated via an internal assertion.
+ *
+ * @throws IllegalArgumentException if `dataType` contains non-UTF8 binary collation.
*/
def hash(value: Any, dataType: DataType, seed: Long): Long = {
+ require(!SchemaUtils.hasNonUTF8BinaryCollation(dataType))
+ // For UTF8_BINARY, hashing behavior is the same regardless of the isCollationAware flag.
+ hash(value = value, dataType = dataType, seed = seed, isCollationAware = false)
+ }
Review Comment:
LGTM for this part!
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2217228239
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -545,11 +580,32 @@ abstract class InterpretedHashFunction {
protected def hashUnsafeBytes(base: AnyRef, offset: Long, length: Int, seed: Long): Long
+ private lazy val legacyCollationAwareHashing: Boolean =
+ SQLConf.get.getConf(SQLConf.COLLATION_AWARE_HASHING_ENABLED)
+
/**
- * Computes hash of a given `value` of type `dataType`. The caller needs to check the validity
- * of input `value`.
+ * This method is intended for callers using the old hash API and preserves compatibility for
+ * supported data types. It must only be used for data types that do not include collated strings
+ * or complex types (e.g., structs, arrays, maps) that may contain collated strings.
+ *
+ * The caller is responsible for ensuring that `dataType` does not involve collation-aware fields.
+ * This is validated via an internal assertion.
+ *
+ * @throws IllegalArgumentException if `dataType` contains non-UTF8 binary collation.
*/
def hash(value: Any, dataType: DataType, seed: Long): Long = {
+ require(!SchemaUtils.hasNonUTF8BinaryCollation(dataType))
+ // For UTF8_BINARY, hashing behavior is the same regardless of the isCollationAware flag.
+ hash(value = value, dataType = dataType, seed = seed, isCollationAware = false)
+ }
Review Comment:
Thank you Milan!
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2217357548
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -421,6 +424,9 @@ abstract class HashExpression[E] extends Expression {
s"$result = $hasherClassName.hashInt($input.months, $microsecondsHash);"
}
+ private def legacyCollationAwareHashing: Boolean =
Review Comment:
Actually, I'm not sure whether we should do this. Caching a SQL config in the current session is not good, because there's no mechanism to effectively update/change the config value over time? i.e. this approach would always revert to using the original default sql config value in this context.
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2258819335
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1025,6 +1025,14 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ lazy val COLLATION_AWARE_HASHING_ENABLED =
+ buildConf("spark.sql.legacy.collationAwareHashFunctions")
+ .doc("Enables collation aware hashing (legacy behavior) for collated strings in " +
Review Comment:
```suggestion
.internal()
.doc("Enables collation aware hashing (legacy behavior) for collated strings in " +
```
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2212799812
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -429,14 +434,43 @@ abstract class HashExpression[E] extends Expression {
val numBytes = s"$input.numBytes()"
s"$result = $hasherClassName.hashUnsafeBytes($baseObject, $baseOffset, $numBytes, $result);"
} else {
- val stringHash = ctx.freshName("stringHash")
- s"""
- long $stringHash = CollationFactory.fetchCollation(${stringType.collationId})
- .hashFunction.applyAsLong($input);
- $result = $hasherClassName.hashLong($stringHash, $result);
- """
+ if (isAlwaysCollationAwareBug && !isCollationAware) {
Review Comment:
to make the branching logic clearer, how about
```
if (collationAware) {
...
} else if (legacyEnabled) {
...
} else {
...
}
```
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2218074008
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -421,6 +424,9 @@ abstract class HashExpression[E] extends Expression {
s"$result = $hasherClassName.hashInt($input.months, $microsecondsHash);"
}
+ private def legacyCollationAwareHashing: Boolean =
Review Comment:
This is the desired behavior, isn't it? We shouldn't apply a new config in the middle of query execution. The semantic should be fixed when the query plan has been planned.
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #51521: [SPARK-52828][SQL] Make hashing for collated strings collation agnostic
URL: https://github.com/apache/spark/pull/51521
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2216492979
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -421,6 +424,9 @@ abstract class HashExpression[E] extends Expression {
s"$result = $hasherClassName.hashInt($input.months, $microsecondsHash);"
}
+ private def legacyCollationAwareHashing: Boolean =
Review Comment:
Yes, this makes sense. Updating.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -545,11 +580,32 @@ abstract class InterpretedHashFunction {
protected def hashUnsafeBytes(base: AnyRef, offset: Long, length: Int, seed: Long): Long
+ private def legacyCollationAwareHashing: Boolean =
Review Comment:
Updating.
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2212799812
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala:
##########
@@ -429,14 +434,43 @@ abstract class HashExpression[E] extends Expression {
val numBytes = s"$input.numBytes()"
s"$result = $hasherClassName.hashUnsafeBytes($baseObject, $baseOffset, $numBytes, $result);"
} else {
- val stringHash = ctx.freshName("stringHash")
- s"""
- long $stringHash = CollationFactory.fetchCollation(${stringType.collationId})
- .hashFunction.applyAsLong($input);
- $result = $hasherClassName.hashLong($stringHash, $result);
- """
+ if (isAlwaysCollationAwareBug && !isCollationAware) {
Review Comment:
to make the branching logic clearer, how about
```
if (collationAware) {
... the proper collation aware behavior
} else if (legacyEnabled) {
... the legacy behavior that is also collation aware
} else {
... the proper collation agnostic behavior
}
```
--
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-52828][SQL] Make hashing for collated strings collation agnostic [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51521:
URL: https://github.com/apache/spark/pull/51521#discussion_r2212821708
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -982,6 +982,14 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ lazy val COLLATION_AGNOSTIC_HASHING_ENABLED =
+ buildConf("spark.sql.collation.collationAgnosticHashing.enabled")
Review Comment:
I'd name it `spark.sql.legacy.collationAwareHashFunctions`, and the default value is false.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -982,6 +982,14 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ lazy val COLLATION_AGNOSTIC_HASHING_ENABLED =
Review Comment:
```suggestion
val COLLATION_AGNOSTIC_HASHING_ENABLED =
```
--
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