zsxwing commented on a change in pull request #24619: [SPARK-27735][SS] Parsing
interval string should be case-insensitive in SS
URL: https://github.com/apache/spark/pull/24619#discussion_r284825671
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -695,8 +695,14 @@ class Dataset[T] private[sql](
// defined on a derived column cannot referenced elsewhere in the plan.
def withWatermark(eventTime: String, delayThreshold: String): Dataset[T] =
withTypedPlan {
val parsedDelay =
- Option(CalendarInterval.fromString("interval " + delayThreshold))
- .getOrElse(throw new AnalysisException(s"Unable to parse time delay
'$delayThreshold'"))
+ try {
+ CalendarInterval.fromCaseInsensitiveString(delayThreshold)
+ } catch {
+ case e: IllegalArgumentException =>
+ throw new AnalysisException(
+ s"Unable to parse time delay '$delayThreshold'",
+ cause = Some(e))
Review comment:
> I'm wondering if we need to propagate `cause` here because previously we
didn't.
Previously it was just a `null` return value and there was no `cause`. Since
we now get a `cause`, it's better to propagate it so that the error will be
clearer.
> And, for the other instance in this PR, the exception messages are not the
same.
This is something we need to pay to unify all usages. At least, the
exception type doesn't change.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
