Skip to content

[SPARK-54785][SQL] Add support for binary sketch aggregations in KLL - #53548

Closed
cboumalh wants to merge 11 commits into
apache:masterfrom
cboumalh:cboumalh-kll-enhancement
Closed

[SPARK-54785][SQL] Add support for binary sketch aggregations in KLL#53548
cboumalh wants to merge 11 commits into
apache:masterfrom
cboumalh:cboumalh-kll-enhancement

Conversation

@cboumalh

@cboumalhcboumalh commented Dec 19, 2025

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR adds SQL aggregate functions with their tests for the KLL merge aggregate functions:

  • kll_merge_agg_bigint
  • kll_merge_agg_float
  • kll_merge_agg_double

These aggregate functions merge multiple binary KLL sketch representations.

Initial PRs:

Why are the changes needed?

The existing scalar kll_sketch_merge_* functions can only merge two sketches at a time. In distributed computing scenarios where sketches are pre-computed across multiple partitions, time windows, or datasets, users need to merge many sketches together.

Does this PR introduce any user-facing change?

Yes, this PR adds 3 new aggregate functions.

How was this patch tested?

New SQL tests were added to sql/core/src/test/resources/sql-tests/inputs/kllquantiles.sql:

Positive tests:

  • Merging bigint/float/double sketches from multiple rows
  • Merging with custom k parameters (400, 300, 500)
  • NULL value handling

Negative tests:

  • Type mismatches (passing non-binary types)
  • Invalid binary data
  • k parameter validation (too small, too large, NULL, non-constant)

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

claude-4.5-sonnet and manual changes.

@cboumalh
cboumalh marked this pull request as draft December 19, 2025 20:52
@cboumalhcboumalh changed the title Add support for binary sketch aggregations in KLL[WIP][SPARK-54785][SQL] Add support for binary sketch aggregations in KLLDec 19, 2025
@cboumalh

cboumalh commented Dec 19, 2025

Copy link
Copy Markdown
ContributorAuthor

cc @dtenedor if you have time to look at this! I think this would be useful (and someone pointed it out here). Thank you for your time, and please let me know your thoughts. I'll update it with the rest of the code (docs, pyspark, dataframe, etc..) if we're moving forward with this.

@cboumalh
cboumalh marked this pull request as ready for review December 19, 2025 21:04
@cboumalh

cboumalh commented Dec 30, 2025

Copy link
Copy Markdown
ContributorAuthor

@cloud-fan do we want to get this out in 4.1.1 (KLL was part of 4.1)? I can finish it off since 4.1.1 is already RC. Thanks!

@dtenedordtenedor changed the title [WIP][SPARK-54785][SQL] Add support for binary sketch aggregations in KLL[SPARK-54785][SQL] Add support for binary sketch aggregations in KLLJan 8, 2026
@github-actions

Copy link
Copy Markdown

JIRA Issue Information

=== Improvement SPARK-54785 ===
Summary: Add support for binary sketch aggregations in KLL
Assignee: None
Status: Open
Affected: ["4.1.0","4.1.1"]


This comment was automatically generated by GitHub Actions

@dtenedordtenedor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cboumalh thanks for working on this! The implementation and testing mostly LGTM, we should probably just deduplicate the implementations of the three functions as much as possible to simplify the code.

@dtenedor

Copy link
Copy Markdown
Contributor

@cloud-fan do we want to get this out in 4.1.1 (KLL was part of 4.1)? I can finish it off since 4.1.1 is already RC. Thanks!

@cboumalh@cloud-fan I'd suggest to backport this to 4.1 if possible, since as a couple folks pointed out, the feature is much more usable if you can accumulate multiple previous sketch buffers using aggregate functions. But up to you guys.

@cboumalh
cboumalh requested a review from dtenedorJanuary 8, 2026 22:08
@cboumalh

Copy link
Copy Markdown
ContributorAuthor

Hi @dtenedor, I addressed the comments and added the rest of the code for this PR. Please let me know of any comments, and thank you for helping with this! I'll be scanning it for things to improve too.

@dtenedordtenedor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, just have a few more testing ideas.

