[SPARK-47102][SQL][COLLATION] Adding COLLATION_ENABLED config#45218
Closed
mihailomilosevic2001 wants to merge 14 commits intoapache:masterfrom
Closed
[SPARK-47102][SQL][COLLATION] Adding COLLATION_ENABLED config#45218mihailomilosevic2001 wants to merge 14 commits intoapache:masterfrom
mihailomilosevic2001 wants to merge 14 commits intoapache:masterfrom
Conversation
mihailomilosevic2001
force-pushed
the
SPARK-47102
branch
from
23f1d40 to
f36a767
Compare
mihailomilosevic2001
changed the title
dbatomic
reviewed
Feb 22, 2024
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
Outdated
Show resolved
Hide resolved
dbatomic
reviewed
Feb 22, 2024
...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collationExpressions.scala
Outdated
Show resolved
Hide resolved
dbatomic
reviewed
Feb 22, 2024
...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollationExpressionSuite.scala
Outdated
Show resolved
Hide resolved
mkaravel
reviewed
Feb 23, 2024
Contributor
There was a problem hiding this comment.
Suggested change
| "Collation feature is under development and not all features are supported. To enable existing features toggle `COLLATION_ENABLED` flag to true." | |
| "Collation support is under development and not all features are supported. To enable existing features set `spark.sql.collation.enabled` to `true`." |
Contributor
There was a problem hiding this comment.
Out of curiosity: do we have or want any mechanism to disallow the enablement of the feature?
Contributor
Author
There was a problem hiding this comment.
If I understood correctly, we want to make sure that customer is prevented from trying to use partially built feature and getting in trouble because some functionality was not implemented at the time.
Contributor
There was a problem hiding this comment.
This is a good question. I am not sure what is the policy here - @cloud-fan and @MaxGekk - can you guide us here?
Contributor
There was a problem hiding this comment.
Preventing the use of a partially baked feature is a good reason here.
...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collationExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
Outdated
Show resolved
Hide resolved
...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollationExpressionSuite.scala
Outdated
Show resolved
Hide resolved
...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollationExpressionSuite.scala
Outdated
Show resolved
Hide resolved
Contributor
|
One more thing: it would be nice to add the |
mihailomilosevic2001
force-pushed
the
SPARK-47102
branch
from
8415953 to
2b85cf9
Compare
mihailomilosevic2001
changed the title
dbatomic
reviewed
Feb 23, 2024
...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollationExpressionSuite.scala
Outdated
Show resolved
Hide resolved
dbatomic
reviewed
Feb 23, 2024
sql/core/src/test/scala/org/apache/spark/sql/errors/QueryCompilationErrorsSuite.scala
Outdated
Show resolved
Hide resolved
Contributor
Author
|
PR should be ready now, but until I get GitHub Actions fixed for my account, CI runs can't be run before merging. Will ping when that gets fixed. |
MaxGekk
requested changes
Feb 26, 2024
mihailomilosevic2001
force-pushed
the
SPARK-47102
branch
from
0ef3f97 to
86a9a58
Compare
dbatomic
approved these changes
Feb 27, 2024
mihailomilosevic2001
force-pushed
the
SPARK-47102
branch
from
84eec82 to
b3a1ea0
Compare
Contributor
Author
|
Switched to #45285 as GitHub Actions are not enabled on my account. Please provide review on that PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR adds
COLLATION_ENABLEDconfig toSQLConfand introduces new error classCOLLATION_SUPPORT_NOT_ENABLEDto appropriately report error on usage of feature under development.Why are the changes needed?
We want to make collations configurable on this flag. These changes disable usage of
collateandcollationfunctions, along with anyCOLLATEsyntax when the flag is set to false. By default, the flag is set to false.Does this PR introduce any user-facing change?
Yes. It introduces new error along with an appropriate message.
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No.