Vectorize the numeric part of Earliest Aggregator by somu-imply · Pull Request #12483 · apache/druid

somu-imply · 2022-04-25T23:54:48Z

Vectorizing the earliest aggregator

SqlExpressionBenchmark.querySql       42           5000000        false  avgt    5   86.661 ± 10.316  ms/op
SqlExpressionBenchmark.querySql       42           5000000        force  avgt    5   58.923 ±  9.156  ms/op
SqlExpressionBenchmark.querySql       43           5000000        false  avgt    5   76.705 ± 13.739  ms/op
SqlExpressionBenchmark.querySql       43           5000000        force  avgt    5   43.493 ±  7.034  ms/op
SqlExpressionBenchmark.querySql       44           5000000        false  avgt    5   75.060 ±  5.100  ms/op
SqlExpressionBenchmark.querySql       44           5000000        force  avgt    5   38.841 ±  1.883  ms/op
SqlExpressionBenchmark.querySql       45           5000000        false  avgt    5  189.588 ±  6.205  ms/op
SqlExpressionBenchmark.querySql       45           5000000        force  avgt    5   62.001 ±  9.902  ms/op

where

42 --> long
43 --> float
44 --> double
45 --> all three

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
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.

…_earliest

TSFenwick · 2022-04-29T17:04:32Z

...essing/src/main/java/org/apache/druid/query/aggregation/first/LongFirstVectorAggregator.java

+ */
+public class LongFirstVectorAggregator extends NumericFirstVectorAggregator
+{
+  long firstValue;


TSFenwick · 2022-04-29T19:17:51Z

...ing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java

+    final boolean[] nullValueVector = valueSelector.getNullVector();
+    boolean nullAbsent = false;
+    firstTime = buf.getLong(position);
+    //check if nullVector is found or not


nit: spacing on comments are inconsistent

TSFenwick · 2022-04-29T19:19:53Z

...ing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java

+  }
+
+  /**
+   *


would be good to have an overall description of the method be in the javadocs.

TSFenwick · 2022-04-29T19:19:58Z

...ing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java

+  }
+
+  /**
+   *


would be good to have an overall description of the method be in the javadocs.

suneet-s · 2022-04-29T22:05:04Z

...sing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstVectorAggregator.java

+
+/**
+ * Vectorized version of on heap 'last' aggregator for column selectors with type LONG..
+ */


stale javadocs. Similar comment for other VectorAggregator classes

suneet-s · 2022-04-29T22:16:49Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

  }

+  @Test
+  public void testPrimitiveEarliestInSubqueryGroupBy() throws Exception


This doesn't look like a subquery that is being tested

suneet-s

|   0% (0/7)     |   0% (0/4)     |  73% (11/15)   | org/apache/druid/query/aggregation/first/FloatFirstAggregatorFactory.java

Overall LGTM. This is the only thing that slightly concerns me. Can you look into why there's no coverage for changes to this class, while there is coverage for the other aggregator factories.

somu-imply · 2022-04-30T17:44:39Z

@suneet-s I have added additional tests that would cover the factory classes for all the three new additions. The coverage would improve now.

Element | class | method | body | branch
---------------------------------------------------------------------------------------
DoubleFirstAggregatorFactory | 100% (6/6) | 59% (22/37) | 71% (74/103) | 45% (20/44)
DoubleFirstVectorAggregator | 100% (1/1) | 100% (4/4) | 100% (10/10) | 50% (1/2)
FloatFirstAggregatorFactory | 100% (6/6) | 59% (22/37) | 71% (71/100) | 50% (20/40)
FloatFirstVectorAggregator | 100% (1/1) | 100% (4/4) | 100% (10/10) | 50% (1/2)
LongFirstAggregatorFactory | 100% (6/6) | 59% (22/37) | 69% (71/102) | 50% (20/40)
LongFirstVectorAggregator | 100% (1/1) | 100% (4/4) | 100% (10/10) | 50% (1/2)
NumericFirstVectorAggregator | 100% (1/1) | 75% (6/8) | 87% (43/49) | 63% (23/36)

@TSFenwick I have updated as per your review comments.

…_earliest

suneet-s · 2022-05-02T15:09:13Z

@somu-imply It looks like there are still some failing tests, can you fix them up please.

TimeseriesQueryRunnerTest.testTimeseriesWithFirstLastAggregator is one example

…ange

somu-imply · 2023-06-12T08:08:32Z

Instead of conflict resolution that's gonna make things difficult, I'll make a fresh PR will close this once the new one's up and linked to this.

And that is #14408

somu-imply added 2 commits April 25, 2022 12:58

Initial commit for vectorizing numeric first aggregator

e8a5d0e

Adding test cases and updating agg factory

cf46d4f

somu-imply changed the title ~~Numeric vectorize earliest~~ Vectorize the numeric part of Earliest Aggregator Apr 25, 2022

somu-imply added 3 commits April 25, 2022 18:29

Updating an unit testcase to be more meaningful

d1aac34

Merge remote-tracking branch 'upstream/master' into numeric_vectorize…

6591dc9

…_earliest

Adding benchmark tests

19a4beb

suneet-s added Performance Area - Querying labels Apr 27, 2022

TSFenwick reviewed Apr 29, 2022

View reviewed changes

suneet-s reviewed Apr 29, 2022

View reviewed changes

Updating tests and addressing comments

602f345

Merge remote-tracking branch 'upstream/master' into numeric_vectorize…

762c034

…_earliest

somu-imply added 2 commits May 2, 2022 08:49

Updating first agg

04277a1

Fixing testcases which were non vectorized before but now needs to ch…

366a92a

…ange

somu-imply closed this Jun 19, 2023

+                }
+                /**
+                 *

Comments

Conversation

somu-imply commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

somu-imply commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suneet-s commented May 2, 2022

Uh oh!

somu-imply commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

somu-imply commented Apr 25, 2022 •

edited

Loading

somu-imply commented Apr 30, 2022 •

edited

Loading

somu-imply commented Jun 12, 2023 •

edited

Loading