Tuple sketch SQL support #13887

frankgrimes97 · 2023-03-06T21:35:11Z

This PR is a follow-up to #13819 so that the Tuple sketch functionality can be used in SQL for both ingestion using Multi-Stage Queries (MSQ) and also for analytic queries against Tuple sketch columns.

Release note

Add SQL functions for creating and operating on Tuple sketches

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

Based on workaround found here: mapstruct/mapstruct#1241

Includes tests which exercise the use-case outlined on: apache#13819

docs/querying/sql-functions.md

docs/querying/sql-aggregations.md

...apache/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregator.java

Also removed unused constant.

anshu-makkar · 2023-03-07T12:47:33Z

@abhishekagarwal87 kindly check this

docs/querying/sql-functions.md

frankgrimes97 · 2023-03-09T14:43:06Z

Hmm... not sure if there's anything to be done about this failing CI check:

> spellcheck
[69](https://github.com/apache/druid/actions/runs/4358587476/jobs/7648241472#step:21:70)
> mdspell --en-us --ignore-numbers --report '../docs/**/*.md' || (./script/notify-spellcheck-issues && false)
[70](https://github.com/apache/druid/actions/runs/4358587476/jobs/7648241472#step:21:71)

[71](https://github.com/apache/druid/actions/runs/4358587476/jobs/7648241472#step:21:72)
    ../docs/querying/sql-functions.md
[72](https://github.com/apache/druid/actions/runs/4358587476/jobs/7648241472#step:21:73)
      139 | ## ARRAY_OF_DOUBLES_SKETCH 
[73](https://github.com/apache/druid/actions/runs/4358587476/jobs/7648241472#step:21:74)
      149 | ## ARRAY_OF_DOUBLES_SKETCH_METRICS_SUM_ESTIMATE

https://github.com/apache/druid/actions/runs/4358587476/jobs/7648241472

frankgrimes97 · 2023-03-13T15:32:57Z

@vtlim Hi, are there any further changes required before this PR can be accepted and merged? Is a review by more maintainers required? Thanks!

vtlim · 2023-03-13T20:59:17Z

@frankgrimes97 Can you add the two new sketch aggregator names to the .spelling file? The tests catch the new names as a misspelling so we just need to add them to the dictionary. For example after L735 in https://github.com/apache/druid/blob/master/website/.spelling#L735. Add a line for ARRAY_OF_DOUBLES_SKETCH and one for ARRAY_OF_DOUBLES_SKETCH_METRICS_SUM_ESTIMATE

I only reviewed the docs side, so there will need to be a reviewer on the code side. cc @abhishekagarwal87 or @rohangarg

docs/querying/sql-functions.md

docs/querying/sql-aggregations.md

docs/querying/sql-functions.md

...g/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchMergeAggregator.java

.../query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSetBaseOperatorConversion.java

This avoids unnecessary changes to ArrayOfDoublesSketchMergeAggregator

...apache/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregator.java

frankgrimes97 · 2023-03-17T14:17:29Z

Hi, can this now be merged or are more changes required? Thanks!

docs/querying/sql-functions.md

paul-rogers

Cool functionality! Left a number of comments on how best to integrate the functionality into SQL, given the way that SQL normally works.

docs/querying/sql-aggregations.md

docs/querying/sql-functions.md

...apache/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregator.java

...he/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregatorTest.java

…tch-sql-rebased-on-pr-13819

Some APIs were recently deprecated as part of: apache#13904 apache#13914 Some CI checks were warning about this so better to address it now instead of needing to do so in a subsequent PR.

- Add missing INTERSECT/NOT/UNION descriptions - Re-order new functions alphabetically in documentation

gianm

The changes generally look good to me. I'd consider changing the names to start with TUPLE_DOUBLES_ rather than ARRAY_OF_DOUBLES_SKETCH_. (Like TUPLE_DOUBLES_SKETCH, TUPLE_DOUBLES_NOT, etc.) It's a bit less typing and IMO clearer.

Curious what people think.

frankgrimes97 · 2023-03-21T12:56:03Z

The changes generally look good to me. I'd consider changing the names to start with TUPLE_DOUBLES_ rather than ARRAY_OF_DOUBLES_SKETCH_. (Like TUPLE_DOUBLES_SKETCH, TUPLE_DOUBLES_NOT, etc.) It's a bit less typing and IMO clearer.

Curious what people think.

One issue with changing the naming only for the SQL functions is that it would be inconsistent with the naming in the native Druid functions and Apache Data Sketches codebase/documentation and so might lead to confusion.
That issue aside, I do think the naming can be more terse without losing too much meaning.

It might be worth noting that the naming of the Data Sketch SQL functions doesn't seem entirely consistent across the board.
e.g. I see other sketch SQL functions use a DS prefix or suffix to designate them as Data Sketches:

APPROX_COUNT_DISTINCT_DS_HLL
APPROX_COUNT_DISTINCT_DS_THETA
APPROX_QUANTILE_DS
DS_CDF
DS_GET_QUANTILE
DS_GET_QUANTILES
DS_HISTOGRAM
DS_HLL
DS_QUANTILE_SUMMARY
DS_QUANTILES_SKETCH
DS_RANK
DS_THETA

We could perhaps consider the following:

DS_TUPLE_DOUBLES
DS_TUPLE_DOUBLES_INTERSECT
DS_TUPLE_DOUBLES_METRICS_SUM_ESTIMATE
DS_TUPLE_DOUBLES_NOT
DS_TUPLE_DOUBLES_UNION

gianm · 2023-03-21T19:49:53Z

It might be worth noting that the naming of the Data Sketch SQL functions doesn't seem entirely consistent across the board.
e.g. I see other sketch SQL functions use a DS prefix or suffix to designate them as Data Sketches:

That's a good point about DS being used in most of the others. I think the suggestions at the end of your post (DS_TUPLE_DOUBLES, etc) are a good balance between consistency, clarity, and terseness. They work for me.

…tch-sql-rebased-on-pr-13819

ARRAY_OF_DOUBLES_SKETCH -> DS_TUPLE_DOUBLES ARRAY_OF_DOUBLES_SKETCH_* -> DS_TUPLE_DOUBLES_*

frankgrimes97 · 2023-03-21T21:53:25Z

@gianm @vtlim I've renamed the SQL functions and updated the documentation accordingly.

docs/querying/sql-functions.md

docs/querying/sql-aggregations.md

docs/querying/sql-functions.md

vtlim

LGTM for docs

gianm

LGTM after CI passes.

frankgrimes97 · 2023-03-28T12:46:58Z

LGTM after CI passes.

@gianm The failing checks seem unrelated to my changes.

abhishekagarwal87 · 2023-03-28T13:16:35Z

Indeed. I am merging this PR since failures are indeed unrelated.

frankgrimes97 added 3 commits March 6, 2023 16:32

Add NetBeans IDE files to .gitignore

a4f7bc7

Fix for NetBeans IDE JaCoCo/Surefire integration

e901836

Based on workaround found here: mapstruct/mapstruct#1241

Add Tuple Sketch SQL support

1ca0a96

Includes tests which exercise the use-case outlined on: apache#13819

github-actions bot added the Area - Documentation label Mar 6, 2023

vtlim reviewed Mar 6, 2023

View reviewed changes

docs/querying/sql-functions.md Outdated Show resolved Hide resolved

docs/querying/sql-functions.md Outdated Show resolved Hide resolved

docs/querying/sql-aggregations.md Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Mar 7, 2023

View reviewed changes

...apache/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregator.java Fixed Show fixed Hide fixed

frankgrimes97 added 2 commits March 7, 2023 07:24

Fix doc copy

692f7c0

Fix deprecation warning

c866501

Also removed unused constant.

frankgrimes97 added 2 commits March 7, 2023 10:35

Fix checkstyle errors

32a5b7a

Address some failing CI checks

cdeff24

vtlim reviewed Mar 7, 2023

View reviewed changes

docs/querying/sql-functions.md Outdated Show resolved Hide resolved

vtlim reviewed Mar 7, 2023

View reviewed changes

docs/querying/sql-functions.md Outdated Show resolved Hide resolved

More doc fixes

2bb0792

gianm reviewed Mar 13, 2023

View reviewed changes

frankgrimes97 added 4 commits March 13, 2023 17:38

Add new functions to .spelling file

1e4b4c9

Move ARRAY_OF_DOUBLES_SKETCH_METRICS_SUM_ESTIMATE docs to sql-scalar.md

185b6f6

Add Tuple Sketch INTERSECT, NOT and UNION SQL functions

fa100e9

Make use of COMPLEX_DECODE_BASE64

2a4b471

This avoids unnecessary changes to ArrayOfDoublesSketchMergeAggregator

github-advanced-security bot found potential problems Mar 15, 2023

View reviewed changes

...apache/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregator.java Fixed Show resolved Hide resolved

...apache/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregator.java Fixed Show resolved Hide resolved

frankgrimes97 requested a review from gianm March 16, 2023 17:50

vtlim reviewed Mar 17, 2023

View reviewed changes

docs/querying/sql-functions.md Outdated Show resolved Hide resolved

paul-rogers reviewed Mar 17, 2023

View reviewed changes

frankgrimes97 added 3 commits March 17, 2023 15:08

Merge remote-tracking branch 'upstream/master' into feature/tuple-ske…

54df44c

…tch-sql-rebased-on-pr-13819

Address new deprecation warnings.

4456358

Some APIs were recently deprecated as part of: apache#13904 apache#13914 Some CI checks were warning about this so better to address it now instead of needing to do so in a subsequent PR.

Address more documentation feedback

43db355

- Add missing INTERSECT/NOT/UNION descriptions - Re-order new functions alphabetically in documentation

gianm reviewed Mar 21, 2023

View reviewed changes

frankgrimes97 added 2 commits March 21, 2023 16:58

Merge remote-tracking branch 'upstream/master' into feature/tuple-ske…

e501a6f

…tch-sql-rebased-on-pr-13819

Rename ARRAY_OF_DOUBLES_SKETCH SQL functions

0a24150

ARRAY_OF_DOUBLES_SKETCH -> DS_TUPLE_DOUBLES ARRAY_OF_DOUBLES_SKETCH_* -> DS_TUPLE_DOUBLES_*

frankgrimes97 requested review from gianm and vtlim and removed request for gianm and vtlim March 21, 2023 21:37

vtlim reviewed Mar 22, 2023

View reviewed changes

docs/querying/sql-functions.md Show resolved Hide resolved

docs/querying/sql-aggregations.md Show resolved Hide resolved

docs/querying/sql-functions.md Outdated Show resolved Hide resolved

Address more documentation feedback

6037f71

vtlim approved these changes Mar 24, 2023

View reviewed changes

frankgrimes97 requested a review from gianm March 24, 2023 18:10

gianm approved these changes Mar 28, 2023

View reviewed changes

abhishekagarwal87 added Release Notes Area - SQL labels Mar 28, 2023

abhishekagarwal87 merged commit 2f98675 into apache:master Mar 28, 2023

clintropolis added this to the 26.0 milestone Apr 10, 2023

techdocsmith mentioned this pull request Apr 12, 2023

[DRAFT] 26.0.0 release notes #14064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tuple sketch SQL support #13887

Tuple sketch SQL support #13887

frankgrimes97 commented Mar 6, 2023

anshu-makkar commented Mar 7, 2023

frankgrimes97 commented Mar 9, 2023

frankgrimes97 commented Mar 13, 2023

vtlim commented Mar 13, 2023

frankgrimes97 commented Mar 17, 2023

paul-rogers left a comment

gianm left a comment

frankgrimes97 commented Mar 21, 2023

gianm commented Mar 21, 2023

frankgrimes97 commented Mar 21, 2023

vtlim left a comment

gianm left a comment

frankgrimes97 commented Mar 28, 2023 •

edited

Loading

abhishekagarwal87 commented Mar 28, 2023

Tuple sketch SQL support #13887

Tuple sketch SQL support #13887

Conversation

frankgrimes97 commented Mar 6, 2023

Release note

anshu-makkar commented Mar 7, 2023

frankgrimes97 commented Mar 9, 2023

frankgrimes97 commented Mar 13, 2023

vtlim commented Mar 13, 2023

frankgrimes97 commented Mar 17, 2023

paul-rogers left a comment

Choose a reason for hiding this comment

gianm left a comment

Choose a reason for hiding this comment

frankgrimes97 commented Mar 21, 2023

gianm commented Mar 21, 2023

frankgrimes97 commented Mar 21, 2023

vtlim left a comment

Choose a reason for hiding this comment

gianm left a comment

Choose a reason for hiding this comment

frankgrimes97 commented Mar 28, 2023 • edited Loading

abhishekagarwal87 commented Mar 28, 2023

frankgrimes97 commented Mar 28, 2023 •

edited

Loading