MaxGekk commented on code in PR #53312:
URL: https://github.com/apache/spark/pull/53312#discussion_r3480563226
##########
sql/core/src/test/scala/org/apache/spark/sql/StatisticsCollectionSuite.scala:
##########
@@ -598,6 +598,127 @@ class StatisticsCollectionSuite extends
StatisticsCollectionTestBase with Shared
}
}
+ test("TimeType column statistics collection and precisions") {
+ val table = "time_stats_table"
+ withTable(table) {
+ // Test all precisions (0-6) with NULL handling
+ sql(s"""
+ CREATE TABLE $table (
+ id INT,
+ time_p0 TIME(0),
+ time_p1 TIME(1),
+ time_p2 TIME(2),
+ time_p3 TIME(3),
+ time_p4 TIME(4),
+ time_p5 TIME(5),
+ time_p6 TIME(6)
+ ) USING parquet
+ """)
+
+ sql(s"""
+ INSERT INTO $table VALUES
+ (1, TIME '08:30:00', TIME '08:30:00.1', TIME '08:30:00.12',
+ TIME '08:30:00.123', TIME '08:30:00.1234', TIME '08:30:00.12345',
+ TIME '08:30:00.123456'),
+ (2, TIME '17:45:30', TIME '17:45:30.9', TIME '17:45:30.98',
+ TIME '17:45:30.987', TIME '17:45:30.9876', TIME '17:45:30.98765',
+ TIME '17:45:30.987654'),
+ (3, TIME '12:00:00', TIME '12:00:00.5', TIME '12:00:00.5',
+ TIME '12:00:00.5', TIME '12:00:00.5', TIME '12:00:00.5',
+ TIME '12:00:00.5'),
+ (4, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+ """)
+
+ sql(s"""ANALYZE TABLE $table COMPUTE STATISTICS FOR COLUMNS
+ time_p0, time_p1, time_p2, time_p3, time_p4, time_p5, time_p6""")
+
+ val catalogTable = getCatalogTable(table)
+
+ for (precision <- 0 to 6) {
Review Comment:
The precision sweep covers TIME(0)–TIME(6), but TIME supports up to TIME(9)
(nanoseconds). The fraction formatter already emits up to 9 digits, so the path
is correct — a single TIME(9) min/max round-trip case (e.g.
`23:59:59.123456789`) would lock that behavior in against regressions.
##########
sql/core/src/test/scala/org/apache/spark/sql/StatisticsCollectionTestBase.scala:
##########
@@ -68,6 +69,14 @@ abstract class StatisticsCollectionTestBase extends
QueryTest with SQLTestUtils
t2.setNanos(987654000)
private val tsNTZ2 = LocalDateTime.parse(t2Str.replace(" ", "T"))
+ // Time test values
+ private val time1Str = "10:30:45.123456"
+ private val time1 = LocalTime.parse(time1Str)
+ private val time1Internal = localTimeToNanos(time1)
Review Comment:
`time1Internal`/`time2Internal` (and the `localTimeToNanos` import added for
them at the top of the file) are never read. Either drop all three, or use the
internal nanos-of-day values in an assertion — they'd be a natural check for
the plan-stat `min`/`max` Long values.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/CommandUtils.scala:
##########
@@ -364,6 +364,7 @@ object CommandUtils extends Logging {
case _: IntegralType => true
case _: DecimalType => true
case DoubleType | FloatType => true
+ case _: TimeType => false // TimeType doesn't support histogram
Review Comment:
Histograms are disabled for TIME here, but every sibling `DatetimeType`
(`Date`/`Timestamp`/`TimestampNTZ`) builds equi-height histograms via the case
just below. The PR description's "circular/periodic data" rationale is
debatable: time-of-day is linearly ordered within `[0, 24h)` and stored as a
`Long` (nanos-of-day), so
`ApproxCountDistinctForIntervals`/`ApproximatePercentile` work on it exactly as
for `Timestamp` — there's no technical blocker. The cost is less accurate
selectivity estimation for TIME range/equality predicates than its siblings.
Was histogram support intentionally deferred? If so, a one-line comment saying
that (rather than the circular-data argument) would be clearer.
##########
sql/core/src/test/scala/org/apache/spark/sql/StatisticsCollectionSuite.scala:
##########
@@ -598,6 +598,127 @@ class StatisticsCollectionSuite extends
StatisticsCollectionTestBase with Shared
}
}
+ test("TimeType column statistics collection and precisions") {
+ val table = "time_stats_table"
+ withTable(table) {
+ // Test all precisions (0-6) with NULL handling
+ sql(s"""
+ CREATE TABLE $table (
+ id INT,
+ time_p0 TIME(0),
+ time_p1 TIME(1),
+ time_p2 TIME(2),
+ time_p3 TIME(3),
+ time_p4 TIME(4),
+ time_p5 TIME(5),
+ time_p6 TIME(6)
+ ) USING parquet
+ """)
+
+ sql(s"""
+ INSERT INTO $table VALUES
+ (1, TIME '08:30:00', TIME '08:30:00.1', TIME '08:30:00.12',
+ TIME '08:30:00.123', TIME '08:30:00.1234', TIME '08:30:00.12345',
+ TIME '08:30:00.123456'),
+ (2, TIME '17:45:30', TIME '17:45:30.9', TIME '17:45:30.98',
+ TIME '17:45:30.987', TIME '17:45:30.9876', TIME '17:45:30.98765',
+ TIME '17:45:30.987654'),
+ (3, TIME '12:00:00', TIME '12:00:00.5', TIME '12:00:00.5',
+ TIME '12:00:00.5', TIME '12:00:00.5', TIME '12:00:00.5',
+ TIME '12:00:00.5'),
+ (4, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+ """)
+
+ sql(s"""ANALYZE TABLE $table COMPUTE STATISTICS FOR COLUMNS
+ time_p0, time_p1, time_p2, time_p3, time_p4, time_p5, time_p6""")
+
+ val catalogTable = getCatalogTable(table)
+
+ for (precision <- 0 to 6) {
+ val col = s"time_p$precision"
+ val stats = catalogTable.stats.get.colStats(col)
+
+ // Verify basic statistics
+ assert(stats.distinctCount.isDefined, s"Distinct count should be
defined for $col")
+ assert(stats.min.isDefined, s"Min should be defined for $col")
+ assert(stats.max.isDefined, s"Max should be defined for $col")
+ assert(stats.nullCount == Some(1), s"Null count should be 1 for $col")
+ assert(stats.avgLen == Some(8), s"Avg length should be 8 bytes for
$col")
+ assert(stats.maxLen == Some(8), s"Max length should be 8 bytes for
$col")
+
+ // Verify format for each precision
+ val minStr = stats.min.get.asInstanceOf[String]
+ val maxStr = stats.max.get.asInstanceOf[String]
+ assert(minStr.matches("\\d{2}:\\d{2}:\\d{2}(\\.\\d+)?"),
+ s"Min should be time format for $col, got: $minStr")
Review Comment:
Optional: these assert only that min/max match the time-format regex, not
their exact values, so a swapped or wrong min/max would still pass here. Exact
values are covered by the `ctime` round-trip in `StatisticsCollectionTestBase`,
so this is polish — asserting the concrete `08:30:00`/`17:45:30` per precision
would make the test self-sufficient.
--
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]
