Preemptive restriction for queries with approximate count distinct on complex columns of unsupported type by Akshat-Jain · Pull Request #16682 · apache/druid

Akshat-Jain · 2024-07-02T10:53:43Z

Description

Currently, we don't support count (distinct complexColumn) + approximation for unsupported types, and the query execution fails with a ClassCastException:

Caused by: java.lang.ClassCastException: class org.apache.druid.segment.nested.StructuredData cannot be cast to class org.apache.druid.hll.HyperLogLogCollector (org.apache.druid.segment.nested.StructuredData and org.apache.druid.hll.HyperLogLogCollector are in unnamed module of loader 'app')

This PR aims to check if the complex column being queried aligns with the supported types in the aggregator and aggregator factories, and throws a user-friendly error message if they don't.

The validation is added in 3 layers:

In the SQL operand type checker - This helps with the case of using APPROX_COUNT_DISTINCT() and other functions.
In the SQL layer - This helps with the case of doing select count (distinct column) type queries.
In the native layer - This helps with the case of doing native queries.

Test plan

Manually verified the functionality of APPROX_COUNT_DISTINCT, APPROX_COUNT_DISTINCT_DS_HLL, APPROX_COUNT_DISTINCT_DS_HLL_UTF8, APPROX_COUNT_DISTINCT_DS_THETA
Manually verified the functionality of select count (distinct column)
Manually verified the functionality of native queries.

Sample error message after this PR's changes in sql layer:

Sample error message after this PR's changes in native layer:

Release Note

Queries like SELECT COUNT(DISTINCT columnName) FROM tableName WHERE columnValue = 'non-existing-value' will also stop working with these changes.

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.

… complex columns of unsupported type

…tory as per offline discussion

…ctSqlAggregator: SQL validation layer + Native layer changes

…chApproxCountDistinctSqlAggregator

…umn)

…_DS_THETA

LakshSingla · 2024-07-08T05:03:42Z

Thanks for the PR @Akshat-Jain 💯 I have made a few suggestions - a few of which are applicable across the aggregators.

We should also add in the release notes that, according to your investigation, the queries like the following would fail if they were passing earlier:

SELECT COUNT(DISTINCT x) FROM table WHERE x = 'non-existing'

LakshSingla

Why is it that a HllSketchMergeAggregatorFactory doesn't have a check for arrays but Theta ones do?

Akshat-Jain · 2024-07-10T17:15:45Z

Why is it that a HllSketchMergeAggregatorFactory doesn't have a check for arrays but Theta ones do?

@LakshSingla I'm not sure about this, lacking context here. @clintropolis Can you chime in? Thanks!

LakshSingla · 2024-07-11T04:38:28Z

+    return Objects.equals(columnType.getComplexTypeName(), HyperUniquesAggregatorFactory.TYPE.getComplexTypeName()) ||
+           Objects.equals(columnType.getComplexTypeName(), HyperUniquesAggregatorFactory.PRECOMPUTED_TYPE.getComplexTypeName());


This doesn't allow UNKNOWN_COMPLEX types. Should it support them as well? It looks inconsistent given that other aggregators allow UNKNOWN_COMPLEX

I'm not sure on this. Will check with Clint / Zoltan on all the pending review comments where I'm lacking context to make a decision.

I'm a big fan of only allowing what's absolutely necessary.

kgyrtkirk · 2024-07-12T09:56:13Z

