ExpressionSelectors: Add optimized selectors. by gianm · Pull Request #5048 · apache/druid

gianm · 2017-11-06T23:30:10Z

Adds optimized selectors for certain cases.

SingleLongInputCaching selector for expressions on the __time column,
using a similar optimization to SingleScanTimeDimSelector
SingleStringInputDimensionSelector for expressions on string columns
that return strings, using a similar optimization to ExtractionFn
based DimensionSelectors.
SingleStringInputCaching selector for expressions on string columns
that return primitives.

Also, in the SQL planner, prefer expressions for time operations
rather than extractionFns.

Benchmark:

patch

Benchmark                                                  (rowsPerSegment)  Mode  Cnt   Score   Error  Units
ExpressionSelectorBenchmark.strlenUsingExpressionAsLong             1000000  avgt   30  16.996 ± 0.236  ms/op
ExpressionSelectorBenchmark.strlenUsingExpressionAsString           1000000  avgt   30  11.705 ± 0.161  ms/op
ExpressionSelectorBenchmark.strlenUsingExtractionFn                 1000000  avgt   30   7.018 ± 1.145  ms/op
ExpressionSelectorBenchmark.timeFloorUsingCursor                    1000000  avgt   30  20.294 ± 0.236  ms/op
ExpressionSelectorBenchmark.timeFloorUsingExpression                1000000  avgt   30  14.346 ± 0.237  ms/op
ExpressionSelectorBenchmark.timeFloorUsingExtractionFn              1000000  avgt   30  11.728 ± 0.247  ms/op

master

Benchmark                                                  (rowsPerSegment)  Mode  Cnt    Score   Error  Units
ExpressionSelectorBenchmark.strlenUsingExpressionAsLong             1000000  avgt   30  107.439 ± 2.332  ms/op
ExpressionSelectorBenchmark.strlenUsingExpressionAsString           1000000  avgt   30  132.240 ± 2.391  ms/op
ExpressionSelectorBenchmark.strlenUsingExtractionFn                 1000000  avgt   30    7.113 ± 1.274  ms/op
ExpressionSelectorBenchmark.timeFloorUsingCursor                    1000000  avgt   30   20.291 ± 0.379  ms/op
ExpressionSelectorBenchmark.timeFloorUsingExpression                1000000  avgt   30  115.135 ± 2.363  ms/op
ExpressionSelectorBenchmark.timeFloorUsingExtractionFn              1000000  avgt   30   11.292 ± 0.121  ms/op

- SingleLongInputCaching selector for expressions on the __time column, using a similar optimization to SingleScanTimeDimSelector - SingleStringInputDimensionSelector for expressions on string columns that return strings, using a similar optimization to ExtractionFn based DimensionSelectors. - SingleStringInputCaching selector for expressions on string columns that return primitives. Also, in the SQL planner, prefer expressions for time operations rather than extractionFns.

leventov · 2017-11-07T02:38:52Z

-        final ExprEval exprEval = baseSelector.getObject();
-        return exprEval.value();
+        // No need for null check on getObject() since baseSelector impls will never return null.
+        return baseSelector.getObject().value();


Could you please add //noinspection ... comment for IntelliJ?

Added //noinspection ConstantConditions

leventov · 2017-11-07T02:53:16Z

+  }
+
+  @VisibleForTesting
+  @Nonnull


Better to annotate the package with @EverythingIsNonnullByDefault instead

I started trying to do this but ran into a few too many things to change as a result (mostly to get rid of false suggestions of "remove null check, X cannot be null"). So I ended up leaving it as-is.

You don't have to resolve them, I think

It's true, but I didn't see much point in adding @EverythingIsNonnullByDefault without fixing the issues that causes with inspections, given that its whole point is to help with inspections in the first place.

leventov · 2017-11-07T02:57:33Z

+  @Nullable
+  static Supplier<Object> supplierFromObjectSelector(final BaseObjectColumnValueSelector<?> selector)
+  {
+    if (selector == null) {


ColumnSelectorFactory.makeColumnValueSelector() always returns non-null now, I think we shouldn't accept nulls here

Replaced this with selector instanceof NilColumnValueSelector since this seems to be how to do it now.

leventov · 2017-11-07T03:09:34Z

+    final Supplier<Object> inputSupplier = ExpressionSelectors.supplierFromDimensionSelector(selector);
+    this.bindings = name -> inputSupplier.get();
+
+    if (selector.getValueCardinality() <= CACHE_SIZE) {


Looks like it doesn't consider -1 which is "unknown" cardinality

There is a check for this a couple of lines up… cardinality >= 0 is a precondition for using this optimized implementation. I moved the check to be closer to this block so they are more visually connected. I also added a note to the javadoc about the precondition.

leventov · 2017-11-07T03:18:02Z

+
+    public ExprEval compute(final int id)
+    {
+      ExprEval value = m.getAndMoveToFirst(id);


For reference: vigna/fastutil#86

leventov · 2017-11-07T03:20:45Z

+  @Override
+  public IndexedInts getRow()
+  {
+    // Treat any non-single-valued row as a row containing a single null value, to ensure consistency with


Please make this comment a javadoc comment of the method and make ExpressionSelectors.supplierFromDimensionSelector a reference.

leventov · 2017-11-07T03:24:30Z

+    } else {
+      // Can't handle non-singly-valued rows in expressions.
+      // Treat them as nulls until we think of something better to do.
+      return new SingleIndexedInt(0);


Could be cached, SingleIndexedInt.of() and cache 0-127 :)

Implemented this.

leventov · 2017-11-07T03:25:43Z

+      if (nullAdjustment == 0) {
+        return row;
+      } else {
+        return new SingleIndexedInt(row.get(0) + nullAdjustment);


Could add MutableSingleIndexedInt and cache the object

I'm not sure if this offers a measurable benefit and I'm also not sure if the return value from getRow ever needs to persist beyond the next movement of the selector. So I left this as-is.

leventov · 2017-11-07T03:27:53Z

+  private final Expr expression;
+  private final SingleInputBindings bindings = new SingleInputBindings();
+
+  // 0 if selector has null as a value; 1 if it doesn't.


Please make a javadoc comment

gianm added Area - Querying Performance labels Nov 6, 2017

leventov reviewed Nov 7, 2017

View reviewed changes

Code review comments.

79e9f61

leventov approved these changes Nov 9, 2017

View reviewed changes

fjy merged commit 77df5e0 into apache:master Nov 14, 2017

jon-wei added this to the 0.12.0 milestone Jan 5, 2018

jon-wei mentioned this pull request Jan 5, 2018

[WIP] Druid 0.12.0 release notes #5211

Closed

gianm mentioned this pull request Nov 11, 2018

Expressions: Optimization for string expressions on the __time column. #5109

Closed

gianm deleted the expression-selector-optimize branch September 23, 2022 19:22

Conversation

gianm commented Nov 6, 2017 • 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!

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!

4 participants

gianm commented Nov 6, 2017 •

edited

Loading