viirya commented on code in PR #55940:
URL: https://github.com/apache/spark/pull/55940#discussion_r3261976552
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala:
##########
@@ -399,42 +399,54 @@ case class MakeInterval(
hour: Any,
min: Any,
sec: Option[Any]): Any = {
- try {
- IntervalUtils.makeInterval(
+ val secs = sec.map(_.asInstanceOf[Decimal]).getOrElse(Decimal(0,
Decimal.MAX_LONG_DIGITS, 6))
+ if (failOnError) {
+ DateTimeConstructorUtils.makeIntervalExact(
year.asInstanceOf[Int],
month.asInstanceOf[Int],
week.asInstanceOf[Int],
day.asInstanceOf[Int],
hour.asInstanceOf[Int],
min.asInstanceOf[Int],
- sec.map(_.asInstanceOf[Decimal]).getOrElse(Decimal(0,
Decimal.MAX_LONG_DIGITS, 6)))
- } catch {
- case e: ArithmeticException =>
- if (failOnError) {
- throw QueryExecutionErrors.arithmeticOverflowError(e.getMessage)
- } else {
- null
- }
+ secs)
+ } else {
+ try {
+ IntervalUtils.makeInterval(
+ year.asInstanceOf[Int],
+ month.asInstanceOf[Int],
+ week.asInstanceOf[Int],
+ day.asInstanceOf[Int],
+ hour.asInstanceOf[Int],
+ min.asInstanceOf[Int],
+ secs)
+ } catch {
+ case _: ArithmeticException => null
+ }
}
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
- nullSafeCodeGen(ctx, ev, (year, month, week, day, hour, min, sec) => {
- val iu = IntervalUtils.getClass.getName.stripSuffix("$")
- val secFrac = sec.getOrElse("0")
- val failOnErrorBranch = if (failOnError) {
- """throw QueryExecutionErrors.arithmeticOverflowError(e.getMessage(),
"", null);"""
- } else {
- s"${ev.isNull} = true;"
- }
- s"""
- try {
- ${ev.value} = $iu.makeInterval($year, $month, $week, $day, $hour,
$min, $secFrac);
- } catch (java.lang.ArithmeticException e) {
- $failOnErrorBranch
- }
- """
- })
+ if (failOnError) {
+ val utils = classOf[DateTimeConstructorUtils].getName
+ nullSafeCodeGen(ctx, ev, (year, month, week, day, hour, min, sec) => {
+ val secFrac =
sec.getOrElse("org.apache.spark.sql.types.Decimal$.MODULE$.apply(0, " +
Review Comment:
secFrac fallback in ANSI codegen is opaque.
Two problems:
- Dead code in practice. MakeInterval.children always has 7 elements
(all the auxiliary this(...) constructors pass Literal(0) /
Literal(Decimal(...)) to fill missing slots), so
SeptenaryExpression.nullSafeCodeGen always passes Some(...) to the lambda.
getOrElse never fires.
- Inconsistency with the non-ANSI branch in the same file: val secFrac =
sec.getOrElse("0") // non-ANSI branch. The non-ANSI branch's "0" is int-typed
but IntervalUtils.makeInterval's 7th argument is Decimal. If the getOrElse ever
fired, the non-ANSI path would emit … makeInterval(…, 0) which wouldn't
compile, while the ANSI path would emit a valid Decimal$.MODULE$.apply(0, ...).
You appear to have noticed the type issue but only fixed one branch. Both
branches' fallbacks are unreachable, but it's still an inconsistency that will
confuse future readers
The pre-PR code had the same "0" dead-code fallback. So the PR didn't
introduce the dead code — but it did expand it into a more elaborate (still
dead) form on one branch only. Cleanest fix: replace sec.getOrElse(...) with
sec.get (assertive) and add a comment that MakeInterval always provides 7
children. Or stick with getOrElse but use the same string on both branches.
--
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]
