Skip to content

[SPARK-47696][SQL] try_to_timestamp should handle SparkUpgradeException #45822

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gengliangwang wants to merge 2 commits into from
Closed

[SPARK-47696][SQL] try_to_timestamp should handle SparkUpgradeException #45822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ae9c87
fix
gengliangwang Apr 2, 2024
98fe991
add comment
gengliangwang Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,11 +1235,13 @@ object TryToTimestampExpressionBuilder extends ExpressionBuilder {
override def build(funcName: String, expressions: Seq[Expression]): Expression = {
val numArgs = expressions.length
if (numArgs == 1 || numArgs == 2) {
ParseToTimestamp(
// The expression ParseToTimestamp will throw an SparkUpgradeException if the input is invalid
// even when failOnError is false. We need to catch the exception and return null.
TryEval(ParseToTimestamp(
Copy link
Contributor

@cloud-fan cloud-fan Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we simply update ToTimestamp#eval and codegen to catch SparkUpgradeException additionally? I'm a bit worried about increasing the scope and swallow all errors.

Copy link
Member Author

@gengliangwang gengliangwang Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in ToTimestamp, there is supposed to be a SparkUpgradeException even when failOnErroris false.
To avoid swallowing all errors, we can add another flag isTryFunction besides failOnError. It will make the code more complicated though.

Copy link
Contributor

@cloud-fan cloud-fan Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we need to maintain 3 behaviors:

  1. try_to_timestamp never fails
  2. to_timestamp ansi mode always fail with invalid input
  3. to_timestamp non-ansi mode only fails with SparkUpgradeException

Do I understand it correctly?

Copy link
Contributor

@cloud-fan cloud-fan Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why try_to_timestamp need a different behavior with to_timestamp non-ansi mode.

Copy link
Member Author

@gengliangwang gengliangwang Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why try_to_timestamp need a different behavior with to_timestamp non-ansi mode.

The try_* functions are supposed to return null on invalid inputs, instead of compatible with non-ansi mode. cc @srielau on this one.

Copy link
Contributor

@cloud-fan cloud-fan Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This upgrade issue seems special and we should always throw it to provide upgrade instructions. How about #45853 ?

Copy link
Member Author

@gengliangwang gengliangwang Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan after #45853, users will face this bug when setting the timeParserPolicy as legacy.

Copy link
Contributor

@cloud-fan cloud-fan Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can argue this is not a bug as the upgrade exception must be thrown.

Copy link
Member Author

@gengliangwang gengliangwang Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan ok, let me close this one for now.

expressions.head,
expressions.drop(1).lastOption,
SQLConf.get.timestampType,
failOnError = false)
failOnError = false))
} else {
throw QueryCompilationErrors.wrongNumArgsError(funcName, Seq(1, 2), numArgs)
}
Expand Down
5 changes: 5 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/DateFunctionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1392,4 +1392,9 @@ class DateFunctionsSuite extends QueryTest with SharedSparkSession {
checkAnswer(df.selectExpr("try_to_timestamp(a)"), Seq(Row(ts)))
checkAnswer(df.select(try_to_timestamp(col("a"))), Seq(Row(ts)))
}

test("try_to_timestamp: return null on SparkUpgradeException") {
val df = spark.sql("SELECT try_to_timestamp('2016-12-1', 'yyyy-MM-dd')")
checkAnswer(df, Seq(Row(null)))
}
}