-
Notifications
You must be signed in to change notification settings - Fork 29.4k
[SPARK-59440][SQL] Support the TIME data type in the hll_sketch_agg function - #58744
Open
stevomitric wants to merge 2 commits into
Open
[SPARK-59440][SQL] Support the TIME data type in the hll_sketch_agg function #58744stevomitric wants to merge 2 commits into
stevomitric wants to merge 2 commits into
Conversation
...unction ### What changes were proposed in this pull request? Add support for the TIME data type to the `hll_sketch_agg` aggregate. TIME is physically stored as a long (nanoseconds since midnight), so it is hashed into the HllSketch exactly like the existing `LongType` path. `hll_union_agg` needs no change: it only ingests already-serialized BINARY sketches, so a sketch built from a TIME column merges through it unchanged. ### Why are the changes needed? Part of SPARK-57550 (extend support for the TIME data type). Approximate distinct counting over TIME columns is a natural, previously-missing capability. ### Does this PR introduce any user-facing change? Yes. `hll_sketch_agg(time_col[, lgConfigK])` is now accepted; previously it raised an analysis error for TIME inputs. ### How was this patch tested? New unit test in `DatasketchesHllSketchSuite` covering analyzer acceptance, cardinality estimation over TIME values, precision-insensitive de-duplication, and a `hll_union_agg` round-trip over sketches built from a TIME column. Co-authored-by: Isaac <no-reply@databricks.com>
...upport Follow-up to the initial commit, addressing code-review feedback: - Regenerate the hll.sql golden files (results and analyzer-results). Adding the TIME type to the input TypeCollection changes the DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE error text, which the ARRAY negative test asserts; the stale golden would have failed CI. - Update the remaining public doc surfaces to mention TIME: the SQL reference table (docs/sql-ref-sketch-aggregates.md) and all five hll_sketch_agg scaladocs in functions.scala. - Add end-to-end SQL coverage in hll.sql: hll_sketch_agg over a TIME column and an hll_union_agg round-trip over sketches built from TIME columns. - Strengthen DatasketchesHllSketchSuite: a sub-microsecond-differing case (so a regression that truncated nanoseconds before hashing would be caught) and a mixed-precision union. - Add a .. versionchanged:: note to the Python hll_sketch_agg docstring. Co-authored-by: Isaac <no-reply@databricks.com>
stevomitric
commented
Sep 12, 2026
Contributor
Author
cc @uros-b PTAL.
uros-b
uros-b
reviewed
Sep 12, 2026
@uros-b
uros-b
left a comment
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sql/catalyst/.../aggregate/datasketchesAggregates.scala (inputTypes TypeCollection, ~L118-124) -- Placing AnyTimeType before StringType in the ordered TypeCollection silently changes coercion for TIMESTAMP / TIMESTAMP_NTZ / DATE inputs in the default ANSI configuration. For a TIME input the ordering is irrelevant (it is accepted by the order-independent expectedType.acceptsType short-circuit in implicitCast), but for a type the collection does not directly accept, ANSI implicitCast walks the members in order and takes the first that canANSIStoreAssign permits. canANSIStoreAssign(TimestampType, TimeType(6)) is true via the (_: DatetimeType, _: DatetimeType) arm, and AnyTimeType now precedes StringType, so:
- On master,
hll_sketch_agg(ts_col)coerces the timestamp to STRING (AtomicType -> StringTypestore-assign) and sketches its string form, counting distinct timestamps correctly. - After this PR, the same call coerces to TIME(6) and sketches only the nanos-of-day (
canAnsiCast(TimestampType, TimeType)istrue, so the cast executes) -- timestamps that share a wall-clock time-of-day but differ in date collapse to one sketch entry, silently under-counting distinct values. TIMESTAMP_NTZ behaves identically. DATE is also re-routed to a TIME(6) target (store-assign true) but has noDateType -> TimeTypecast rule, so it takes an error path instead of its prior working STRING path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Add support for the TIME data type to the
hll_sketch_aggaggregate. TIME is physically stored as a long (nanoseconds since midnight), so it is hashed into the HllSketch exactly like the existingLongTypepath.hll_union_aggneeds no change: it only ingests already-serialized BINARY sketches, so a sketch built from a TIME column merges through it unchanged.Why are the changes needed?
Part of SPARK-57550 (extend support for the TIME data type). Approximate distinct counting over TIME columns is a natural, previously-missing capability.
Does this PR introduce any user-facing change?
Yes.
hll_sketch_agg(time_col[, lgConfigK])is now accepted; previously it raised an analysis error for TIME inputs.How was this patch tested?
New unit test in
DatasketchesHllSketchSuitecovering analyzer acceptance, cardinality estimation over TIME values, precision-insensitive de-duplication, and ahll_union_agground-trip over sketches built from a TIME column.Was this patch authored or co-authored using generative AI tooling?
Co-Authored-By: Claude Opus 4.8