SQL support for t-digest based sketch aggregators by samarthjain · Pull Request #8100 · apache/druid

samarthjain · 2019-07-17T23:56:41Z

Description

This PR adds support for t-digest based sketch aggregators that was added in #7331

Additionally this PR removes previously added mergeTDigestSketch aggregator. The merging/combining functionality has been added in buildTDigestSketch aggregator. The docs also have been updated with the relevant changes.

Note that a couple of tests added in this PR will fail till #8099 is merged.

This PR has:

[X ] been self-reviewed.
- [X ] using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
[ X] added documentation for new or modified features or behaviors.
[ X] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
[X ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
[ X] added unit tests or modified existing tests to cover new code paths.
added integration tests.
[ X] been tested in a test Druid cluster.

For reviewers: the key changed/added classes in this PR are TDigestGenerateSketchSqlAggregator, and TDigestSketchQuantileSqlAggregator.

gianm · 2019-07-26T22:46:44Z

Hey @samarthjain, I see this error in CI,

Caused by: java.lang.ClassNotFoundException: org.apache.druid.query.expression.LookupEnabledTestExprMacroTable
	at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	... 28 more

You probably need this in your pom:

        <dependency>
            <groupId>org.apache.druid</groupId>
            <artifactId>druid-server</artifactId>
            <version>${project.parent.version}</version>
            <scope>test</scope>
        </dependency>

samarthjain · 2019-07-26T23:15:00Z

Hmm, I do have that dependency added.
https://github.com/apache/incubator-druid/blob/adeac28b1e774990ba5fad8a52e2d7d3abe7160c/extensions-contrib/tdigestsketch/pom.xml#L157

Taking a closer look.

gianm · 2019-07-26T23:18:19Z

Oh, actually I think you need this:

        <dependency>
            <groupId>org.apache.druid</groupId>
            <artifactId>druid-server</artifactId>
            <version>${project.parent.version}</version>
            <type>test-jar</type>
            <scope>test</scope>
        </dependency>

(The <type>test-jar</type> part in particular)

jon-wei · 2019-07-29T23:46:17Z

@samarthjain Can you fix conflicts?

jon-wei · 2019-07-30T00:09:29Z

Suggest changing "ingested_sketch" here to <input-column>, since the aggregator can accept raw values or pre-generated sketches

jon-wei · 2019-07-30T00:19:24Z

Suggest rewording this section, there's only one aggregator since the PostAggregators don't really aggregate (they only operate on values within a single row), they're more like "post aggregation transforms".

jon-wei · 2019-07-30T00:47:01Z

Suggest rewriting to:

"The input field reference, which must be a PostAggregator that outputs T-Digest sketches. This can be a fieldAccess PostAggregator, which simply returns the value of the referenced input column."

jon-wei · 2019-07-30T00:47:20Z

quantilesFromTDigestSketch -> quantileFromTDigestSketch

jon-wei · 2019-07-30T01:36:02Z

Since build/merge implementations are combined, suggest calling the type "tDigestSketch"

jon-wei · 2019-07-31T20:52:31Z

Since this is only one fraction, suggest changing the property name to "fraction"

jon-wei · 2019-07-31T21:00:13Z

maxNumEntriesOperand should be renamed to compressionOperand

jon-wei · 2019-07-31T21:04:28Z

Since the compression operand is optional, there should be a check that aggregateCall.getArgList() has size > 1

jon-wei · 2019-07-31T21:13:06Z

The agg function here should be adjusted to support the optional compression param like in the quantile version

jon-wei · 2019-07-31T21:17:34Z

This should check that the compression and quantile parameters match as well

Hmm, not sure how to do that. Compression is part of the TDigestSketchAggregatorFactory and quantile isn't. The quantile is used in postAggregator.

Ah, it should just check compression

jon-wei · 2019-07-31T21:18:04Z

Can you update the docs to mention that optional compression param is supported for the quantile SQL function as well

So I thought about this a bit more and I am not sure what is the right thing to do. Consider a query like this:

SELECT TDIGEST_QUANTILE(column1, 0.1, 100), TDIGEST_QUANTILE(column1, 0.2, 200), TDIGEST_QUANTILE(column1, 0.2, 300) FROM FOO

This query does the aggregation to generate a sketch on column1 and then applies the post aggregator to compute quantile out of it (3 times). The problem is that the sketch is generated only once using the compression param 100 (from the first TDIGEST_QUANTILE(column1, 0.1, 100) and the compression params from the following calls are ignored.

Is there a way to validate in Druid SQL that all the calls on TDIGEST_QUANTILE() for a column are using the same compression param? The other alternative is to not support compression param for this method and just use the default value of compression.

The problem is that the sketch is generated only once using the compression param 100 (from the first TDIGEST_QUANTILE(column1, 0.1, 100) and the compression params from the following calls are ignored.

If you add a check that the compression parameter matches before reusing the
aggregator (#8100 (comment)), does this still occur?

Thanks, that worked.

jon-wei

lgtm after comments are addressed

jon-wei · 2019-07-31T21:27:02Z

Suggest adding tests with a quantile SQL agg that has the compression parameter is specified, and where the sketch build SQL agg uses default compression

jon-wei · 2019-07-31T21:28:27Z

Since SKETCH_TO_QUANTILES and SKETCH_TO_QUANTILE are postaggs, their IDs should go in PostAggregatorIds instead

clintropolis added Area - SQL Release Notes labels Jul 18, 2019

gianm requested a review from jon-wei July 19, 2019 20:53

samarthjain force-pushed the tdigestsqlsupport branch 2 times, most recently from bdbe581 to adeac28 Compare July 26, 2019 20:56

jon-wei reviewed Jul 30, 2019

View reviewed changes

samarthjain force-pushed the tdigestsqlsupport branch from 594658d to 86546e6 Compare July 30, 2019 20:31

jon-wei reviewed Jul 31, 2019

View reviewed changes

samarthjain added 6 commits August 2, 2019 11:22

SQL support for t-digest based sketch aggregators

7f42f84

Fix teamcity errors

4184db0

Add missing dependencies

bd643b9

Remove unused dependency

4b9caac

Address code review comments

c54bb5a

Add checks for compression param

d0d9043

samarthjain force-pushed the tdigestsqlsupport branch from 52891e5 to d0d9043 Compare August 2, 2019 18:23

jon-wei approved these changes Aug 5, 2019

View reviewed changes

jon-wei merged commit 93cf9d4 into apache:master Aug 5, 2019

jihoonson mentioned this pull request Apr 6, 2020

Unknown complex types break Druid SQL #9421

Closed

Conversation

samarthjain commented Jul 17, 2019

Description

Uh oh!

gianm commented Jul 26, 2019

Uh oh!

samarthjain commented Jul 26, 2019

Uh oh!

gianm commented Jul 26, 2019

Uh oh!

jon-wei commented Jul 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants