cloud-fan commented on a change in pull request #25581: [SPARK-28495][SQL] Introduce ANSI store assignment policy for table insertion URL: https://github.com/apache/spark/pull/25581#discussion_r317942895
########## File path: sql/catalyst/src/test/scala/org/apache/spark/sql/types/DataTypeWriteCompatibilitySuite.scala ########## @@ -297,87 +508,10 @@ class DataTypeWriteCompatibilitySuite extends SparkFunSuite { "Should allow map of int written to map of long column") } - test("Check types with multiple errors") { - val readType = StructType(Seq( - StructField("a", ArrayType(DoubleType, containsNull = false)), - StructField("arr_of_structs", ArrayType(point2, containsNull = false)), - StructField("bad_nested_type", ArrayType(StringType)), - StructField("m", MapType(LongType, FloatType, valueContainsNull = false)), - StructField("map_of_structs", MapType(StringType, point3, valueContainsNull = false)), - StructField("x", IntegerType, nullable = false), - StructField("missing1", StringType, nullable = false), - StructField("missing2", StringType) - )) - - val missingMiddleField = StructType(Seq( - StructField("x", FloatType, nullable = false), - StructField("z", FloatType, nullable = false))) - - val writeType = StructType(Seq( - StructField("a", ArrayType(StringType)), - StructField("arr_of_structs", ArrayType(point3)), - StructField("bad_nested_type", point3), - StructField("m", MapType(DoubleType, DoubleType)), - StructField("map_of_structs", MapType(StringType, missingMiddleField)), - StructField("y", LongType) - )) - - assertNumErrors(writeType, readType, "top", "Should catch 14 errors", 14) { errs => - assert(errs(0).contains("'top.a.element'"), "Should identify bad type") - assert(errs(0).contains("Cannot safely cast")) - assert(errs(0).contains("StringType to DoubleType")) - - assert(errs(1).contains("'top.a'"), "Should identify bad type") - assert(errs(1).contains("Cannot write nullable elements to array of non-nulls")) - - assert(errs(2).contains("'top.arr_of_structs.element'"), "Should identify bad type") - assert(errs(2).contains("'z'"), "Should identify bad field") - assert(errs(2).contains("Cannot write extra fields to struct")) - - assert(errs(3).contains("'top.arr_of_structs'"), "Should identify bad type") - assert(errs(3).contains("Cannot write nullable elements to array of non-nulls")) - - assert(errs(4).contains("'top.bad_nested_type'"), "Should identify bad type") - assert(errs(4).contains("is incompatible with")) - - assert(errs(5).contains("'top.m.key'"), "Should identify bad type") - assert(errs(5).contains("Cannot safely cast")) - assert(errs(5).contains("DoubleType to LongType")) - - assert(errs(6).contains("'top.m.value'"), "Should identify bad type") - assert(errs(6).contains("Cannot safely cast")) - assert(errs(6).contains("DoubleType to FloatType")) - - assert(errs(7).contains("'top.m'"), "Should identify bad type") - assert(errs(7).contains("Cannot write nullable values to map of non-nulls")) - - assert(errs(8).contains("'top.map_of_structs.value'"), "Should identify bad type") - assert(errs(8).contains("expected 'y', found 'z'"), "Should detect name mismatch") - assert(errs(8).contains("field name does not match"), "Should identify name problem") - - assert(errs(9).contains("'top.map_of_structs.value'"), "Should identify bad type") - assert(errs(9).contains("'z'"), "Should identify missing field") - assert(errs(9).contains("missing fields"), "Should detect missing field") - - assert(errs(10).contains("'top.map_of_structs'"), "Should identify bad type") - assert(errs(10).contains("Cannot write nullable values to map of non-nulls")) - - assert(errs(11).contains("'top.x'"), "Should identify bad type") - assert(errs(11).contains("Cannot safely cast")) - assert(errs(11).contains("LongType to IntegerType")) - - assert(errs(12).contains("'top'"), "Should identify bad type") - assert(errs(12).contains("expected 'x', found 'y'"), "Should detect name mismatch") - assert(errs(12).contains("field name does not match"), "Should identify name problem") - - assert(errs(13).contains("'top'"), "Should identify bad type") - assert(errs(13).contains("'missing1'"), "Should identify missing field") - assert(errs(13).contains("missing fields"), "Should detect missing field") - } - } - // Helper functions + protected def storeAssignmentPolicy: StoreAssignmentPolicy.Value + def assertAllowed( Review comment: it looks to me that, we can make the code simpler if this method takes the policy parameter. e.g. ``` test("Check atomic types: write allowed only when casting is safe") { atomicTypes.foreach { w => atomicTypes.foreach { r => if (Cast.canUpCast(w, r)) { assertAllowed(Policy.STRICT, ...) } else { assertSingleError(Policy.STRICT, ...) } if (Cast.canANSIStoreAssign(w, r)) ... } } } ``` ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org