sarutak commented on code in PR #60083:
URL: https://github.com/apache/spark/pull/60083#discussion_r4470585744
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -594,6 +594,20 @@ object Cast extends QueryErrorsBase {
case _ => false
}
+ /**
+ * Returns true if a LEGACY cast from `from` type to `to` type never fails
at runtime. Among
+ * numeric, boolean and string types, it wraps around on overflow or returns
null for an invalid
+ * input instead. Other LEGACY casts can fail, for example from or to an
interval and to
+ * CHAR/VARCHAR. The patterns matched reflect the current implementation in
the Cast node and
+ * must stay in sync with it.
Review Comment:
Nit: not every accepted pair wraps around on overflow. A float or double to
INT or BIGINT saturates, and a string to an integral type or a cast to a
decimal returns null on overflow. Also, not every LEGACY cast this rejects can
fail, for example a date to a string or an interval to a string. How about
something like "Among numeric, boolean and string types, except to
CHAR/VARCHAR, it wraps around or saturates on overflow, or returns null,
instead of failing. Returns false for other LEGACY casts, some of which can
fail, for example from an interval to a number, from a string to an interval,
and to CHAR/VARCHAR.", keeping the last sentence about staying in sync? The
same "wrap around on overflow or return null for an invalid input" sentence is
in the description, which becomes the commit message.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/ConstantFoldingSuite.scala:
##########
@@ -342,6 +342,23 @@ class ConstantFoldingSuite extends PlanTest {
}
}
+ test("SPARK-60868: Do not fold the size of CreateArray over a LEGACY cast
that can fail") {
+ val t = LocalRelation($"i".int, $"d".dayTimeInterval())
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> "false") {
+ Seq(
+ // This cannot fail, as in SPARK-37392.
+ Cast($"i", StringType) -> true,
+ // These fail with CAST_OVERFLOW and EXCEED_LIMIT_LENGTH.
+ Cast($"d", ByteType) -> false,
+ Cast($"i", VarcharType(2)) -> false).foreach { case (cast, foldable) =>
Review Comment:
Two mutants still pass all of `ConstantFoldingSuite`: (1) sending TRY casts
through `Cast.isNoThrowLegacyCast` as well, which stops folding, for example, a
TRY cast from an interval to TINYINT; (2) returning `true` for an accepted
LEGACY cast without checking its child. Could you add rows such as `Cast($"d",
ByteType, evalMode = EvalMode.TRY) -> true` and `Cast(Cast($"d", ByteType),
StringType) -> false`, relaxing the assert to `cast.evalMode != EvalMode.ANSI`?
Adding a string column (for example `$"str".string`) to the relation and a row
`Cast($"str", DayTimeIntervalType()) -> false` would also cover the
description's second example.
##########
docs/sql-migration-guide.md:
##########
@@ -31,6 +31,7 @@ license: |
- Since Spark 4.4, when a file source reads a partition column with a
user-specified `DECIMAL` type from directory names, it converts each value to
the precision and scale of that type, rounding half up as `CAST` does.
Previously, the vectorized Parquet and ORC readers applied the declared scale
to the digits in the directory name without an error, for example `d=1.5` read
as `DECIMAL(10,2)` returned `0.15`. Data derived from affected query results
may need to be recomputed. A value that does not fit the declared precision now
fails with `INVALID_PARTITION_VALUE` regardless of `spark.sql.ansi.enabled`.
Previously, those readers returned a wrong value and the other readers returned
`NULL`. To get `NULL` for such a value, set
`spark.sql.sources.validatePartitionColumns` to `false`. Partitions tracked in
the catalog are not affected.
- Since Spark 4.4, when a file source reads a partition column with a
user-specified `TINYINT` or `SMALLINT` type from directory names, a value out
of the range of that type fails with `INVALID_PARTITION_VALUE` regardless of
`spark.sql.ansi.enabled`, as it does for `INT`. Previously, the value wrapped
around without an error, for example `b=300` read as `TINYINT` returned `44`.
Data derived from affected query results may need to be recomputed. To get
`NULL` for such a value, set `spark.sql.sources.validatePartitionColumns` to
`false`. Reading partitions tracked in the catalog is not affected, but writing
to a table whose catalog tracks such a partition, for example one added by
`MSCK REPAIR TABLE`, can now fail with `NumberFormatException` unless
`spark.sql.sources.validatePartitionColumns` is `false`.
- Since Spark 4.4, `size` and `cardinality` of a map built by `map(...)`
return the number of entries of the built map. Previously, the optimizer could
return the number of key-value pairs instead, for example `size(map(1, 'a', 1,
'b'))` returned `2` instead of `1` when `spark.sql.mapKeyDedupPolicy` is
`LAST_WIN`. Data derived from affected query results may need to be recomputed.
Under the default `EXCEPTION` policy, such a query now fails with
`DUPLICATED_MAP_KEY`, and a NULL key fails with `NULL_MAP_KEY`, as building the
map does. This also applies to a map inside `array(...)`, for example
`size(array(map(k, 1)))`.
+- Since Spark 4.4, when `spark.sql.ansi.enabled` is `false`, `size` and
`cardinality` of an array built by `array(...)` evaluate the LEGACY casts in it
that can fail, such as a cast from an interval to `TINYINT`, from a string to
an interval, or to `VARCHAR(n)`. Previously, the optimizer could return the
number of elements without evaluating such a cast. For example, for an
`INTERVAL SECOND` column `i` with the value `INTERVAL '1000' SECOND`,
`size(array(CAST(i AS TINYINT)))` returned `1` instead of failing with
`CAST_OVERFLOW`, and `try_add(DATE'2020-01-01', size(array(CAST(i AS
TINYINT))))` returned `2020-01-02` instead of `NULL`. Data derived from
affected query results may need to be recomputed.
Review Comment:
"or to `VARCHAR(n)`" holds only when CHAR/VARCHAR are kept as types in a
cast. By default, `CharVarcharUtils.replaceCharVarcharWithStringForCast`
replaces `VARCHAR(n)` with `STRING`, so `size(array(CAST(12345 AS
VARCHAR(2))))` still returns `1` with this PR. I got `EXCEED_LIMIT_LENGTH` with
`spark.sql.preserveCharVarcharTypeInfo=true`, and also with
`spark.sql.charVarchar.standardSemantics.enabled=true`. How about "or, when
`spark.sql.preserveCharVarcharTypeInfo` or
`spark.sql.charVarchar.standardSemantics.enabled` is `true`, to `VARCHAR(n)`"?
The third example in the description could mention
`spark.sql.charVarchar.standardSemantics.enabled` too.
--
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]
