You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "yadavay-amzn (via GitHub)" <gi...@apache.org> on 2026/06/30 20:50:53 UTC
[PR] [SPARK-57784][SQL] Support the TIME data type in cost-based optimizer statistics estimation [spark]
yadavay-amzn opened a new pull request, #56910:
URL: https://github.com/apache/spark/pull/56910
### What changes were proposed in this pull request?
Add `TimeType` support to cost-based optimizer (CBO) statistics estimation so range-based selectivity, join, and union estimation work for TIME columns. `TimeType` (stored as nanoseconds-of-day in a Long) is handled at every site the other fixed-width temporal types use - `EstimationUtils.toDouble/fromDouble`, `FilterEstimation.evaluateBinary/evaluateInSet`, `UnionEstimation.isTypeSupported`, and `CatalogColumnStat` min/max external-string (de)serialization (via `TimeFormatter`) - mirroring the existing `TimestampType` (micros-as-Long) handling.
### Why are the changes needed?
Before this, filters/joins/unions over a TIME column fell back to default selectivity because `TimeType` was not among the types CBO estimates, yielding poor row-count estimates and plans. TIME maps cleanly to a numeric scale (nanos-of-day), so it can be estimated the same way as DATE/TIMESTAMP.
### Does this PR introduce _any_ user-facing change?
No. (Affects plan costing/estimates only, not query results.)
### How was this patch tested?
New `FilterEstimationSuite` tests for TIME range/equality/IN selectivity (asserting exact `rowCount`/`distinctCount`/`min`/`max`), and extended `JoinEstimationSuite`/`UnionEstimation` tests with TIME columns. All existing CBO estimation tests pass; scalastyle clean.
### Was this patch authored or co-authored using generative AI tooling?
Authored with assistance by Claude Opus 4.8.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57784][SQL] Support the TIME data type in cost-based optimizer statistics estimation [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on code in PR #56910:
URL: https://github.com/apache/spark/pull/56910#discussion_r3502321750
##########
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:
Fixed - switched the TimeType arm to TimeFormatter.getFractionFormatter(), which preserves up to nanosecond resolution and round-trips losslessly with the stringToTimeAnsi parse on the read side. Verified 12:00:00.123456 (nanos 43200123456000) now survives toExternalString -> fromExternalString unchanged (previously it serialized to "12:00:00" and read back as 43200000000000).
##########
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:
Added a sub-second TIME min/max round-trip test in StatisticsCollectionSuite using 12:00:00.123456. Confirmed it FAILS with the old fractionless formatter (deserializes to 43200000000000) and passes with the fraction formatter, so it guards this truncation going forward.
##########
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:
Reworded to: (12:00 - 08:00) / (17:00 - 08:00) = 4/9 of 10 rows => ~4.44 => ceil => 5.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57784][SQL] Support the TIME data type in cost-based optimizer statistics estimation [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #56910:
URL: https://github.com/apache/spark/pull/56910#issuecomment-6243855027
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57784][SQL] Support the TIME data type in cost-based optimizer statistics estimation [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on PR #56910:
URL: https://github.com/apache/spark/pull/56910#issuecomment-4922051197
@MaxGekk Addressed the review comments: the TIME (de)serialization arm now uses TimeFormatter.getFractionFormatter() so sub-second precision round-trips losslessly, added a sub-second min/max round-trip test in StatisticsCollectionSuite, and reworded the selectivity comment. Could you take another look when you have a chance?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57784][SQL] Support the TIME data type in cost-based optimizer statistics estimation [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on PR #56910:
URL: https://github.com/apache/spark/pull/56910#issuecomment-4930857755
CI failure looks unrelated to the change, re-running
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57784][SQL] Support the TIME data type in cost-based optimizer statistics estimation [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57784][SQL] Support the TIME data type in cost-based optimizer statistics estimation [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #56910: [SPARK-57784][SQL] Support the TIME data type in cost-based optimizer statistics estimation
URL: https://github.com/apache/spark/pull/56910
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org