MaxGekk commented on code in PR #56910:
URL: https://github.com/apache/spark/pull/56910#discussion_r3502149007
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -1047,6 +1047,7 @@ object CatalogColumnStat extends Logging {
case TimestampType => getTimestampFormatter(isParsing = true).parse(s)
case TimestampNTZType =>
getTimestampFormatter(isParsing = true, forTimestampNTZ =
true).parse(s)
+ case _: TimeType => TimeFormatter(isParsing = true).parse(s)
Review Comment:
These two TIME `(de)serialization` arms aren't covered by the new tests —
the `FilterEstimationSuite` TIME cases use whole-second values and build
`ColumnStat` in-memory, so they never round-trip through
`toExternalString`/`fromExternalString` (which is why the truncation on the
format side goes unnoticed). Worth a sub-second TIME min/max round-trip in the
existing temporal stats coverage (`StatisticsCollectionSuite` — its type list
and `checkTimestampStats` already cover `DateType`/`TimestampType`); a
fractional value there would catch the truncation.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/FilterEstimationSuite.scala:
##########
@@ -523,6 +533,44 @@ class FilterEstimationSuite extends
StatsEstimationTestBase {
expectedRowCount = 3)
}
+ test("ctime = cast('10:00:00' AS TIME)") {
+ val t10 = DateTimeUtils.localTimeToNanos(java.time.LocalTime.of(10, 0, 0))
+ validateEstimatedStats(
+ Filter(EqualTo(attrTime, Literal(t10, TimeType())),
+ childStatsTestPlan(Seq(attrTime), 10L)),
+ Seq(attrTime -> ColumnStat(distinctCount = Some(1),
+ min = Some(t10), max = Some(t10),
+ nullCount = Some(0), avgLen = Some(8), maxLen = Some(8))),
+ expectedRowCount = 1)
+ }
+
+ test("ctime < cast('12:00:00' AS TIME)") {
+ // 12:00 is 43200000000000L nanos. Range is [08:00, 17:00] = 10 distinct
values.
+ // Fraction: (12:00 - 08:00) / (17:00 - 08:00) = 4/9 hours => ~4.44 =>
rounded to 5
Review Comment:
Minor: `4/9` is a dimensionless selectivity ratio — the hour spans in
numerator and denominator cancel, so "hours" is misleading. And `~4.44 =>
rounded to 5` is actually a ceiling (4.44 wouldn't round to 5). Maybe: `(12:00
- 08:00) / (17:00 - 08:00) = 4/9 of 10 rows => ~4.44 => ceil => 5`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -1073,6 +1074,7 @@ object CatalogColumnStat extends Logging {
case TimestampNTZType =>
getTimestampFormatter(isParsing = false, forTimestampNTZ = true)
.format(v.asInstanceOf[Long])
+ case _: TimeType => TimeFormatter(isParsing =
false).format(v.asInstanceOf[Long])
Review Comment:
**Blocking:** this truncates sub-second precision. `TimeFormatter(isParsing
= false)` resolves to `DefaultTimeFormatter`, which overrides only `parse` (→
`stringToTimeAnsi`, preserves nanos) and inherits `format` from
`Iso8601TimeFormatter` with the fractionless `"HH:mm:ss"` pattern. So a TIME
min/max of `12:00:00.123456` (nanos `43200123456000`) is serialized as
`"12:00:00"` and read back by `fromExternalString` as `12:00:00.000000` — a
wrong, truncated min/max persisted for any fractional TIME column, which
silently corrupts the range estimates this PR is adding.
(`fromExternalString`'s parse side is fine; only this format side is lossy.)
Contrast `TimestampType` just above, which formats with the `.SSSSSS`
pattern (lossless to micros). Use a fraction-preserving formatter here:
```suggestion
case _: TimeType =>
TimeFormatter.getFractionFormatter().format(v.asInstanceOf[Long])
```
`FractionTimeFormatter` preserves up to nanosecond resolution (trailing
zeros trimmed) and round-trips with the `stringToTimeAnsi` parse on the read
side.
This path is reachable and net-new: `AnalyzeColumnCommand.supportsType`
accepts `_: DatetimeType` (TimeType extends it), so `ANALYZE … FOR COLUMNS`
over a TIME column collects min/max and persists through here. Pre-PR there was
no TimeType arm, so it hit the `case _ => throw
columnStatisticsSerializationNotSupportedError` fallthrough — i.e. this PR is
what enables ANALYZE-on-TIME, so the lossy serialization ships as new behavior.
(Histogram bins serialize as raw `Double` and are fine; only these `min`/`max`
companions truncate.)
--
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]
