You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "vladimirg-db (via GitHub)" <gi...@apache.org> on 2025/03/04 16:36:50 UTC

[PR] [SPARK-51388][SQL] Improve SQL fragment propagation in to_timestamp and UNION [spark]

vladimirg-db opened a new pull request, #50154:
URL: https://github.com/apache/spark/pull/50154

   ### What changes were proposed in this pull request?
   
   Improve SQL fragment propagation in to_timestamp and UNION.
   
   ### Why are the changes needed?
   
   To improve error messages:
   ![image](https://github.com/user-attachments/assets/4952ce49-5a29-4bf7-9d9f-0e443b1127ec)
   
   
   ### Does this PR introduce _any_ user-facing change?
   
   Invalid `Cast` error messages become better.
   
   ### How was this patch tested?
   
   Golden file tests are regenerated.
   
   ### Was this patch authored or co-authored using generative AI tooling?
   
   No.


-- 
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

Re: [PR] [SPARK-51388][SQL] Improve SQL fragment propagation in to_timestamp and UNION [spark]

Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50154:
URL: https://github.com/apache/spark/pull/50154#discussion_r1980802016


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -2104,11 +2105,13 @@ case class ParseToDate(
     ansiEnabled: Boolean = SQLConf.get.ansiEnabled)
   extends RuntimeReplaceable with ImplicitCastInputTypes with TimeZoneAwareExpression {
 
-  override lazy val replacement: Expression = format.map { f =>
-    Cast(GetTimestamp(left, f, TimestampType, "try_to_date", timeZoneId, ansiEnabled), DateType,
-      timeZoneId, EvalMode.fromBoolean(ansiEnabled))
-  }.getOrElse(Cast(left, DateType, timeZoneId,
-    EvalMode.fromBoolean(ansiEnabled))) // backwards compatibility
+  override lazy val replacement: Expression = withOrigin(origin) {

Review Comment:
   Actually, I don't think this will work trivially... `withOrigin` sets a thread-local `CurrentOrigin.value` to something provided from above, so that `TreeNode`s constructed below automatically get this origin. But `replacement` is usually a `lazy val`, so we would have to change all the node definitions, because we need `withOrigin` at the moment of construction.



-- 
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

Re: [PR] [SPARK-51388][SQL] Improve SQL fragment propagation in to_timestamp and UNION [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50154:
URL: https://github.com/apache/spark/pull/50154#issuecomment-2700324637

   thanks, merging to master/4.0!


-- 
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

Re: [PR] [SPARK-51388][SQL] Improve SQL fragment propagation in to_timestamp and UNION [spark]

Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50154:
URL: https://github.com/apache/spark/pull/50154#discussion_r1980785955


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -2104,11 +2105,13 @@ case class ParseToDate(
     ansiEnabled: Boolean = SQLConf.get.ansiEnabled)
   extends RuntimeReplaceable with ImplicitCastInputTypes with TimeZoneAwareExpression {
 
-  override lazy val replacement: Expression = format.map { f =>
-    Cast(GetTimestamp(left, f, TimestampType, "try_to_date", timeZoneId, ansiEnabled), DateType,
-      timeZoneId, EvalMode.fromBoolean(ansiEnabled))
-  }.getOrElse(Cast(left, DateType, timeZoneId,
-    EvalMode.fromBoolean(ansiEnabled))) // backwards compatibility
+  override lazy val replacement: Expression = withOrigin(origin) {

Review Comment:
   Are you proposing to replace all the `replacement` usages with a new method name?



-- 
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

Re: [PR] [SPARK-51388][SQL] Improve SQL fragment propagation in to_timestamp and UNION [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50154:
URL: https://github.com/apache/spark/pull/50154#discussion_r1981001564


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -2104,11 +2105,13 @@ case class ParseToDate(
     ansiEnabled: Boolean = SQLConf.get.ansiEnabled)
   extends RuntimeReplaceable with ImplicitCastInputTypes with TimeZoneAwareExpression {
 
-  override lazy val replacement: Expression = format.map { f =>
-    Cast(GetTimestamp(left, f, TimestampType, "try_to_date", timeZoneId, ansiEnabled), DateType,
-      timeZoneId, EvalMode.fromBoolean(ansiEnabled))
-  }.getOrElse(Cast(left, DateType, timeZoneId,
-    EvalMode.fromBoolean(ansiEnabled))) // backwards compatibility
+  override lazy val replacement: Expression = withOrigin(origin) {

Review Comment:
   I see. I think the best way is
   ```
   protected def createReplacement ... // implemented by subclasses
   lazy val replacement = withOrigin(this)(createReplacement)
   ```
   But this needs to change all subclasses.
   
   Let's leave it for the future.



-- 
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

Re: [PR] [SPARK-51388][SQL] Improve SQL fragment propagation in to_timestamp and UNION [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #50154: [SPARK-51388][SQL] Improve SQL fragment propagation in to_timestamp and UNION
URL: https://github.com/apache/spark/pull/50154


-- 
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

Re: [PR] [SPARK-51388][SQL] Improve SQL fragment propagation in to_timestamp and UNION [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50154:
URL: https://github.com/apache/spark/pull/50154#discussion_r1980719180


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -2104,11 +2105,13 @@ case class ParseToDate(
     ansiEnabled: Boolean = SQLConf.get.ansiEnabled)
   extends RuntimeReplaceable with ImplicitCastInputTypes with TimeZoneAwareExpression {
 
-  override lazy val replacement: Expression = format.map { f =>
-    Cast(GetTimestamp(left, f, TimestampType, "try_to_date", timeZoneId, ansiEnabled), DateType,
-      timeZoneId, EvalMode.fromBoolean(ansiEnabled))
-  }.getOrElse(Cast(left, DateType, timeZoneId,
-    EvalMode.fromBoolean(ansiEnabled))) // backwards compatibility
+  override lazy val replacement: Expression = withOrigin(origin) {

Review Comment:
   This looks like a general issue. Shall we fix it at the framework level? e.g.
   
   ```
   trait RuntimeReplaceable ... {
     def replacement
   
     def replacementFinal = withOrigin(this)(replacement)
   
     // all the invocations of `replacement` should use `replacementFinal` instead.
   }
   ```



-- 
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