MaxGekk commented on a change in pull request #32645:
URL: https://github.com/apache/spark/pull/32645#discussion_r650735752
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##########
@@ -345,6 +346,66 @@ case class MakeInterval(
)
}
+// scalastyle:off line.size.limit
Review comment:
Is this really needed? The code below fits to 100 chars in lines.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##########
@@ -345,6 +346,66 @@ case class MakeInterval(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(years, months) - Make year-month interval from years,
months.",
+ arguments = """
+ Arguments:
+ * years - the number of years, positive or negative
+ * months - the number of months, positive or negative
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(1, 2);
+ 1-2
+ > SELECT _FUNC_(1, 0);
+ 1-0
+ > SELECT _FUNC_(0, 1);
+ 0-1
+ """,
+ since = "3.2.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MakeYMInterval(years: Expression, months: Expression)
+ extends BinaryExpression with ImplicitCastInputTypes with NullIntolerant {
+
+ def this(years: Expression) = this(years, Literal(0))
+ def this() = this(Literal(0))
+
+ override def left: Expression = years
+ override def right: Expression = months
+ override def inputTypes: Seq[AbstractDataType] = Seq(IntegerType,
IntegerType)
+ override def dataType: DataType = YearMonthIntervalType
+ override def nullable: Boolean = children.exists(_.nullable)
Review comment:
Actually, the parent class has the same implementation. You can remove
this, I guess.
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala
##########
@@ -446,4 +446,10 @@ class IntervalExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
checkEvaluation(ExtractANSIIntervalMinutes(Literal(null,
DayTimeIntervalType)), null)
checkEvaluation(ExtractANSIIntervalSeconds(Literal(null,
DayTimeIntervalType)), null)
}
+
+ test("SPARK-35129: make_ym_interval") {
+ checkEvaluation(MakeYMInterval(Literal(0), Literal(10)), 10)
+ checkEvaluation(MakeYMInterval(Literal(1), Literal(10)), 22)
+ checkEvaluation(MakeYMInterval(Literal(1), Literal(0)), 12)
Review comment:
Please, test:
1. Negative parameters
2. Other types (check implicit casts)
3. Negative tests (check wrong parameters).
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##########
@@ -345,6 +346,66 @@ case class MakeInterval(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(years, months) - Make year-month interval from years,
months.",
+ arguments = """
+ Arguments:
+ * years - the number of years, positive or negative
+ * months - the number of months, positive or negative
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(1, 2);
+ 1-2
+ > SELECT _FUNC_(1, 0);
+ 1-0
+ > SELECT _FUNC_(0, 1);
+ 0-1
+ """,
+ since = "3.2.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MakeYMInterval(years: Expression, months: Expression)
+ extends BinaryExpression with ImplicitCastInputTypes with NullIntolerant {
+
+ def this(years: Expression) = this(years, Literal(0))
+ def this() = this(Literal(0))
+
+ override def left: Expression = years
+ override def right: Expression = months
+ override def inputTypes: Seq[AbstractDataType] = Seq(IntegerType,
IntegerType)
+ override def dataType: DataType = YearMonthIntervalType
+ override def nullable: Boolean = children.exists(_.nullable)
+
+ private def evalIntValue(dt: DataType, value: Any): Long = dt match {
+ case _: ByteType | _: ShortType | _: IntegerType =>
value.asInstanceOf[Number].longValue()
Review comment:
If you extend `ImplicitCastInputTypes`, why do you expect other types.
Could you explain, please.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##########
@@ -345,6 +346,66 @@ case class MakeInterval(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(years, months) - Make year-month interval from years,
months.",
+ arguments = """
+ Arguments:
+ * years - the number of years, positive or negative
+ * months - the number of months, positive or negative
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(1, 2);
+ 1-2
+ > SELECT _FUNC_(1, 0);
+ 1-0
+ > SELECT _FUNC_(0, 1);
+ 0-1
+ """,
+ since = "3.2.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MakeYMInterval(years: Expression, months: Expression)
+ extends BinaryExpression with ImplicitCastInputTypes with NullIntolerant {
+
+ def this(years: Expression) = this(years, Literal(0))
+ def this() = this(Literal(0))
+
+ override def left: Expression = years
+ override def right: Expression = months
+ override def inputTypes: Seq[AbstractDataType] = Seq(IntegerType,
IntegerType)
+ override def dataType: DataType = YearMonthIntervalType
+ override def nullable: Boolean = children.exists(_.nullable)
+
+ private def evalIntValue(dt: DataType, value: Any): Long = dt match {
+ case _: ByteType | _: ShortType | _: IntegerType =>
value.asInstanceOf[Number].longValue()
+ case _: LongType => value.asInstanceOf[Long]
+ }
+
+ override def nullSafeEval(year: Any, month: Any): Any = {
+ LongExactNumeric.toInt(Math.addExact(evalIntValue(right.dataType, month),
Review comment:
Why not `Math.toIntExact` instead of `LongExactNumeric.toInt`?
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##########
@@ -345,6 +346,66 @@ case class MakeInterval(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(years, months) - Make year-month interval from years,
months.",
+ arguments = """
+ Arguments:
+ * years - the number of years, positive or negative
+ * months - the number of months, positive or negative
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(1, 2);
+ 1-2
+ > SELECT _FUNC_(1, 0);
+ 1-0
+ > SELECT _FUNC_(0, 1);
+ 0-1
Review comment:
I would rather show negative parameters.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##########
@@ -345,6 +346,66 @@ case class MakeInterval(
)
}
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(years, months) - Make year-month interval from years,
months.",
+ arguments = """
+ Arguments:
+ * years - the number of years, positive or negative
+ * months - the number of months, positive or negative
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(1, 2);
+ 1-2
+ > SELECT _FUNC_(1, 0);
+ 1-0
+ > SELECT _FUNC_(0, 1);
+ 0-1
+ """,
+ since = "3.2.0",
+ group = "datetime_funcs")
+// scalastyle:on line.size.limit
+case class MakeYMInterval(years: Expression, months: Expression)
+ extends BinaryExpression with ImplicitCastInputTypes with NullIntolerant {
+
+ def this(years: Expression) = this(years, Literal(0))
+ def this() = this(Literal(0))
+
+ override def left: Expression = years
+ override def right: Expression = months
+ override def inputTypes: Seq[AbstractDataType] = Seq(IntegerType,
IntegerType)
+ override def dataType: DataType = YearMonthIntervalType
+ override def nullable: Boolean = children.exists(_.nullable)
+
+ private def evalIntValue(dt: DataType, value: Any): Long = dt match {
+ case _: ByteType | _: ShortType | _: IntegerType =>
value.asInstanceOf[Number].longValue()
+ case _: LongType => value.asInstanceOf[Long]
+ }
+
+ override def nullSafeEval(year: Any, month: Any): Any = {
+ LongExactNumeric.toInt(Math.addExact(evalIntValue(right.dataType, month),
+ Math.multiplyExact(evalIntValue(left.dataType, year), MONTHS_PER_YEAR)))
+ }
+
+ override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ defineCodeGen(ctx, ev, (years, months) => {
+ val extractor = LongExactNumeric.getClass.getName.stripSuffix("$")
+ s"""
+ |$extractor.toInt(java.lang.Math.addExact($months,
+ | java.lang.Math.multiplyExact($years, $MONTHS_PER_YEAR)))
+ |""".stripMargin
+ })
+ }
+
+ override def prettyName: String = "make_ym_interval"
+
+ override protected def withNewChildrenInternal(
+ newLeft: Expression,
+ newRight: Expression): Expression =
+ copy(years = newLeft, months = newRight)
+}
Review comment:
Could you clarify (add comments) why do you need to override
`withNewChildrenInternal`.
--
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
