Skip to content

[SPARK-59440][SQL] Support the TIME data type in the hll_sketch_agg function - #58744

Open
stevomitric wants to merge 2 commits into
apache:masterfrom
stevomitric:stevomitric/spark-hll-sketch-time
Open

[SPARK-59440][SQL] Support the TIME data type in the hll_sketch_agg function#58744
stevomitric wants to merge 2 commits into
apache:masterfrom
stevomitric:stevomitric/spark-hll-sketch-time

Conversation

@stevomitric

Copy link
Copy Markdown
Contributor

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.

Was this patch authored or co-authored using generative AI tooling?

Co-Authored-By: Claude Opus 4.8

stevomitric and others added 2 commits September 11, 2026 14:05
…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

Copy link
Copy Markdown
Contributor Author

cc @uros-b PTAL.

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member

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 -> StringType store-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) is true, 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 no DateType -> TimeType cast rule, so it takes an error path instead of its prior working STRING path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants