dongjoon-hyun commented on code in PR #59926: URL: https://github.com/apache/spark/pull/59926#discussion_r4417439344
########## sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/IndeterminateCollationCheckSuite.scala: ########## @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.catalyst.analysis + +import org.apache.spark.SparkFunSuite +import org.apache.spark.sql.AnalysisException +import org.apache.spark.sql.catalyst.expressions.{AttributeReference, DropField, Length, UpdateFields, Upper} +import org.apache.spark.sql.types.{DataType, IndeterminateStringType, IntegerType, StringType, StructType} + +/** The indeterminate-collation check of [[CollationTypeCoercion]]. */ +class IndeterminateCollationCheckSuite extends SparkFunSuite { + + private val struct = AttributeReference( + "s", new StructType().add("a", IntegerType).add("b", StringType))() + + private val indeterminate = AttributeReference("x", IndeterminateStringType)() + + test("does not ask the field operations of UpdateFields for a data type") { + val drop = new CountingDropField("b") + val update = UpdateFields(struct, Seq(drop)) + assert(CollationTypeCoercion(update) eq update) + assert(drop.dataTypeRequests == 0) Review Comment: **1. [Low] The regression test only checks a `DropField` under a direct `apply` call** This test calls `CollationTypeCoercion` once, on the `UpdateFields` itself, with a single `DropField`. `CollationTypeCasts` and the single-pass resolver call it on every node, and the reorder also stops the rule from typing the children of an `UpdateNestedField` when it visits that node. The test does not reach that case: with the old code, an operation inside an `UpdateNestedField` is asked for its data type only when the rule visits the `UpdateNestedField`, which a single `apply` on the `UpdateFields` never does. Could the test visit every node, as the rule does, and include the other operations? For example (not run; it also needs `Literal`, `UpdateNestedField` and `WithField` in the imports): ```scala val drop = new CountingDropField("b") val nestedDrop = new CountingDropField("y") val update = UpdateFields(struct, Seq(WithField("c", Literal(1)), drop, UpdateNestedField("n", Seq(nestedDrop)))) // Visit every node, as CollationTypeCasts does. assert(update.transformUp { case e => CollationTypeCoercion(e) } eq update) assert(drop.dataTypeRequests == 0) assert(nestedDrop.dataTypeRequests == 0) ``` For the plan to make sense, `struct` would also need a struct field `n` with a field `y`, although nothing here checks it. ########## sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/IndeterminateCollationCheckSuite.scala: ########## @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.catalyst.analysis + +import org.apache.spark.SparkFunSuite +import org.apache.spark.sql.AnalysisException +import org.apache.spark.sql.catalyst.expressions.{AttributeReference, DropField, Length, UpdateFields, Upper} +import org.apache.spark.sql.types.{DataType, IndeterminateStringType, IntegerType, StringType, StructType} + +/** The indeterminate-collation check of [[CollationTypeCoercion]]. */ +class IndeterminateCollationCheckSuite extends SparkFunSuite { + + private val struct = AttributeReference( + "s", new StructType().add("a", IntegerType).add("b", StringType))() + + private val indeterminate = AttributeReference("x", IndeterminateStringType)() + + test("does not ask the field operations of UpdateFields for a data type") { + val drop = new CountingDropField("b") + val update = UpdateFields(struct, Seq(drop)) + assert(CollationTypeCoercion(update) eq update) + assert(drop.dataTypeRequests == 0) + } + + test("fails an expression that does not accept an indeterminate collation input") { + checkError( + exception = intercept[AnalysisException] { + CollationTypeCoercion(Upper(indeterminate)) + }, + condition = "INDETERMINATE_COLLATION_IN_EXPRESSION", + parameters = Map("expr" -> "\"upper(x)\"")) + } + + test("keeps an expression that accepts an indeterminate collation input") { + val length = Length(indeterminate) + assert(CollationTypeCoercion(length) eq length) + } + + /** A [[DropField]] that counts the requests for its data type, which it does not have. */ + private class CountingDropField(name: String) extends DropField(name) { Review Comment: **4. [Nit] `CountingDropField` subclasses the case class `DropField`** It works, but it inherits the `copy`, `equals` and `hashCode` of `DropField`: `copy()` returns a plain `DropField`, and `new CountingDropField("b") == DropField("b")` is true. Nothing here copies or compares it today, but the test would also stop compiling if `DropField` were ever made `final`. A small case class in the test that extends `StructFieldsOperation with LeafLike[Expression]` and keeps the same counter would avoid both, and it would not look like a real `DropField` in a failure message. ########## sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/IndeterminateCollationCheckSuite.scala: ########## @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.catalyst.analysis + +import org.apache.spark.SparkFunSuite +import org.apache.spark.sql.AnalysisException +import org.apache.spark.sql.catalyst.expressions.{AttributeReference, DropField, Length, UpdateFields, Upper} +import org.apache.spark.sql.types.{DataType, IndeterminateStringType, IntegerType, StringType, StructType} + +/** The indeterminate-collation check of [[CollationTypeCoercion]]. */ +class IndeterminateCollationCheckSuite extends SparkFunSuite { Review Comment: **5. [Nit] The suite name is one word away from `IndeterminateCollationTestSuite`** `sql/core` already has `org.apache.spark.sql.collation.IndeterminateCollationTestSuite`, so the two suites are easy to mix up in `testOnly` patterns and searches. This suite tests `CollationTypeCoercion` directly, and catalyst has no suite for it yet, so `CollationTypeCoercionSuite` might be easier to find and would also fit later unit tests of the object. ########## sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/IndeterminateCollationCheckSuite.scala: ########## @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.catalyst.analysis + +import org.apache.spark.SparkFunSuite +import org.apache.spark.sql.AnalysisException +import org.apache.spark.sql.catalyst.expressions.{AttributeReference, DropField, Length, UpdateFields, Upper} +import org.apache.spark.sql.types.{DataType, IndeterminateStringType, IntegerType, StringType, StructType} + +/** The indeterminate-collation check of [[CollationTypeCoercion]]. */ +class IndeterminateCollationCheckSuite extends SparkFunSuite { + + private val struct = AttributeReference( + "s", new StructType().add("a", IntegerType).add("b", StringType))() + + private val indeterminate = AttributeReference("x", IndeterminateStringType)() + + test("does not ask the field operations of UpdateFields for a data type") { + val drop = new CountingDropField("b") + val update = UpdateFields(struct, Seq(drop)) + assert(CollationTypeCoercion(update) eq update) + assert(drop.dataTypeRequests == 0) + } + + test("fails an expression that does not accept an indeterminate collation input") { + checkError( + exception = intercept[AnalysisException] { + CollationTypeCoercion(Upper(indeterminate)) + }, + condition = "INDETERMINATE_COLLATION_IN_EXPRESSION", + parameters = Map("expr" -> "\"upper(x)\"")) Review Comment: **3. [Nit] No test has a rejecting expression whose indeterminate input is not its first child** The change moves `children.exists` behind the check of the expression, but `Upper` has one child, so a predicate that looked only at the first child would pass these tests too. A comparison with the indeterminate input second would cover that, for example (not run; it needs `EqualTo` and `Literal` in the imports): ```scala test("fails when only a later child has an indeterminate collation") { val e = intercept[AnalysisException] { CollationTypeCoercion(EqualTo(Literal("a"), indeterminate)) } assert(e.getCondition == "INDETERMINATE_COLLATION_IN_EXPRESSION") } ``` ########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala: ########## @@ -507,8 +507,11 @@ object CollationTypeCoercion extends SQLConfHelper { case _: Throwable => NullType } - expression.children.exists(child => - shouldFailWithIndeterminateCollation(expression, getDataTypeSafe(child))) + // Most expressions can contain indeterminate collation, so their children's types are not + // needed. Some of those types are an internal error to build and swallow, such as the type of + // a WithField or DropField operation of an UpdateFields. Review Comment: **2. [Nit] The new comment leaves out `UpdateNestedField`** `UpdateNestedField` is the third `StructFieldsOperation` that can be a child of `UpdateFields`, and its `dataType` throws in the same way. The phrase "an internal error to build and swallow" was also hard for me to parse. Maybe: ```suggestion // Most expressions can contain indeterminate collation, so check the expression before // typing its children. Some children have no data type and throw an internal error when // asked for one, such as the WithField, DropField and UpdateNestedField operations of an // UpdateFields. ``` -- 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]
