Defer more expressions in vectorized groupBy. by gianm · Pull Request #16338 · apache/druid

gianm · 2024-04-25T22:20:52Z

This patch adds a way for columns to provide GroupByVectorColumnSelectors, which controls how the groupBy engine operates on them. This mechanism is used by ExpressionVirtualColumn to provide an ExpressionDeferredGroupByVectorColumnSelector that uses the inputs of an expression as the grouping key. The actual expression evaluation is deferred until the grouped ResultRow is created.

A new context parameter deferExpressionDimensions allows users to control when this deferred selector is used. The default is fixedWidthNonNumeric, which is a behavioral change from the prior behavior. Users can get the prior behavior by setting this to singleString.

Benchmarks of a few selected queries from SqlExpressionBenchmark follow. Findings:

Query 26, GROUP BY CONCAT(string2, '-', long2), speeds up when the expression is deferred.
Queries 22, 24, 30, and 31 slow down when the expression is deferred. These are GROUP BY TIME_FLOOR(TIMESTAMPADD(DAY, -1, __time), GROUP BY long1 * long2, GROUP BY CAST(long1 as BOOLEAN) AND CAST (long2 as BOOLEAN), and GROUP BY long5 IS NULL, long3 IS NOT NULL. All are simple expressions with numeric inputs and outputs.

For these reasons, I think fixedWidthNonNumeric is a good default.

Benchmark                        (deferExpressionDimensions)  (query)  (rowsPerSegment)  (schema)  (vectorize)  Mode  Cnt     Score     Error  Units

SqlExpressionBenchmark.querySql                 singleString       22           5000000      auto        force  avgt    5   260.078 ±  14.858  ms/op
SqlExpressionBenchmark.querySql                   fixedWidth       22           5000000      auto        force  avgt    5  1970.522 ±  58.400  ms/op
SqlExpressionBenchmark.querySql         fixedWidthNonNumeric       22           5000000      auto        force  avgt    5   263.535 ±   5.549  ms/op
SqlExpressionBenchmark.querySql                       always       22           5000000      auto        force  avgt    5  2021.229 ± 125.010  ms/op

SqlExpressionBenchmark.querySql                 singleString       24           5000000      auto        force  avgt    5   624.300 ±  36.616  ms/op
SqlExpressionBenchmark.querySql                   fixedWidth       24           5000000      auto        force  avgt    5   889.836 ±  31.123  ms/op
SqlExpressionBenchmark.querySql         fixedWidthNonNumeric       24           5000000      auto        force  avgt    5   646.920 ±  24.566  ms/op
SqlExpressionBenchmark.querySql                       always       24           5000000      auto        force  avgt    5   890.384 ±  53.748  ms/op

SqlExpressionBenchmark.querySql                 singleString       26           5000000      auto        force  avgt    5   824.417 ±  21.941  ms/op
SqlExpressionBenchmark.querySql                   fixedWidth       26           5000000      auto        force  avgt    5   244.232 ±  15.514  ms/op
SqlExpressionBenchmark.querySql         fixedWidthNonNumeric       26           5000000      auto        force  avgt    5   244.598 ±  14.268  ms/op
SqlExpressionBenchmark.querySql                       always       26           5000000      auto        force  avgt    5   248.505 ±   8.004  ms/op

SqlExpressionBenchmark.querySql                 singleString       30           5000000      auto        force  avgt    5   223.687 ±   9.362  ms/op
SqlExpressionBenchmark.querySql                   fixedWidth       30           5000000      auto        force  avgt    5   562.844 ±  42.288  ms/op
SqlExpressionBenchmark.querySql         fixedWidthNonNumeric       30           5000000      auto        force  avgt    5   227.850 ±   3.374  ms/op
SqlExpressionBenchmark.querySql                       always       30           5000000      auto        force  avgt    5   562.631 ±  69.408  ms/op

SqlExpressionBenchmark.querySql                 singleString       31           5000000      auto        force  avgt    5   324.208 ±   9.420  ms/op
SqlExpressionBenchmark.querySql                   fixedWidth       31           5000000      auto        force  avgt    5  1271.630 ±  87.264  ms/op
SqlExpressionBenchmark.querySql         fixedWidthNonNumeric       31           5000000      auto        force  avgt    5   323.169 ±   6.383  ms/op
SqlExpressionBenchmark.querySql                       always       31           5000000      auto        force  avgt    5  1185.118 ±  34.146  ms/op

This patch adds a way for columns to provide GroupByVectorColumnSelectors, which controls how the groupBy engine operates on them. This mechanism is used by ExpressionVirtualColumn to provide an ExpressionDeferredGroupByVectorColumnSelector that uses the inputs of an expression as the grouping key. The actual expression evaluation is deferred until the grouped ResultRow is created. A new context parameter "deferExpressionDimensions" allows users to control when this deferred selector is used. The default is "fixedWidthNonNumeric", which is a behavioral change from the prior behavior. Users can get the prior behavior by setting this to "singleString".

+   */
+  @Nullable
+  default GroupByVectorColumnSelector makeGroupByVectorColumnSelector(
+      String columnName,


LakshSingla · 2024-05-10T05:25:05Z

There's also SqlGroupByBenchmark that benchmarks the code with various distributions and cardinalities. Maybe we should benchmark the code with the string columns and different parameters.

Are you suggesting adding a new @Benchmark method to SqlGroupByBenchmark that uses a SQL query with expressions?

clintropolis · 2024-06-17T22:49:16Z

+  {
+    this.expr = expr;
+    this.subSelectors = subSelectors;
+    this.exprKeyBytes = subSelectors.stream().mapToInt(GroupByVectorColumnSelector::getGroupingKeySize).sum();


nit: i think this should be the same size as exprInputSignature.size()? if true i think we should move this into that loop initializing the input bindings suppliers

clintropolis · 2024-06-17T22:52:35Z

-                  dimensionSpec ->
-                      ColumnProcessors.makeVectorProcessor(
+                  dimensionSpec -> {
+                    if (dimensionSpec instanceof DefaultDimensionSpec) {


is this just a sanity check since currently only DefaultDimensionSpec can vectorize, and ideally it will be the only one since DimensionSpec that do fancy stuff need to just be virtual columns.

It's a defensive move in case any DimensionSpec other than DefaultDimensionSpec are vectorizable in the future. I don't expect this to happen either, but it didn't sit right to write code that would be incorrect for other DimensionSpec types.

clintropolis · 2024-06-17T22:53:37Z

+        }
+      }
+
+      return true;


does this also need to check ColumnCapabilities.areDictionaryValuesUnique?

I don't think so, because non-unique dictionary (like one that maps both 0 and 1 to the same value foo) can still be used for deferred evaluation. The non-uniqueness doesn't hurt, because we will be doing another pass after this that fully groups things.

Here I'm assuming that isDictionaryEncoded is not going to be set for string columns that lack any kind of dictionary. Like, I'm assuming that the ones where DimensionSelector#getValueCardinality is -1 would have isDictionaryEncoded set to false (or at least unknown).

Here I'm assuming that isDictionaryEncoded is not going to be set for string columns that lack any kind of dictionary. Like, I'm assuming that the ones where DimensionSelector#getValueCardinality is -1 would have isDictionaryEncoded set to false (or at least unknown).

This is mainly what i was worried about when i wrote the comment, though looking closer at it i think its probably safe since https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionPlan.java#L279 seems like the only risky place and I believe setting that basically ensures we use the deferred single value selector, so 👍

clintropolis · 2024-06-17T22:58:39Z

+
+        allNumericInputs = allNumericInputs && capabilities.isNumeric();
+
+        if (!capabilities.isNumeric() && !capabilities.isDictionaryEncoded().isTrue()) {


should this check areDictionaryValuesUnique?

see #16338 (comment), i think it doesn't need to.

clintropolis · 2024-06-17T22:58:59Z

+          return false;
+        }
+
+        if (!capabilities.isNumeric() && !capabilities.isDictionaryEncoded().isTrue()) {


should this check areDictionaryValuesUnique? Technically it probably doesn't have to since i think the only things that set this to false while retaining isDictionaryEncoded as true are non-default dimension specs like extractionFn which are not 1:1, but also not supported for use with vectorization.

Also someday i'd like to have a cooler way to detect fixed width types that works for complex too, but that can wait for a future change.

see #16338 (comment), i think it doesn't need to.

github-actions Bot added Area - Documentation Area - Segment Format and Ser/De labels Apr 25, 2024

gianm added 2 commits April 25, 2024 15:30

Fix style.

c58e9a4

Add deferExpressionDimensions to SqlExpressionBenchmark.

0093d3b

gianm added the Release Notes label Apr 25, 2024

github-advanced-security AI found potential problems Apr 25, 2024

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/segment/VirtualColumn.java

*/

@Nullable

default GroupByVectorColumnSelector makeGroupByVectorColumnSelector(

String columnName,

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'columnName' is never used.

gianm added 4 commits April 26, 2024 08:36

Fix style.

eae00b4

Fix inspections.

261170c

Add more testing.

8657cbb

Use valueOrDefault.

3206f7a

LakshSingla reviewed May 10, 2024

View reviewed changes

clintropolis reviewed Jun 17, 2024

View reviewed changes

gianm added 2 commits June 26, 2024 10:11

Merge branch 'master' into deferred-expr-selector

95fc68b

Compute exprKeyBytes a bit lighter-weight.

cf7e311

clintropolis approved these changes Jun 27, 2024

View reviewed changes

gianm merged commit dbed1b0 into apache:master Jun 27, 2024

gianm deleted the deferred-expr-selector branch June 27, 2024 00:28

clintropolis mentioned this pull request Jun 27, 2024

fix vector grouping expression deferred evaluation to only consider dictionary encoded strings as fixed width #16666

Merged

gianm added the Performance label Jul 3, 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


		allNumericInputs = allNumericInputs && capabilities.isNumeric();

		if (!capabilities.isNumeric() && !capabilities.isDictionaryEncoded().isTrue()) {

Conversation

gianm commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Check notice

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gianm commented Apr 25, 2024 •

edited

Loading