Skip to content

Vectorize SpectatorHistogram#18813

Merged
jtuglu1 merged 4 commits into
apache:masterfrom
jtuglu1:spectator-histogram-vectorize
Dec 10, 2025
Merged

Vectorize SpectatorHistogram#18813
jtuglu1 merged 4 commits into
apache:masterfrom
jtuglu1:spectator-histogram-vectorize

Conversation

@jtuglu1

@jtuglu1 jtuglu1 commented Dec 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Vectorizes SpectatorHistogram. Based off ApproximateHistogramVectorAggregator.

Performance

Need to do real-world testing (TODO), but benchmarks are promising (>10x speed-up):

Benchmark                                                    (metricName)  (rowsPerSegment)  (vectorize)  Mode  Cnt      Score   Error  Units
SpectatorHistogramAggregatorBenchmark.queryIncrementalIndex         long1           1000000        false  avgt    2  92451.646          us/op
SpectatorHistogramAggregatorBenchmark.queryIncrementalIndex         long1           1000000         true  avgt    2  73224.875          us/op
SpectatorHistogramAggregatorBenchmark.queryQueryableIndex           long1           1000000        false  avgt    2   3776.303          us/op
SpectatorHistogramAggregatorBenchmark.queryQueryableIndex           long1           1000000         true  avgt    2    202.916          us/op

I will remove the benchmarks before merging.

Other perf ideas

  • Possibly apply some fixed step loop unrolling to save even more instructions.

Release note

Key changed/added classes in this PR
  • MyFoo
  • OurBar
  • TheirBaz

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@jtuglu1
jtuglu1 marked this pull request as draft December 5, 2025 07:28
@jtuglu1

jtuglu1 commented Dec 5, 2025

Copy link
Copy Markdown
Contributor Author

Test failure is unrelated and being fixed in #18814

@jtuglu1
jtuglu1 force-pushed the spectator-histogram-vectorize branch 4 times, most recently from 48516d9 to 290afb0 Compare December 7, 2025 20:02
@jtuglu1
jtuglu1 marked this pull request as ready for review December 8, 2025 16:56
@jtuglu1
jtuglu1 force-pushed the spectator-histogram-vectorize branch 3 times, most recently from d8f7072 to 9cfc0de Compare December 8, 2025 18:11
@jtuglu1
jtuglu1 force-pushed the spectator-histogram-vectorize branch from 9cfc0de to 42df8f9 Compare December 8, 2025 18:16
@jtuglu1

jtuglu1 commented Dec 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@gianm @clintropolis do you know why many VectorAggregator implementations(2 examples below):

use the rows index to check whether the row being accessed is null, but ApproximateHistogramVectorAggregator does not index into this null vector with the rows index to fetch that information (and instead uses i directly)?.

in other words: using selector.getNullVector()[i] vs selector.getNullVector()[rows != null ? rows[i] : i]?

jasonk000
jasonk000 reviewed Dec 8, 2025
View reviewed changes

@jasonk000 jasonk000 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.

Looks great, albeit some smaller notes/tweaks. Appreciate inclusion of tests and JMH!

Comment thread benchmarks/src/test/java/org/apache/druid/benchmark/SpectatorHistogramAggregatorBenchmark.java Outdated
Comment thread benchmarks/src/test/java/org/apache/druid/benchmark/SpectatorHistogramAggregatorBenchmark.java Outdated
Comment thread ...am/src/main/java/org/apache/druid/spectator/histogram/SpectatorHistogramAggregateHelper.java Outdated
Comment thread ...am/src/main/java/org/apache/druid/spectator/histogram/SpectatorHistogramAggregateHelper.java
Comment thread ...am/src/main/java/org/apache/druid/spectator/histogram/SpectatorHistogramAggregateHelper.java
Comment thread ...am/src/main/java/org/apache/druid/spectator/histogram/SpectatorHistogramAggregateHelper.java
Comment thread ...java/org/apache/druid/spectator/histogram/SpectatorHistogramNumericVectorizedAggregator.java
Comment thread ...java/org/apache/druid/spectator/histogram/SpectatorHistogramNumericVectorizedAggregator.java
Comment thread ...rc/test/java/org/apache/druid/spectator/histogram/SpectatorHistogramAggregateHelperTest.java
Comment thread ...rc/test/java/org/apache/druid/spectator/histogram/SpectatorHistogramAggregateHelperTest.java
@jtuglu1

jtuglu1 commented Dec 8, 2025

Copy link
Copy Markdown
Contributor Author

@jasonk000 one note: I will not be leaving the JMH in the final PR (this is because benchmarks are intended to be Druid core pieces, and not extensions).

@jtuglu1
jtuglu1 requested review from jasonk000 and kfaraz December 9, 2025 07:21
@jtuglu1
jtuglu1 requested a review from bsyk December 10, 2025 04:14
@jtuglu1
jtuglu1 merged commit 7272d27 into apache:master Dec 10, 2025
32 checks passed
@kgyrtkirk kgyrtkirk added this to the 36.0.0 milestone Jan 19, 2026
@gianm

gianm commented Mar 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

@gianm @clintropolis do you know why many VectorAggregator implementations(2 examples below):

* [NullableNumericVectorAggregator](https://github.com/apache/druid/blob/-/processing/src/main/java/org/apache/druid/query/aggregation/NullableNumericVectorAggregator.java#L116-L117)

* [DoubleHllSketchBuildVectorProcessor](https://github.com/apache/druid/blob/master/extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/vector/DoubleHllSketchBuildVectorProcessor.java#L62-L66)

use the rows index to check whether the row being accessed is null, but ApproximateHistogramVectorAggregator does not index into this null vector with the rows index to fetch that information (and instead uses i directly)?.

in other words: using selector.getNullVector()[i] vs selector.getNullVector()[rows != null ? rows[i] : i]?

It looks like a bug in ApproximateHistogramVectorAggregator to me.

edit: fix in #19086

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

Reviewers

@jasonk000 jasonk000 jasonk000 approved these changes
@maytasm maytasm maytasm approved these changes
@clintropolis clintropolis Awaiting requested review from clintropolis
@gianm gianm Awaiting requested review from gianm
@kfaraz kfaraz Awaiting requested review from kfaraz
+1 more reviewer
@bsyk bsyk bsyk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area - Dependencies Performance

Projects

None yet

Milestone

36.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@jtuglu1 @gianm @jasonk000 @bsyk @maytasm @kgyrtkirk