Move Enablement Config and Incorporate Feedback

apache · Feb 28, 2024 · a9f2c19 · a9f2c19
1 parent 16b468e
commit a9f2c19
Show file tree

Hide file tree

Showing 8 changed files with 58 additions and 29 deletions.
diff --git a/common/utils/src/main/resources/error/error-classes.json b/common/utils/src/main/resources/error/error-classes.json
@@ -3856,7 +3856,7 @@
       },
       "COLLATION" : {
         "message" : [
-          "Collation support is under development and not all features are supported. To enable existing features set <collationEnabled> to `true`."
+          "Collation feature is not yet supported."
         ]
       },
       "COMBINATION_QUERY_RESULT_CLAUSES" : {

diff --git a/docs/sql-error-conditions-unsupported-feature-error-class.md b/docs/sql-error-conditions-unsupported-feature-error-class.md
@@ -60,7 +60,7 @@ Catalog `<catalogName>` does not support `<operation>`.
 
 ## COLLATION
 
-Collation support is under development and not all features are supported. To enable existing features set `<collationEnabled>` to `true`.
+Collation feature is not yet supported.
 
 ## COMBINATION_QUERY_RESULT_CLAUSES
 

diff --git a/...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collationExpressions.scala b/...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collationExpressions.scala
@@ -41,18 +41,17 @@ import org.apache.spark.sql.types._
   group = "string_funcs")
 object CollateExpressionBuilder extends ExpressionBuilder {
   override def build(funcName: String, expressions: Seq[Expression]): Expression = {
-    // Needed in order to make sure no parse errors appear when Collation Support is not enabled
-    // because this might suggest to the customer that feature is enabled until they reproduce
-    // the right syntax
-    if (!SQLConf.get.collationEnabled) {
-      throw QueryCompilationErrors.collationNotEnabledError()
-    }
     expressions match {
       case Seq(e: Expression, collationExpr: Expression) =>
         (collationExpr.dataType, collationExpr.foldable) match {
           case (StringType, true) =>
             val evalCollation = collationExpr.eval()
             if (evalCollation == null) {
+              // We need to throw this error first, as we do not want user to see
+              // misleading hints that collation is enabled
+              if (!SQLConf.get.collationEnabled) {
+                throw QueryCompilationErrors.collationNotEnabledError()
+              }
               throw QueryCompilationErrors.unexpectedNullError("collation", collationExpr)
             } else {
               Collate(e, evalCollation.toString)
@@ -74,9 +73,6 @@ object CollateExpressionBuilder extends ExpressionBuilder {
  */
 case class Collate(child: Expression, collationName: String)
   extends UnaryExpression with ExpectsInputTypes {
-  if (!SQLConf.get.collationEnabled) {
-    throw QueryCompilationErrors.collationNotEnabledError()
-  }
   private val collationId = CollationFactory.collationNameToId(collationName)
   override def dataType: DataType = StringType(collationId)
   override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
@@ -100,9 +96,6 @@ case class Collate(child: Expression, collationName: String)
   since = "4.0.0",
   group = "string_funcs")
 case class Collation(child: Expression) extends UnaryExpression with RuntimeReplaceable {
-  if (!SQLConf.get.collationEnabled) {
-    throw QueryCompilationErrors.collationNotEnabledError()
-  }
   override def dataType: DataType = StringType
   override protected def withNewChildInternal(newChild: Expression): Collation = copy(newChild)
   override def replacement: Expression = {

diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
@@ -3198,7 +3198,11 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
         commentSpec = Some(spec)
       }
     }
-
+    val dataType = typedVisit[DataType](ctx.dataType)
+    if (!SQLConf.get.collationEnabled &&
+      dataType.isInstanceOf[StringType] && !dataType.asInstanceOf[StringType].isDefaultCollation) {
+      throw QueryCompilationErrors.collationNotEnabledError()
+    }
     ColumnDefinition(
       name = name,
       dataType = typedVisit[DataType](ctx.dataType),

diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
@@ -300,9 +300,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
   def collationNotEnabledError(): Throwable = {
     new AnalysisException(
       errorClass = "UNSUPPORTED_FEATURE.COLLATION",
-      messageParameters = Map(
-        "collationEnabled" -> SQLConf.COLLATION_ENABLED.key)
-    )
+      messageParameters = Map.empty)
   }
 
   def unresolvedUsingColForJoinError(

diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala
@@ -22,7 +22,7 @@ import java.util.Locale
 import org.apache.spark.sql.{AnalysisException, SaveMode, SparkSession}
 import org.apache.spark.sql.catalyst.analysis._
 import org.apache.spark.sql.catalyst.catalog._
-import org.apache.spark.sql.catalyst.expressions.{Expression, InputFileBlockLength, InputFileBlockStart, InputFileName, RowOrdering}
+import org.apache.spark.sql.catalyst.expressions.{Collate, Collation, Expression, InputFileBlockLength, InputFileBlockStart, InputFileName, RowOrdering}
 import org.apache.spark.sql.catalyst.plans.logical._
 import org.apache.spark.sql.catalyst.rules.Rule
 import org.apache.spark.sql.catalyst.types.DataTypeUtils.toAttributes
@@ -32,6 +32,7 @@ import org.apache.spark.sql.errors.QueryCompilationErrors
 import org.apache.spark.sql.execution.command.DDLUtils
 import org.apache.spark.sql.execution.datasources.{CreateTable => CreateTableV1}
 import org.apache.spark.sql.execution.datasources.v2.FileDataSourceV2
+import org.apache.spark.sql.internal.SQLConf
 import org.apache.spark.sql.sources.InsertableRelation
 import org.apache.spark.sql.types.{AtomicType, StructType, VariantType}
 import org.apache.spark.sql.util.PartitioningUtils.normalizePartitionSpec
@@ -592,3 +593,18 @@ case class QualifyLocationWithWarehouse(catalog: SessionCatalog) extends Rule[Lo
       c.copy(tableDesc = newTable)
   }
 }
+
+object CollationCheck extends (LogicalPlan => Unit) {
+  def apply(plan: LogicalPlan): Unit = {
+    plan.foreach {
+      case operator: LogicalPlan =>
+        operator transformExpressionsUp {
+          case e@(_: Collation | _: Collate) =>
+            if (!SQLConf.get.collationEnabled) {
+              throw QueryCompilationErrors.collationNotEnabledError()
+            }
+            e
+        }
+    }
+  }
+}
diff --git a/sql/core/src/main/scala/org/apache/spark/sql/internal/BaseSessionStateBuilder.scala b/sql/core/src/main/scala/org/apache/spark/sql/internal/BaseSessionStateBuilder.scala
@@ -223,6 +223,7 @@ abstract class BaseSessionStateBuilder(
         HiveOnlyCheck +:
         TableCapabilityCheck +:
         CommandCheck +:
+        CollationCheck +:
         customCheckRules
   }
 

diff --git a/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryCompilationErrorsSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryCompilationErrorsSuite.scala
@@ -964,16 +964,36 @@ class QueryCompilationErrorsSuite
         "className" -> "org.apache.spark.sql.catalyst.expressions.UnsafeRow"))
   }
 
-  test("SPARK-47102: Collation in CollateContext when COLLATION_ENABLED is false") {
+  test("SPARK-47102: Collate in CollateClauseContext when COLLATION_ENABLED is false") {
     withSQLConf(SQLConf.COLLATION_ENABLED.key -> "false") {
       checkError(
         exception = intercept[AnalysisException] {
-          sql(s"select 'aaa' collate 'UNICODE_CI'")
+          sql(s"CREATE TABLE t(col STRING COLLATE 'UNICODE_CI') USING parquet")
         },
         errorClass = "UNSUPPORTED_FEATURE.COLLATION",
-        parameters = Map(
-          "collationEnabled" -> SQLConf.COLLATION_ENABLED.key)
-      )
+        parameters = Map.empty)
+    }
+  }
+
+  test("SPARK-47102: Collate in NamedExpressionContext when COLLATION_ENABLED is false") {
+    withSQLConf(SQLConf.COLLATION_ENABLED.key -> "false") {
+      checkError(
+        exception = intercept[AnalysisException] {
+          sql(s"SELECT collate('aaa', 'UNICODE_CI')")
+        },
+        errorClass = "UNSUPPORTED_FEATURE.COLLATION",
+        parameters = Map.empty)
+    }
+  }
+
+  test("SPARK-47102: Collate in CollateContext when COLLATION_ENABLED is false") {
+    withSQLConf(SQLConf.COLLATION_ENABLED.key -> "false") {
+      checkError(
+        exception = intercept[AnalysisException] {
+          sql(s"SELECT 'aaa' COLLATE 'UNICODE_CI'")
+        },
+        errorClass = "UNSUPPORTED_FEATURE.COLLATION",
+        parameters = Map.empty)
     }
   }
 
@@ -984,15 +1004,12 @@ class QueryCompilationErrorsSuite
           sql(s"select collation('aaa')")
         },
         errorClass = "UNSUPPORTED_FEATURE.COLLATION",
-        sqlState = Some("0A000"),
-        parameters = Map(
-          "collationEnabled" -> SQLConf.COLLATION_ENABLED.key),
-        context = ExpectedContext(
-          fragment = "collation('aaa')", start = 7, stop = 22)
+        parameters = Map.empty
       )
     }
   }
 
+
   test("INTERNAL_ERROR: Convert unsupported data type from Spark to Parquet") {
     val converter = new SparkToParquetSchemaConverter
     val dummyDataType = new DataType {