+  {
+    if (capabilities != null) {
+      final ColumnType type = capabilities.toColumnType();
+      if (!(ColumnType.UNKNOWN_COMPLEX.equals(type) || TYPE.equals(type) || PRECOMPUTED_TYPE.equals(type))) {


you push the negation into this conditional...or return early if they are ok...

but why allow everything in case capabilities == null ? does that cause any trouble? if its not I think its better to throw an error in that case as well....

but why allow everything in case capabilities == null ?

What should be the expectation when capabilities is null?

I think if capabilities is null an exception should be raised

@kgyrtkirk if (capabilities != null) {} seems to be used in a bunch of other layers as well, so seems like should be tackled separate from this PR?

makes sense to me

LakshSingla · 2024-07-16T15:17:58Z

Had taken a few passes over the changes. Need some validation on the Calcite and SQL side modifications that @kgyrtkirk has given.

kgyrtkirk · 2024-07-17T08:48:13Z

+    return Objects.equals(columnType.getComplexTypeName(), HyperUniquesAggregatorFactory.TYPE.getComplexTypeName()) ||
+           Objects.equals(columnType.getComplexTypeName(), HyperUniquesAggregatorFactory.PRECOMPUTED_TYPE.getComplexTypeName());


I'm a big fan of only allowing what's absolutely necessary.

kgyrtkirk · 2024-07-17T14:17:02Z

+  {
+    if (capabilities != null) {
+      final ColumnType type = capabilities.toColumnType();
+      if (!(ColumnType.UNKNOWN_COMPLEX.equals(type) || TYPE.equals(type) || PRECOMPUTED_TYPE.equals(type))) {


makes sense to me

LakshSingla · 2024-07-22T16:04:22Z

Thanks for the patch @Akshat-Jain

Akshat-Jain · 2024-07-22T16:05:42Z

Thanks for the exhaustive review cycles @LakshSingla and @kgyrtkirk! 🙌

… complex columns of unsupported type (apache#16682) This PR aims to check if the complex column being queried aligns with the supported types in the aggregator and aggregator factories, and throws a user-friendly error message if they don't.

Preemptive restriction for queries with approximate count distinct on…

a414096

… complex columns of unsupported type

github-actions Bot added the Area - Querying label Jul 2, 2024

Akshat-Jain added 8 commits July 3, 2024 15:38

Update ApproxCountDistinctSqlAggregator and HyperUniquesAggregatorFac…

90d8e16

…tory as per offline discussion

Fix error message in HyperUniquesAggregatorFactory

d5af441

Update HllSketchMergeAggregatorFactory and HllSketchApproxCountDistin…

6926ba2

…ctSqlAggregator: SQL validation layer + Native layer changes

Fix tests and cleanup for HllSketchMergeAggregatorFactory and HllSket…

f310f8f

…chApproxCountDistinctSqlAggregator

Add checks in HllSketchBaseSqlAggregator to handle count(distinct col…

c514d37

…umn)

SQL validation layer + Native layer changes for APPROX_COUNT_DISTINCT…

2796978

…_DS_THETA

Add tests

0f90ff5

Minor cleanup

c2213c8

Akshat-Jain marked this pull request as ready for review July 4, 2024 13:56

Akshat-Jain added 2 commits July 4, 2024 19:37

Minor cleanup

6c8a18e

Fix failing tests

acb3ad6

LakshSingla reviewed Jul 8, 2024

View reviewed changes

Address review comments

32df559

Akshat-Jain requested a review from LakshSingla July 8, 2024 15:28

LakshSingla reviewed Jul 10, 2024

View reviewed changes

Akshat-Jain added 2 commits July 10, 2024 22:34

Address review comments

2a398f6

Minor cleanup

7ce44ed

Akshat-Jain requested a review from LakshSingla July 10, 2024 17:40

Improve javadoc

9428d1e

LakshSingla reviewed Jul 11, 2024

View reviewed changes

Address review comments

bc183c8

kgyrtkirk reviewed Jul 12, 2024

View reviewed changes

Address review comments

76ab0e3

Akshat-Jain requested a review from kgyrtkirk July 15, 2024 09:08

kgyrtkirk reviewed Jul 17, 2024

View reviewed changes

Comment thread ...java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchBaseSqlAggregator.java Outdated

Address review comments

0aceb72

Akshat-Jain requested a review from kgyrtkirk July 17, 2024 12:23

kgyrtkirk reviewed Jul 17, 2024

View reviewed changes

Comment thread ...java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchBaseSqlAggregator.java Outdated

Address review comments

a0b6309

kgyrtkirk approved these changes Jul 17, 2024

View reviewed changes

Trigger Build

18e0de8

LakshSingla approved these changes Jul 22, 2024

View reviewed changes

LakshSingla merged commit 6a2348b into apache:master Jul 22, 2024

kfaraz added this to the 31.0.0 milestone Oct 4, 2024

kfaraz mentioned this pull request Oct 11, 2024

[DRAFT] 31.0.0 Release Notes #17332

Closed

		return Objects.equals(columnType.getComplexTypeName(), HyperUniquesAggregatorFactory.TYPE.getComplexTypeName()) \|\|
		Objects.equals(columnType.getComplexTypeName(), HyperUniquesAggregatorFactory.PRECOMPUTED_TYPE.getComplexTypeName());

Conversation

Akshat-Jain commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test plan

Release Note

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LakshSingla commented Jul 8, 2024

Uh oh!

LakshSingla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Akshat-Jain commented Jul 10, 2024

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

LakshSingla commented Jul 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LakshSingla commented Jul 22, 2024

Uh oh!

Akshat-Jain commented Jul 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Akshat-Jain commented Jul 2, 2024 •

edited

Loading