Comment threaddocs/sql-ref-sketch-aggregates.md Outdated
Comment threaddocs/sql-ref-sketch-aggregates.md Outdated

@dtenedordtenedor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after implementing remaining code review comments.

@dtenedordtenedor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merging to master and 4.1.

dtenedor pushed a commit that referenced this pull request Jan 12, 2026
This PR adds SQL aggregate functions with their tests for the KLL merge aggregate functions:
- `kll_merge_agg_bigint`
- `kll_merge_agg_float`
- `kll_merge_agg_double`
These aggregate functions merge multiple binary KLL sketch representations.
Initial PRs:
- #52900
- #52800
The existing scalar `kll_sketch_merge_*` functions can only merge two sketches at a time. In distributed computing scenarios where sketches are pre-computed across multiple partitions, time windows, or datasets, users need to merge many sketches together.
Yes, this PR adds 3 new aggregate functions.
New SQL tests were added to `sql/core/src/test/resources/sql-tests/inputs/kllquantiles.sql`:
**Positive tests:**
- Merging bigint/float/double sketches from multiple rows
- Merging with custom k parameters (400, 300, 500)
- NULL value handling
**Negative tests:**
- Type mismatches (passing non-binary types)
- Invalid binary data
- k parameter validation (too small, too large, NULL, non-constant)
claude-4.5-sonnet and manual changes.
Closes#53548 from cboumalh/cboumalh-kll-enhancement.
Lead-authored-by: Chris Boumalhab <cboumalh@amazon.com>
Co-authored-by: Chris Boumalhab <84485659+cboumalh@users.noreply.github.com>
Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>
(cherry picked from commit fc15f72)
Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>
sketch (range 8-65535). If k is not specified, the merged sketch adopts the k value
from the first input sketch.

.. versionadded:: 4.1.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally we don't cherry-pick new feature to old branches, and after this PR, these new functions actually started since 4.1.2.

It is causing confusion when I audit public APIs by comparing 4.2.0 vs 4.1.0.

will fix in #56135

