mkaravel commented on code in PR #45218:
URL: https://github.com/apache/spark/pull/45218#discussion_r1500231222
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -463,6 +463,12 @@
],
"sqlState" : "42704"
},
+ "COLLATION_SUPPORT_DISABLED" : {
+ "message" : [
+ "Collation feature is under development and not all features are
supported. To enable existing features toggle `COLLATION_ENABLED` flag to true."
Review Comment:
```suggestion
"Collation support is under development and not all features are
supported. To enable existing features set `spark.sql.collation.enabled` to
`true`."
```
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -463,6 +463,12 @@
],
"sqlState" : "42704"
},
+ "COLLATION_SUPPORT_DISABLED" : {
+ "message" : [
+ "Collation feature is under development and not all features are
supported. To enable existing features toggle `COLLATION_ENABLED` flag to true."
Review Comment:
Out of curiosity: do we have or want any mechanism to disallow the
enablement of the feature?
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -463,6 +463,12 @@
],
"sqlState" : "42704"
},
+ "COLLATION_SUPPORT_DISABLED" : {
Review Comment:
How about renaming this to `COLLATION_SUPPORT_NOT_ENABLED`?
Somehow it feels better: we want to convey the information that the feature
is not enabled, rather than that it is disabled.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -289,14 +289,21 @@ private[sql] object QueryCompilationErrors extends
QueryErrorsBase with Compilat
}
def namedArgumentsNotEnabledError(functionName: String, argumentName:
String): Throwable = {
- new AnalysisException(
+ new SparkUnsupportedOperationException(
errorClass = "NAMED_PARAMETER_SUPPORT_DISABLED",
messageParameters = Map(
"functionName" -> toSQLId(functionName),
"argument" -> toSQLId(argumentName))
)
}
+ def collationDisabledError(): Throwable = {
Review Comment:
In the same spirit as my previous comment, I would rename this to
`collationNotEnabledError`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -289,14 +289,21 @@ private[sql] object QueryCompilationErrors extends
QueryErrorsBase with Compilat
}
def namedArgumentsNotEnabledError(functionName: String, argumentName:
String): Throwable = {
- new AnalysisException(
+ new SparkUnsupportedOperationException(
Review Comment:
Why are we changing this from an `AnalysisException` to a
`SparkUnsupportedOperationException`?
Another way to ask kind of the same thing: why are we not using
`SparkUnsupportedOperationException` in OSS?
##########
docs/sql-error-conditions.md:
##########
@@ -386,6 +386,12 @@ For more details see
[CODEC_NOT_AVAILABLE](sql-error-conditions-codec-not-availa
Cannot find a short name for the codec `<codecName>`.
+### COLLATION_SUPPORT_DISABLED
+
+[SQLSTATE:
0A000](sql-error-conditions-sqlstates.html#class-0A-feature-not-supported)
+
+Collation feature is under development and not all features are supported. To
enable existing features toggle `COLLATION_ENABLED` flag to true.
Review Comment:
```suggestion
Collation support is under development and not all features are supported.
To enable existing features set `spark.sql.collation.enabled` to `true`.
```
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollationExpressionSuite.scala:
##########
@@ -19,59 +19,75 @@ package org.apache.spark.sql.catalyst.expressions
import org.apache.spark.{SparkException, SparkFunSuite}
import org.apache.spark.sql.catalyst.util.CollationFactory
+import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._
class CollationExpressionSuite extends SparkFunSuite with ExpressionEvalHelper
{
test("validate default collation") {
- val collationId = CollationFactory.collationNameToId("UCS_BASIC")
- assert(collationId == 0)
- val collateExpr = Collate(Literal("abc"), "UCS_BASIC")
- assert(collateExpr.dataType === StringType(collationId))
- collateExpr.dataType.asInstanceOf[StringType].collationId == 0
- checkEvaluation(collateExpr, "abc")
+ withSQLConf(SQLConf.COLLATION_ENABLED.key -> "true") {
Review Comment:
+1
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collationExpressions.scala:
##########
@@ -67,6 +68,9 @@ object CollateExpressionBuilder extends ExpressionBuilder {
*/
case class Collate(child: Expression, collationName: String)
extends UnaryExpression with ExpectsInputTypes {
+ if (!SQLConf.get.collationEnabled) {
Review Comment:
+1
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollationExpressionSuite.scala:
##########
@@ -19,59 +19,75 @@ package org.apache.spark.sql.catalyst.expressions
import org.apache.spark.{SparkException, SparkFunSuite}
import org.apache.spark.sql.catalyst.util.CollationFactory
+import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._
class CollationExpressionSuite extends SparkFunSuite with ExpressionEvalHelper
{
test("validate default collation") {
- val collationId = CollationFactory.collationNameToId("UCS_BASIC")
- assert(collationId == 0)
- val collateExpr = Collate(Literal("abc"), "UCS_BASIC")
- assert(collateExpr.dataType === StringType(collationId))
- collateExpr.dataType.asInstanceOf[StringType].collationId == 0
- checkEvaluation(collateExpr, "abc")
+ withSQLConf(SQLConf.COLLATION_ENABLED.key -> "true") {
Review Comment:
Another option is to enable the feature for testing purposes and disable it
otherwise. I believe (not 100% sure) that this is doable if we define the
config as:
```scala
val COLLATION_ENABLED =
buildConf("spark.sql.collation.enabled")
.doc("Collations feature is under development and its use should be
done under this" +
"feature flag.")
.version("4.0.0")
.booleanConf
.createWithDefault(Utils.isTesting)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
