mkaravel commented on code in PR #45285:
URL: https://github.com/apache/spark/pull/45285#discussion_r1504599200
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3854,6 +3854,11 @@
"Catalog <catalogName> does not support <operation>."
]
},
+ "COLLATION" : {
+ "message" : [
+ "Collation support is under development and not all features are
supported. To enable existing features set <collationEnabled> to `true`."
Review Comment:
+1 on what @cloud-fan suggest. Let's make it simple and just mention that it
is unsupported.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collationExpressions.scala:
##########
@@ -40,6 +41,12 @@ 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()
+ }
Review Comment:
When is this executed? How often?
How come we do not block this at the analyzer level?
##########
sql/core/src/test/scala/org/apache/spark/sql/errors/QueryCompilationErrorsSuite.scala:
##########
@@ -964,6 +964,35 @@ class QueryCompilationErrorsSuite
"className" -> "org.apache.spark.sql.catalyst.expressions.UnsafeRow"))
}
+ test("SPARK-47102: Collation in CollateContext when COLLATION_ENABLED is
false") {
+ withSQLConf(SQLConf.COLLATION_ENABLED.key -> "false") {
+ checkError(
+ exception = intercept[AnalysisException] {
+ sql(s"select 'aaa' collate 'UNICODE_CI'")
Review Comment:
+1 on adding such a test.
##########
sql/core/src/test/scala/org/apache/spark/sql/CollationSuite.scala:
##########
@@ -32,6 +33,11 @@ import org.apache.spark.sql.types.StringType
class CollationSuite extends DatasourceV2SQLBase {
protected val v2Source = classOf[FakeV2ProviderWithCustomSchema].getName
+ protected override def beforeAll(): Unit = {
+ System.setProperty(IS_TESTING.key, "true")
Review Comment:
We have already set the SQL config to `Utils.isTesting`. This should not be
necessary at all.
--
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]