zhengruifeng added a commit that referenced this pull request May 28, 2026
…_agg_* (4.1.0 -> 4.1.2)
### What changes were proposed in this pull request?
Fix the `versionadded` / `since` / `ExpressionDescription.since` annotation on `kll_merge_agg_{bigint,float,double}`, changing `4.1.0` to `4.1.2`. Touches three files:
- `python/pyspark/sql/functions/builtin.py` — 3 `.. versionadded::` docstrings
- `sql/api/src/main/scala/org/apache/spark/sql/functions.scala` — 15 `since` Scaladocs (5 overloads × 3 types)
- `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/kllAggregates.scala` — 3 `ExpressionDescription(since = ...)` annotations
### Why are the changes needed?
All three places currently claim "Since 4.1.0", but:
| Tag | Date | Contains `kll_merge_agg_*`? |
|---|---|---|
| v4.1.0 | 2025-12-11 | No |
| v4.1.1 | 2026-01-02 | No |
| v4.1.2 | 2026-05-16 | **Yes** (via cherry-pick `a39c1b8e5e2`) |
| v4.2.0-preview2 | 2026-02-05 | Yes (via master commit `fc15f726eab`) |
The introducing commit on master (SPARK-54785 / #53548, `fc15f726eab` on 2026-01-12) landed *after* v4.1.0 was tagged, and the branch-4.1 cherry-pick (`a39c1b8e5e2`) shipped first in v4.1.2. No 4.1.0/4.1.1 release contains these functions.
`4.1.2` is the earliest stable release in which they are available, so that is the correct value across all three annotation sites.
### Does this PR introduce _any_ user-facing change?
Documentation-only change. The rendered Python API ref, Scaladoc, and SQL function reference currently render "New in version 4.1.0" / "Since: 4.1.0", which is misleading; this PR corrects all of them to `4.1.2`.
### How was this patch tested?
Doc/annotation-only change. Manually verified each touched site is one of the 15 `kll_merge_agg_*` overload Scaladocs, the 3 KllMergeAgg expression descriptions, or the 3 Python docstrings — sibling annotations on `kll_sketch_agg_*` (which legitimately shipped in v4.1.0) are left unchanged.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (model: claude-opus-4-7)
Closes#56135 from zhengruifeng/spark-doc-fixes-dev1.
Authored-by: Ruifeng Zheng <ruifengz@apache.org>
Signed-off-by: Ruifeng Zheng <ruifengz@foxmail.com>
zhengruifeng added a commit that referenced this pull request May 28, 2026
…_agg_* (4.1.0 -> 4.1.2)
### What changes were proposed in this pull request?
Fix the `versionadded` / `since` / `ExpressionDescription.since` annotation on `kll_merge_agg_{bigint,float,double}`, changing `4.1.0` to `4.1.2`. Touches three files:
- `python/pyspark/sql/functions/builtin.py` — 3 `.. versionadded::` docstrings
- `sql/api/src/main/scala/org/apache/spark/sql/functions.scala` — 15 `since` Scaladocs (5 overloads × 3 types)
- `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/kllAggregates.scala` — 3 `ExpressionDescription(since = ...)` annotations
### Why are the changes needed?
All three places currently claim "Since 4.1.0", but:
| Tag | Date | Contains `kll_merge_agg_*`? |
|---|---|---|
| v4.1.0 | 2025-12-11 | No |
| v4.1.1 | 2026-01-02 | No |
| v4.1.2 | 2026-05-16 | **Yes** (via cherry-pick `a39c1b8e5e2`) |
| v4.2.0-preview2 | 2026-02-05 | Yes (via master commit `fc15f726eab`) |
The introducing commit on master (SPARK-54785 / #53548, `fc15f726eab` on 2026-01-12) landed *after* v4.1.0 was tagged, and the branch-4.1 cherry-pick (`a39c1b8e5e2`) shipped first in v4.1.2. No 4.1.0/4.1.1 release contains these functions.
`4.1.2` is the earliest stable release in which they are available, so that is the correct value across all three annotation sites.
### Does this PR introduce _any_ user-facing change?
Documentation-only change. The rendered Python API ref, Scaladoc, and SQL function reference currently render "New in version 4.1.0" / "Since: 4.1.0", which is misleading; this PR corrects all of them to `4.1.2`.
### How was this patch tested?
Doc/annotation-only change. Manually verified each touched site is one of the 15 `kll_merge_agg_*` overload Scaladocs, the 3 KllMergeAgg expression descriptions, or the 3 Python docstrings — sibling annotations on `kll_sketch_agg_*` (which legitimately shipped in v4.1.0) are left unchanged.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (model: claude-opus-4-7)
Closes#56135 from zhengruifeng/spark-doc-fixes-dev1.
Authored-by: Ruifeng Zheng <ruifengz@apache.org>
Signed-off-by: Ruifeng Zheng <ruifengz@foxmail.com>
(cherry picked from commit 63f9c88)
Signed-off-by: Ruifeng Zheng <ruifengz@foxmail.com>
zhengruifeng added a commit that referenced this pull request May 28, 2026
…_agg_* (4.1.0 -> 4.1.2)
### What changes were proposed in this pull request?
Fix the `versionadded` / `since` / `ExpressionDescription.since` annotation on `kll_merge_agg_{bigint,float,double}`, changing `4.1.0` to `4.1.2`. Touches three files:
- `python/pyspark/sql/functions/builtin.py` — 3 `.. versionadded::` docstrings
- `sql/api/src/main/scala/org/apache/spark/sql/functions.scala` — 15 `since` Scaladocs (5 overloads × 3 types)
- `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/kllAggregates.scala` — 3 `ExpressionDescription(since = ...)` annotations
### Why are the changes needed?
All three places currently claim "Since 4.1.0", but:
| Tag | Date | Contains `kll_merge_agg_*`? |
|---|---|---|
| v4.1.0 | 2025-12-11 | No |
| v4.1.1 | 2026-01-02 | No |
| v4.1.2 | 2026-05-16 | **Yes** (via cherry-pick `a39c1b8e5e2`) |
| v4.2.0-preview2 | 2026-02-05 | Yes (via master commit `fc15f726eab`) |
The introducing commit on master (SPARK-54785 / #53548, `fc15f726eab` on 2026-01-12) landed *after* v4.1.0 was tagged, and the branch-4.1 cherry-pick (`a39c1b8e5e2`) shipped first in v4.1.2. No 4.1.0/4.1.1 release contains these functions.
`4.1.2` is the earliest stable release in which they are available, so that is the correct value across all three annotation sites.
### Does this PR introduce _any_ user-facing change?
Documentation-only change. The rendered Python API ref, Scaladoc, and SQL function reference currently render "New in version 4.1.0" / "Since: 4.1.0", which is misleading; this PR corrects all of them to `4.1.2`.
### How was this patch tested?
Doc/annotation-only change. Manually verified each touched site is one of the 15 `kll_merge_agg_*` overload Scaladocs, the 3 KllMergeAgg expression descriptions, or the 3 Python docstrings — sibling annotations on `kll_sketch_agg_*` (which legitimately shipped in v4.1.0) are left unchanged.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (model: claude-opus-4-7)
Closes#56135 from zhengruifeng/spark-doc-fixes-dev1.
Authored-by: Ruifeng Zheng <ruifengz@apache.org>
Signed-off-by: Ruifeng Zheng <ruifengz@foxmail.com>
(cherry picked from commit 63f9c88)
Signed-off-by: Ruifeng Zheng <ruifengz@foxmail.com>
zhengruifeng added a commit that referenced this pull request May 28, 2026
…_agg_* (4.1.0 -> 4.1.2)
### What changes were proposed in this pull request?
Fix the `versionadded` / `since` / `ExpressionDescription.since` annotation on `kll_merge_agg_{bigint,float,double}`, changing `4.1.0` to `4.1.2`. Touches three files:
- `python/pyspark/sql/functions/builtin.py` — 3 `.. versionadded::` docstrings
- `sql/api/src/main/scala/org/apache/spark/sql/functions.scala` — 15 `since` Scaladocs (5 overloads × 3 types)
- `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/kllAggregates.scala` — 3 `ExpressionDescription(since = ...)` annotations
### Why are the changes needed?
All three places currently claim "Since 4.1.0", but:
| Tag | Date | Contains `kll_merge_agg_*`? |
|---|---|---|
| v4.1.0 | 2025-12-11 | No |
| v4.1.1 | 2026-01-02 | No |
| v4.1.2 | 2026-05-16 | **Yes** (via cherry-pick `a39c1b8e5e2`) |
| v4.2.0-preview2 | 2026-02-05 | Yes (via master commit `fc15f726eab`) |
The introducing commit on master (SPARK-54785 / #53548, `fc15f726eab` on 2026-01-12) landed *after* v4.1.0 was tagged, and the branch-4.1 cherry-pick (`a39c1b8e5e2`) shipped first in v4.1.2. No 4.1.0/4.1.1 release contains these functions.
`4.1.2` is the earliest stable release in which they are available, so that is the correct value across all three annotation sites.
### Does this PR introduce _any_ user-facing change?
Documentation-only change. The rendered Python API ref, Scaladoc, and SQL function reference currently render "New in version 4.1.0" / "Since: 4.1.0", which is misleading; this PR corrects all of them to `4.1.2`.
### How was this patch tested?
Doc/annotation-only change. Manually verified each touched site is one of the 15 `kll_merge_agg_*` overload Scaladocs, the 3 KllMergeAgg expression descriptions, or the 3 Python docstrings — sibling annotations on `kll_sketch_agg_*` (which legitimately shipped in v4.1.0) are left unchanged.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (model: claude-opus-4-7)
Closes#56135 from zhengruifeng/spark-doc-fixes-dev1.
Authored-by: Ruifeng Zheng <ruifengz@apache.org>
Signed-off-by: Ruifeng Zheng <ruifengz@foxmail.com>
(cherry picked from commit 63f9c88)
Signed-off-by: Ruifeng Zheng <ruifengz@foxmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cboumalh@dtenedor@cloud-fan@zhengruifeng