You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/05/13 06:17:53 UTC
[GitHub] [spark] beliefer opened a new pull request, #36531: [SPARK-39171][SQL] Unify the Cast expression
beliefer opened a new pull request, #36531:
URL: https://github.com/apache/spark/pull/36531
### What changes were proposed in this pull request?
Currently, Spark have `Cast` and `AnsiCast` expression. But their functions overlap.
Use `Cast` only is better.
### Why are the changes needed?
Unify the Cast expression
### Does this PR introduce _any_ user-facing change?
'Yes'.
`AnsiCast` will be removed.
### How was this patch tested?
N/A
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r874525801
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala:
##########
@@ -772,15 +772,19 @@ abstract class TypeCoercionBase {
case e if !e.childrenResolved => e
case DateAdd(l, r) if r.dataType == StringType && r.foldable =>
val days = try {
- AnsiCast(r, IntegerType).eval().asInstanceOf[Int]
+ val cast = Cast(r, IntegerType, ansiEnabled = true)
+ cast.setTagValue(Cast.TABLE_INSERTION_RESOLVER, true)
Review Comment:
This is not table insertion
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r880620355
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -41,6 +40,113 @@ import org.apache.spark.unsafe.types.{CalendarInterval, UTF8String}
import org.apache.spark.unsafe.types.UTF8String.{IntWrapper, LongWrapper}
object Cast {
+ /**
+ * As per section 6.13 "cast specification" in "Information technology — Database languages " +
+ * "- SQL — Part 2: Foundation (SQL/Foundation)":
+ * If the <cast operand> is a <value expression>, then the valid combinations of TD and SD
+ * in a <cast specification> are given by the following table. “Y” indicates that the
+ * combination is syntactically valid without restriction; “M” indicates that the combination
+ * is valid subject to other Syntax Rules in this Sub- clause being satisfied; and “N” indicates
+ * that the combination is not valid:
+ * SD TD
+ * EN AN C D T TS YM DT BO UDT B RT CT RW
+ * EN Y Y Y N N N M M N M N M N N
+ * AN Y Y Y N N N N N N M N M N N
+ * C Y Y Y Y Y Y Y Y Y M N M N N
+ * D N N Y Y N Y N N N M N M N N
+ * T N N Y N Y Y N N N M N M N N
+ * TS N N Y Y Y Y N N N M N M N N
+ * YM M N Y N N N Y N N M N M N N
+ * DT M N Y N N N N Y N M N M N N
+ * BO N N Y N N N N N Y M N M N N
+ * UDT M M M M M M M M M M M M M N
+ * B N N N N N N N N N M Y M N N
+ * RT M M M M M M M M M M M M N N
+ * CT N N N N N N N N N M N N M N
+ * RW N N N N N N N N N N N N N M
+ *
+ * Where:
+ * EN = Exact Numeric
+ * AN = Approximate Numeric
+ * C = Character (Fixed- or Variable-Length, or Character Large Object)
+ * D = Date
+ * T = Time
+ * TS = Timestamp
+ * YM = Year-Month Interval
+ * DT = Day-Time Interval
+ * BO = Boolean
+ * UDT = User-Defined Type
+ * B = Binary (Fixed- or Variable-Length or Binary Large Object)
+ * RT = Reference type
+ * CT = Collection type
+ * RW = Row type
+ *
+ * Spark's ANSI mode follows the syntax rules, except it specially allow the following
+ * straightforward type conversions which are disallowed as per the SQL standard:
+ * - Numeric <=> Boolean
+ * - String <=> Binary
+ */
+ def canAnsiCast(from: DataType, to: DataType): Boolean = (from, to) match {
+ case (fromType, toType) if fromType == toType => true
+
+ case (NullType, _) => true
+
+ case (_, StringType) => true
+
+ case (StringType, _: BinaryType) => true
+
+ case (StringType, BooleanType) => true
+ case (_: NumericType, BooleanType) => true
+
+ case (StringType, TimestampType) => true
+ case (DateType, TimestampType) => true
+ case (TimestampNTZType, TimestampType) => true
+ case (_: NumericType, TimestampType) => true
+
+ case (StringType, TimestampNTZType) => true
+ case (DateType, TimestampNTZType) => true
+ case (TimestampType, TimestampNTZType) => true
+
+ case (StringType, _: CalendarIntervalType) => true
+ case (StringType, _: DayTimeIntervalType) => true
+ case (StringType, _: YearMonthIntervalType) => true
+
+ case (_: DayTimeIntervalType, _: DayTimeIntervalType) => true
+ case (_: YearMonthIntervalType, _: YearMonthIntervalType) => true
+
+ case (StringType, DateType) => true
+ case (TimestampType, DateType) => true
+ case (TimestampNTZType, DateType) => true
+
+ case (_: NumericType, _: NumericType) => true
+ case (StringType, _: NumericType) => true
+ case (BooleanType, _: NumericType) => true
+ case (TimestampType, _: NumericType) => true
+
+ case (ArrayType(fromType, fn), ArrayType(toType, tn)) =>
+ canAnsiCast(fromType, toType) && resolvableNullability(fn, tn)
+
+ case (MapType(fromKey, fromValue, fn), MapType(toKey, toValue, tn)) =>
+ canAnsiCast(fromKey, toKey) && canAnsiCast(fromValue, toValue) &&
+ resolvableNullability(fn, tn)
+
+ case (StructType(fromFields), StructType(toFields)) =>
+ fromFields.length == toFields.length &&
+ fromFields.zip(toFields).forall {
+ case (fromField, toField) =>
+ canAnsiCast(fromField.dataType, toField.dataType) &&
+ resolvableNullability(fromField.nullable, toField.nullable)
+ }
+
+ case (udt1: UserDefinedType[_], udt2: UserDefinedType[_]) if udt2.acceptsType(udt1) => true
+
+ case _ => false
+ }
+
+ /**
+ * A tag to identify if a CAST added by the table insertion resolver.
+ */
+ val BY_TABLE_INSERTION = TreeNodeTag[Unit]("tableInsertionResolver")
Review Comment:
```suggestion
val BY_TABLE_INSERTION = TreeNodeTag[Unit]("by_table_insertion")
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r880618445
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TableOutputResolver.scala:
##########
@@ -235,7 +235,9 @@ object TableOutputResolver {
} else {
val casted = storeAssignmentPolicy match {
case StoreAssignmentPolicy.ANSI =>
- AnsiCast(queryExpr, tableAttr.dataType, Option(conf.sessionLocalTimeZone))
+ val cast = Cast(queryExpr, tableAttr.dataType, Option(conf.sessionLocalTimeZone), true)
Review Comment:
```suggestion
val cast = Cast(queryExpr, tableAttr.dataType, Option(conf.sessionLocalTimeZone), ansiEnabled = true)
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r873314783
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -2117,7 +2265,9 @@ case class Cast(
child: Expression,
dataType: DataType,
timeZoneId: Option[String] = None,
- override val ansiEnabled: Boolean = SQLConf.get.ansiEnabled)
+ override val ansiEnabled: Boolean = SQLConf.get.ansiEnabled,
+ fallbackConfKey: String = SQLConf.ANSI_ENABLED.key,
+ fallbackConfValue: String = "false")
Review Comment:
Can we make it an abstract class so that implementations can override? I'm really worried about changing the class constructor as many spark plugins use `Cast.apply/unapply`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r880666863
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala:
##########
@@ -112,8 +112,10 @@ object DataSourceAnalysis extends Rule[LogicalPlan] {
// the reason that the parser has erased the type info of static partition values
// and converted them to string.
case StoreAssignmentPolicy.ANSI | StoreAssignmentPolicy.STRICT =>
- Some(Alias(AnsiCast(Literal(partValue), field.dataType,
- Option(conf.sessionLocalTimeZone)), field.name)())
+ val cast =
+ Cast(Literal(partValue), field.dataType, Option(conf.sessionLocalTimeZone), true)
Review Comment:
```suggestion
Cast(Literal(partValue), field.dataType, Option(conf.sessionLocalTimeZone), ansiEnabled = true)
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r872468181
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -2117,7 +2265,9 @@ case class Cast(
child: Expression,
dataType: DataType,
timeZoneId: Option[String] = None,
- override val ansiEnabled: Boolean = SQLConf.get.ansiEnabled)
+ override val ansiEnabled: Boolean = SQLConf.get.ansiEnabled,
+ fallbackConfKey: String = SQLConf.ANSI_ENABLED.key,
+ fallbackConfValue: String = "false")
Review Comment:
why do we need these 2 parameters? Can we just check `ansiEnabled` flag? if it's true, then we use `SQLConf.ANSI_ENABLED.key`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #36531:
URL: https://github.com/apache/spark/pull/36531#issuecomment-1139354384
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #36531:
URL: https://github.com/apache/spark/pull/36531#issuecomment-1139361847
@cloud-fan Thank you for the help !
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r873508975
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -275,6 +376,55 @@ object Cast {
case _ => null
}
}
+
+ // Show suggestion on how to complete the disallowed explicit casting with built-in type
+ // conversion functions.
+ private def suggestionOnConversionFunctions (
+ from: DataType,
+ to: DataType,
+ functionNames: String): String = {
+ // scalastyle:off line.size.limit
+ s"""cannot cast ${from.catalogString} to ${to.catalogString}.
+ |To convert values from ${from.catalogString} to ${to.catalogString}, you can use $functionNames instead.
+ |""".stripMargin
+ // scalastyle:on line.size.limit
+ }
+
+ def typeCheckFailureMessage(
+ from: DataType,
+ to: DataType,
+ fallbackConfKey: Option[String],
+ fallbackConfValue: Option[String]): String =
+ (from, to) match {
+ case (_: NumericType, TimestampType) =>
+ suggestionOnConversionFunctions(from, to,
+ "functions TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS")
+
+ case (TimestampType, _: NumericType) =>
+ suggestionOnConversionFunctions(from, to, "functions UNIX_SECONDS/UNIX_MILLIS/UNIX_MICROS")
+
+ case (_: NumericType, DateType) =>
+ suggestionOnConversionFunctions(from, to, "function DATE_FROM_UNIX_DATE")
+
+ case (DateType, _: NumericType) =>
+ suggestionOnConversionFunctions(from, to, "function UNIX_DATE")
+
+ // scalastyle:off line.size.limit
+ case _ if fallbackConfKey.isDefined && fallbackConfValue.isDefined && Cast.canCast(from, to) =>
+ s"""
+ | cannot cast ${from.catalogString} to ${to.catalogString} with ANSI mode on.
+ | If you have to cast ${from.catalogString} to ${to.catalogString}, you can set ${fallbackConfKey.get} as ${fallbackConfValue.get}.
Review Comment:
Now I see the value of `AnsiCast`: it identifies the cast added by the table insertion resolver so that we can provide a different error message here.
I think it's a bit overkill to have a class `AnsiCast` for this purpose. We can have a bool `TreeNodeTag` for `Cast` to indicate if it's added by table insertion resolver.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r872962001
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -2117,7 +2265,9 @@ case class Cast(
child: Expression,
dataType: DataType,
timeZoneId: Option[String] = None,
- override val ansiEnabled: Boolean = SQLConf.get.ansiEnabled)
+ override val ansiEnabled: Boolean = SQLConf.get.ansiEnabled,
+ fallbackConfKey: String = SQLConf.ANSI_ENABLED.key,
+ fallbackConfValue: String = "false")
Review Comment:
`typeCheckFailureMessage` in `Cast` is different from `AnsiCast`. The former use `ANSI_ENABLED` but the latter use `STORE_ASSIGNMENT_POLICY`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r873277888
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -275,6 +376,53 @@ object Cast {
case _ => null
}
}
+
+ // Show suggestion on how to complete the disallowed explicit casting with built-in type
+ // conversion functions.
+ private def suggestionOnConversionFunctions (
+ from: DataType,
+ to: DataType,
+ functionNames: String): String = {
+ // scalastyle:off line.size.limit
+ s"""cannot cast ${from.catalogString} to ${to.catalogString}.
+ |To convert values from ${from.catalogString} to ${to.catalogString}, you can use $functionNames instead.
+ |""".stripMargin
+ // scalastyle:on line.size.limit
+ }
+
+ def typeCheckFailureMessage(
+ from: DataType,
+ to: DataType,
+ fallbackConfKey: Option[String],
+ fallbackConfValue: Option[String]): String =
+ (from, to) match {
+ case (_: NumericType, TimestampType) =>
+ suggestionOnConversionFunctions(from, to,
+ "functions TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS")
+
+ case (TimestampType, _: NumericType) =>
+ suggestionOnConversionFunctions(from, to, "functions UNIX_SECONDS/UNIX_MILLIS/UNIX_MICROS")
+
+ case (_: NumericType, DateType) =>
+ suggestionOnConversionFunctions(from, to, "function DATE_FROM_UNIX_DATE")
+
+ case (DateType, _: NumericType) =>
+ suggestionOnConversionFunctions(from, to, "function UNIX_DATE")
+
+ // scalastyle:off line.size.limit
+ case _ if fallbackConfKey.isDefined && fallbackConfValue.isDefined && Cast.canCast(from, to) =>
+ s"""
+ | cannot cast ${from.catalogString} to ${to.catalogString} with ANSI mode on.
+ | If you have to cast ${from.catalogString} to ${to.catalogString}, you can set ${fallbackConfKey.get} as ${fallbackConfValue.get}.
+ |""".stripMargin
+ // scalastyle:on line.size.limit
+
+ case _ => s"cannot cast ${from.catalogString} to ${to.catalogString}"
+ }
+
+ def ansiCast(child: Expression, dataType: DataType, timeZoneId: Option[String] = None): Cast =
+ Cast(child, dataType, timeZoneId, true,
+ SQLConf.STORE_ASSIGNMENT_POLICY.key, SQLConf.StoreAssignmentPolicy.LEGACY.toString)
}
abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression with NullIntolerant {
Review Comment:
Yes. `TryCast` extends this parent class 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #36531:
URL: https://github.com/apache/spark/pull/36531#issuecomment-1132584853
ping @cloud-fan
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r880662966
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/AnsiCastSuiteBase.scala:
##########
@@ -618,10 +618,14 @@ class AnsiCastSuiteWithAnsiModeOn extends AnsiCastSuiteBase {
}
Review Comment:
I think we should clean up the test suites a bit. Now we only need to test `Cast` and `TryCast`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r872466512
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -275,6 +376,53 @@ object Cast {
case _ => null
}
}
+
+ // Show suggestion on how to complete the disallowed explicit casting with built-in type
+ // conversion functions.
+ private def suggestionOnConversionFunctions (
+ from: DataType,
+ to: DataType,
+ functionNames: String): String = {
+ // scalastyle:off line.size.limit
+ s"""cannot cast ${from.catalogString} to ${to.catalogString}.
+ |To convert values from ${from.catalogString} to ${to.catalogString}, you can use $functionNames instead.
+ |""".stripMargin
+ // scalastyle:on line.size.limit
+ }
+
+ def typeCheckFailureMessage(
+ from: DataType,
+ to: DataType,
+ fallbackConfKey: Option[String],
+ fallbackConfValue: Option[String]): String =
+ (from, to) match {
+ case (_: NumericType, TimestampType) =>
+ suggestionOnConversionFunctions(from, to,
+ "functions TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS")
+
+ case (TimestampType, _: NumericType) =>
+ suggestionOnConversionFunctions(from, to, "functions UNIX_SECONDS/UNIX_MILLIS/UNIX_MICROS")
+
+ case (_: NumericType, DateType) =>
+ suggestionOnConversionFunctions(from, to, "function DATE_FROM_UNIX_DATE")
+
+ case (DateType, _: NumericType) =>
+ suggestionOnConversionFunctions(from, to, "function UNIX_DATE")
+
+ // scalastyle:off line.size.limit
+ case _ if fallbackConfKey.isDefined && fallbackConfValue.isDefined && Cast.canCast(from, to) =>
+ s"""
+ | cannot cast ${from.catalogString} to ${to.catalogString} with ANSI mode on.
+ | If you have to cast ${from.catalogString} to ${to.catalogString}, you can set ${fallbackConfKey.get} as ${fallbackConfValue.get}.
+ |""".stripMargin
+ // scalastyle:on line.size.limit
+
+ case _ => s"cannot cast ${from.catalogString} to ${to.catalogString}"
+ }
+
+ def ansiCast(child: Expression, dataType: DataType, timeZoneId: Option[String] = None): Cast =
+ Cast(child, dataType, timeZoneId, true,
+ SQLConf.STORE_ASSIGNMENT_POLICY.key, SQLConf.StoreAssignmentPolicy.LEGACY.toString)
}
abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression with NullIntolerant {
Review Comment:
do we still need this parent class?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r874528129
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -2138,199 +2287,28 @@ case class Cast(
final override def nodePatternsInternal(): Seq[TreePattern] = Seq(CAST)
override def canCast(from: DataType, to: DataType): Boolean = if (ansiEnabled) {
- AnsiCast.canCast(from, to)
+ Cast.canAnsiCast(from, to)
} else {
Cast.canCast(from, to)
}
+ private val (fallbackConfKey, fallbackConfValue) =
+ if (getTagValue(Cast.TABLE_INSERTION_RESOLVER).getOrElse(false)) {
Review Comment:
can we pass a boolean flag (existence of this tag) to `typeCheckFailureMessage` and determine the config names there?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r880623685
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -275,6 +381,49 @@ object Cast {
case _ => null
}
}
+
+ // Show suggestion on how to complete the disallowed explicit casting with built-in type
+ // conversion functions.
+ private def suggestionOnConversionFunctions (
+ from: DataType,
+ to: DataType,
+ functionNames: String): String = {
+ // scalastyle:off line.size.limit
+ s"""cannot cast ${from.catalogString} to ${to.catalogString}.
+ |To convert values from ${from.catalogString} to ${to.catalogString}, you can use $functionNames instead.
+ |""".stripMargin
+ // scalastyle:on line.size.limit
+ }
+
+ def typeCheckFailureMessage(
+ from: DataType,
+ to: DataType,
+ fallbackConfKey: Option[String],
Review Comment:
how about putting key and value together: `fallbackConf: Option[(String, String)]`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
URL: https://github.com/apache/spark/pull/36531
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r880661301
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/V2WriteAnalysisSuite.scala:
##########
@@ -122,7 +122,10 @@ abstract class V2ANSIWriteAnalysisSuiteBase extends V2WriteAnalysisSuiteBase {
expectedPlan: LogicalPlan,
caseSensitive: Boolean = true): Unit = {
val expectedPlanWithAnsiCast = expectedPlan transformAllExpressions {
- case c: Cast => AnsiCast(c.child, c.dataType, c.timeZoneId)
+ case c: Cast =>
+ val cast = Cast(c.child, c.dataType, c.timeZoneId, true)
Review Comment:
```suggestion
val cast = Cast(c.child, c.dataType, c.timeZoneId, ansiEnabled = true)
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r880662966
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/AnsiCastSuiteBase.scala:
##########
@@ -618,10 +618,14 @@ class AnsiCastSuiteWithAnsiModeOn extends AnsiCastSuiteBase {
}
Review Comment:
I think we should clean up the test suites a bit. Now we only need to test `Cast` and `TryCast`. `AnsiCastSuiteWithAnsiModeOn` doesn't make sense any more.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r877893550
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -275,6 +376,55 @@ object Cast {
case _ => null
}
}
+
+ // Show suggestion on how to complete the disallowed explicit casting with built-in type
+ // conversion functions.
+ private def suggestionOnConversionFunctions (
+ from: DataType,
+ to: DataType,
+ functionNames: String): String = {
+ // scalastyle:off line.size.limit
+ s"""cannot cast ${from.catalogString} to ${to.catalogString}.
+ |To convert values from ${from.catalogString} to ${to.catalogString}, you can use $functionNames instead.
+ |""".stripMargin
+ // scalastyle:on line.size.limit
+ }
+
+ def typeCheckFailureMessage(
+ from: DataType,
+ to: DataType,
+ fallbackConfKey: Option[String],
+ fallbackConfValue: Option[String]): String =
+ (from, to) match {
+ case (_: NumericType, TimestampType) =>
+ suggestionOnConversionFunctions(from, to,
+ "functions TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS")
+
+ case (TimestampType, _: NumericType) =>
+ suggestionOnConversionFunctions(from, to, "functions UNIX_SECONDS/UNIX_MILLIS/UNIX_MICROS")
+
+ case (_: NumericType, DateType) =>
+ suggestionOnConversionFunctions(from, to, "function DATE_FROM_UNIX_DATE")
+
+ case (DateType, _: NumericType) =>
+ suggestionOnConversionFunctions(from, to, "function UNIX_DATE")
+
+ // scalastyle:off line.size.limit
+ case _ if fallbackConfKey.isDefined && fallbackConfValue.isDefined && Cast.canCast(from, to) =>
+ s"""
+ | cannot cast ${from.catalogString} to ${to.catalogString} with ANSI mode on.
+ | If you have to cast ${from.catalogString} to ${to.catalogString}, you can set ${fallbackConfKey.get} as ${fallbackConfValue.get}.
Review Comment:
OK
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r880669148
##########
sql/core/src/test/scala/org/apache/spark/sql/sources/DataSourceAnalysisSuite.scala:
##########
@@ -63,7 +63,9 @@ class DataSourceAnalysisSuite extends SparkFunSuite with BeforeAndAfterAll with
def cast(e: Expression, dt: DataType): Expression = {
SQLConf.get.storeAssignmentPolicy match {
case StoreAssignmentPolicy.ANSI | StoreAssignmentPolicy.STRICT =>
- AnsiCast(e, dt, Option(SQLConf.get.sessionLocalTimeZone))
+ val cast = Cast(e, dt, Option(SQLConf.get.sessionLocalTimeZone), true)
Review Comment:
```suggestion
val cast = Cast(e, dt, Option(SQLConf.get.sessionLocalTimeZone), ansiEnabled = true)
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36531:
URL: https://github.com/apache/spark/pull/36531#discussion_r874526787
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -41,6 +40,113 @@ import org.apache.spark.unsafe.types.{CalendarInterval, UTF8String}
import org.apache.spark.unsafe.types.UTF8String.{IntWrapper, LongWrapper}
object Cast {
+ /**
+ * As per section 6.13 "cast specification" in "Information technology — Database languages " +
+ * "- SQL — Part 2: Foundation (SQL/Foundation)":
+ * If the <cast operand> is a <value expression>, then the valid combinations of TD and SD
+ * in a <cast specification> are given by the following table. “Y” indicates that the
+ * combination is syntactically valid without restriction; “M” indicates that the combination
+ * is valid subject to other Syntax Rules in this Sub- clause being satisfied; and “N” indicates
+ * that the combination is not valid:
+ * SD TD
+ * EN AN C D T TS YM DT BO UDT B RT CT RW
+ * EN Y Y Y N N N M M N M N M N N
+ * AN Y Y Y N N N N N N M N M N N
+ * C Y Y Y Y Y Y Y Y Y M N M N N
+ * D N N Y Y N Y N N N M N M N N
+ * T N N Y N Y Y N N N M N M N N
+ * TS N N Y Y Y Y N N N M N M N N
+ * YM M N Y N N N Y N N M N M N N
+ * DT M N Y N N N N Y N M N M N N
+ * BO N N Y N N N N N Y M N M N N
+ * UDT M M M M M M M M M M M M M N
+ * B N N N N N N N N N M Y M N N
+ * RT M M M M M M M M M M M M N N
+ * CT N N N N N N N N N M N N M N
+ * RW N N N N N N N N N N N N N M
+ *
+ * Where:
+ * EN = Exact Numeric
+ * AN = Approximate Numeric
+ * C = Character (Fixed- or Variable-Length, or Character Large Object)
+ * D = Date
+ * T = Time
+ * TS = Timestamp
+ * YM = Year-Month Interval
+ * DT = Day-Time Interval
+ * BO = Boolean
+ * UDT = User-Defined Type
+ * B = Binary (Fixed- or Variable-Length or Binary Large Object)
+ * RT = Reference type
+ * CT = Collection type
+ * RW = Row type
+ *
+ * Spark's ANSI mode follows the syntax rules, except it specially allow the following
+ * straightforward type conversions which are disallowed as per the SQL standard:
+ * - Numeric <=> Boolean
+ * - String <=> Binary
+ */
+ def canAnsiCast(from: DataType, to: DataType): Boolean = (from, to) match {
+ case (fromType, toType) if fromType == toType => true
+
+ case (NullType, _) => true
+
+ case (_, StringType) => true
+
+ case (StringType, _: BinaryType) => true
+
+ case (StringType, BooleanType) => true
+ case (_: NumericType, BooleanType) => true
+
+ case (StringType, TimestampType) => true
+ case (DateType, TimestampType) => true
+ case (TimestampNTZType, TimestampType) => true
+ case (_: NumericType, TimestampType) => true
+
+ case (StringType, TimestampNTZType) => true
+ case (DateType, TimestampNTZType) => true
+ case (TimestampType, TimestampNTZType) => true
+
+ case (StringType, _: CalendarIntervalType) => true
+ case (StringType, _: DayTimeIntervalType) => true
+ case (StringType, _: YearMonthIntervalType) => true
+
+ case (_: DayTimeIntervalType, _: DayTimeIntervalType) => true
+ case (_: YearMonthIntervalType, _: YearMonthIntervalType) => true
+
+ case (StringType, DateType) => true
+ case (TimestampType, DateType) => true
+ case (TimestampNTZType, DateType) => true
+
+ case (_: NumericType, _: NumericType) => true
+ case (StringType, _: NumericType) => true
+ case (BooleanType, _: NumericType) => true
+ case (TimestampType, _: NumericType) => true
+
+ case (ArrayType(fromType, fn), ArrayType(toType, tn)) =>
+ canAnsiCast(fromType, toType) && resolvableNullability(fn, tn)
+
+ case (MapType(fromKey, fromValue, fn), MapType(toKey, toValue, tn)) =>
+ canAnsiCast(fromKey, toKey) && canAnsiCast(fromValue, toValue) &&
+ resolvableNullability(fn, tn)
+
+ case (StructType(fromFields), StructType(toFields)) =>
+ fromFields.length == toFields.length &&
+ fromFields.zip(toFields).forall {
+ case (fromField, toField) =>
+ canAnsiCast(fromField.dataType, toField.dataType) &&
+ resolvableNullability(fromField.nullable, toField.nullable)
+ }
+
+ case (udt1: UserDefinedType[_], udt2: UserDefinedType[_]) if udt2.acceptsType(udt1) => true
+
+ case _ => false
+ }
+
+ /**
+ * A tag to identify if a CAST added by the table insertion resolver.
+ */
+ val TABLE_INSERTION_RESOLVER = TreeNodeTag[Boolean]("tableInsertionResolver")
Review Comment:
how about `BY_TABLE_INSERTION`? And we can make it a `TreeNodeTag[Unit]` as we only need to check its existence.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #36531: [SPARK-39171][SQL] Unify the Cast expression
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #36531:
URL: https://github.com/apache/spark/pull/36531#issuecomment-1125949545
ping @huaxingao cc @cloud-fan
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org