MaxGekk commented on code in PR #56207:
URL: https://github.com/apache/spark/pull/56207#discussion_r3326573766
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala:
##########
@@ -141,6 +142,61 @@ class OrderingSuite extends SparkFunSuite with
ExpressionEvalHelper {
GenerateOrdering.generate(Array.fill(5000)(sortOrder).toImmutableArraySeq)
}
+ // SPARK-57103: ordering for nanosecond timestamp types. Not driven by the
generic
+ // `atomicTypes` loop above because `RandomDataGenerator` does not yet
support the new
+ // types (tracked separately in SPARK-57034); we hand-roll edge cases here
instead.
+ private def compareNanos(
+ dataType: AtomicType,
+ a: TimestampNanosVal,
+ b: TimestampNanosVal,
+ expected: Int): Unit = {
+ test(s"compare two $dataType values: a = $a, b = $b") {
+ val rowA = InternalRow(a)
+ val rowB = InternalRow(b)
+ Seq(Ascending, Descending).foreach { direction =>
+ val sortOrder = direction match {
+ case Ascending => BoundReference(0, dataType, nullable = true).asc
+ case Descending => BoundReference(0, dataType, nullable = true).desc
+ }
+ val expectedCompareResult = direction match {
+ case Ascending => signum(expected)
+ case Descending => -1 * signum(expected)
+ }
+ val intOrdering = new InterpretedOrdering(sortOrder :: Nil)
+ val genOrdering = new LazilyGeneratedOrdering(sortOrder :: Nil)
+ Seq(intOrdering, genOrdering).foreach { ordering =>
+ assert(ordering.compare(rowA, rowA) === 0)
+ assert(ordering.compare(rowB, rowB) === 0)
+ assert(signum(ordering.compare(rowA, rowB)) ===
expectedCompareResult)
+ assert(signum(ordering.compare(rowB, rowA)) === -1 *
expectedCompareResult)
+ }
+ }
+ }
+ }
+
+ Seq(TimestampNTZNanosType(9), TimestampLTZNanosType(9)).foreach { dt =>
Review Comment:
The chosen edge cases are good. Two cheap additions would round out coverage:
- **A null case.** The `BoundReference` is `nullable = true`, but no null
values are ever passed, so null-vs-value ordering for nanos columns isn't
exercised. It's generic (handled in `BaseOrdering`), so low risk, but nearly
free to cover.
- **A non-`9` precision case** (e.g. `7` or `8`). Ordering is
precision-independent here, so one case would document that intent.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -667,6 +667,7 @@ class CodegenContext extends Logging {
case dt: DataType if isPrimitiveType(dt) => s"($c1 > $c2 ? 1 : $c1 < $c2 ?
-1 : 0)"
case BinaryType =>
s"org.apache.spark.unsafe.types.ByteArray.compareBinary($c1, $c2)"
case CalendarIntervalType => s"$c1.compareTo($c2)"
+ case _: TimestampNTZNanosType | _: TimestampLTZNanosType =>
s"$c1.compareTo($c2)"
Review Comment:
Consider restoring a short comment here explaining *why* this arm is needed.
It's not self-evident: `TimestampNTZNanosType`/`TimestampLTZNanosType` are
`AtomicType`s, so without this case they'd fall through to the `case other if
other.isInstanceOf[AtomicType] => s"$c1.compare($c2)"` fallback a few lines
below — which emits `.compare(...)`, a method `TimestampNanosVal` does not have
(it's `Comparable`, providing `compareTo`, not `Ordered`). A future maintainer
could reasonably delete this arm as "redundant" and silently break codegen
compilation for these types. Something like:
```scala
// AtomicType, but the generic AtomicType fallback below emits `.compare`,
which
// TimestampNanosVal (Comparable, not Ordered) does not have; emit
`.compareTo`.
case _: TimestampNTZNanosType | _: TimestampLTZNanosType =>
s"$c1.compareTo($c2)"
```
--
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]
