Vectorized theta sketch aggregator + rework of VectorColumnProcessorFactory. #10767

gianm · 2021-01-15T06:57:42Z

Update 2: the patch has been expanded to pay down some debt related to how vector processors handle complex columns, in order to ensure that the theta agg behaves properly in all situations. (See comment below.)

Update: the patch has been reworked to use composition instead of inheritance, using the same technique from #10304 mentioned by @abhishekagarwal87.

Also a refactoring of BufferAggregator and VectorAggregator such that
they share a common interface, BaseBufferAggregator. This allows
implementing both in the same file with an abstract + dual subclass
structure.

Also a refactoring of BufferAggregator and VectorAggregator such that they share a common interface, BaseBufferAggregator. This allows implementing both in the same file with an abstract + dual subclass structure.

abhishekagarwal87 · 2021-01-15T07:46:25Z

By the way, I did a similar thing in #10304 to reduce some boilerplate code but used composition instead of inheritance. Added package-private*BufferAggregatorHelper classes which in turn are used by vector and buffer aggregator. The helper classes know how to work with buffers. The logic for getting an object out of selector and vectorization stuff was left out for Buffer and Vector classes.

gianm · 2021-01-15T09:10:57Z

By the way, I did a similar thing in #10304 to reduce some boilerplate code but used composition instead of inheritance. Added package-private*BufferAggregatorHelper classes which in turn are used by vector and buffer aggregator. The helper classes know how to work with buffers. The logic for getting an object out of selector and vectorization stuff was left out for Buffer and Vector classes.

Ah, thanks for pointing that out. I haven't had a chance to look at your patch yet. Do you think I should rework this patch to do the same thing you did? I suppose we don't want to have too many ways of doing the same thing in the codebase.

abhishekagarwal87 · 2021-01-15T09:19:15Z

Right. I was being subtle about it :) It would be nice to use the same style everywhere. If that pattern needs some modification, I can do that as well.

gianm · 2021-01-15T09:23:15Z

Which pattern do you think is better? I haven't had a chance to take a look at yours yet, so I'm wondering if I should make yours match mine, or mine match yours.

abhishekagarwal87 · 2021-01-15T10:48:22Z

I usually avoid abstract classes so there is no tight coupling. It can make unit testing easier as well. I can pass a mock helper into a vector aggregator and unit test in different ways easily. so my preference is toward the way ApproximateHistogram is implemented.

gianm · 2021-01-15T19:22:56Z

I've pushed up a new patch that undoes the refactoring and instead implements things using the same technique as #10304.

abhishekagarwal87

LGTM.

regular types. Involved finally moving makeVectorProcessor from DimensionHandlerUtils into ColumnProcessors and harmonizing the two things.

gianm · 2021-01-20T07:29:29Z

When debugging a test failure, I realized that some additional work is needed to make the aggregator able to handle both complex inputs (existing theta sketches) and regular inputs (strings, primitives). There's some debt that needed to be paid down:

VectorColumnProcessorFactory had to be taught about complex columns, instead of handling them all as all-null string columns
Existing users of VectorColumnProcessorFactory had to be taught about complex columns too
To support the above, makeVectorProcessor needed to be moved from DimensionHandlerUtils to ColumnProcessors and harmonized with the ColumnProcessor stuff

clintropolis

overall refactor lgtm, its nice to untangle vector stuff from DimensionHandlerUtils, i think you've got some style issues failing in CI

clintropolis · 2021-01-20T11:36:14Z

processing/src/main/java/org/apache/druid/segment/VectorColumnProcessorFactory.java

+  /**
+   * Called when {@link ColumnCapabilities#getType()} is COMPLEX.
+   */
+  T makeComplexProcessor(ColumnCapabilities capabilities, VectorObjectSelector selector);


Since this isn't actually specific to complex columns, can this be called makeObjectProcessor? I have the same method signature except with that name added to this class in #10613, there to support using object selectors for string expression virtual columns which are not dictionary encoded.

Sure, although I'll keep the javadoc the same for now, since as of this patch it's still only called when the type is COMPLEX. It seems that one of us will need to rework implementations a bit based on which patch gets merged first.

gianm · 2021-01-20T17:42:48Z

Pushed up a new patch.

clintropolis

+1 after CI (still some style/dependency/inspection issues)

The patch uses the same "helper" approach as apache#10767 and apache#10304, and extends the tests to run in both vectorized and non-vectorized modes. Also includes some minor changes to the theta sketch vector aggregator: - Cosmetic changes to make the hll and theta implementations look more similar. - Extends the theta SQL tests to run in vectorized mode.

* Vectorized versions of HllSketch aggregators. The patch uses the same "helper" approach as #10767 and #10304, and extends the tests to run in both vectorized and non-vectorized modes. Also includes some minor changes to the theta sketch vector aggregator: - Cosmetic changes to make the hll and theta implementations look more similar. - Extends the theta SQL tests to run in vectorized mode. * Updates post-code-review. * Fix javadoc.

@suneet-s

* Update security overview with additional recommendations (apache#11016) * updatee security overview with additional recommendations for improved security * address first set of review questions * Update docs/operations/security-overview.md * Update docs/operations/security-overview.md * apply changes from review * Update docs/operations/security-overview.md Co-authored-by: Suneet Saldanha <suneet@apache.org> * Update docs/operations/security-overview.md Co-authored-by: Suneet Saldanha <suneet@apache.org> * Update docs/operations/security-overview.md Co-authored-by: Suneet Saldanha <suneet@apache.org> * Update security-overview.md fix additional comments & typos cc: @suneet-s, @jihoonsoon Co-authored-by: Suneet Saldanha <suneet@apache.org> * Enable rewriting certain inner joins as filters. (apache#11068) * Enable rewriting certain inner joins as filters. The main logic for doing the rewrite is in JoinableFactoryWrapper's segmentMapFn method. The requirements are: - It must be an inner equi-join. - The right-hand columns referenced by the condition must not contain any duplicate values. (If they did, the inner join would not be guaranteed to return at most one row for each left-hand-side row.) - No columns from the right-hand side can be used by anything other than the join condition itself. HashJoinSegmentStorageAdapter is also modified to pass through to the base adapter (even allowing vectorization!) in the case where 100% of join clauses could be rewritten as filters. In support of this goal: - Add Query getRequiredColumns() method to help us figure out whether the right-hand side of a join datasource is being used or not. - Add JoinConditionAnalysis getRequiredColumns() method to help us figure out if the right-hand side of a join is being used by later join clauses acting on the same base. - Add Joinable getNonNullColumnValuesIfAllUnique method to enable retrieving the set of values that will form the "in" filter. - Add LookupExtractor canGetKeySet() and keySet() methods to support LookupJoinable in its efforts to implement the new Joinable method. - Add "enableRewriteJoinToFilter" feature flag to JoinFilterRewriteConfig. The default is disabled. * Test improvements. * Test fixes. * Avoid slow size() call. * Remove invalid test. * Fix style. * Fix mistaken default. * Small fixes. * Fix logic error. * Doc updates for union datasources. (apache#11103) The main one is updating datasources.md to talk about SQL. (It still said that table unions are not supported in SQL.) Also, this doc update adds some clarifying details on limitations. * [Security] Bump netty4.version from 4.1.48.Final to 4.1.63.Final (apache#11117) * Vectorized versions of HllSketch aggregators. (apache#11115) * Vectorized versions of HllSketch aggregators. The patch uses the same "helper" approach as apache#10767 and apache#10304, and extends the tests to run in both vectorized and non-vectorized modes. Also includes some minor changes to the theta sketch vector aggregator: - Cosmetic changes to make the hll and theta implementations look more similar. - Extends the theta SQL tests to run in vectorized mode. * Updates post-code-review. * Fix javadoc. * Web console: update dev dependencies (apache#11119) * Update some dev dependencies, prettify, tslint-fix * Sort tsconfig keys for easy comparison * Set noImplicitThis * Slightly more accurate types * Bump Jest and related * Bump react to latest on v16 * Bump node-sass, sass-loader for node14 support * Remove node-sass-chokidar (unused) * More unused dependencies * Fix blueprint imports * Webpack 5 * Update webpack config for 'process' usage * Update playwright-chromium * Emit esnext modules for tree shaking * Enable source maps in development * Dedupe * Bump babel and things * npm audit fix * Add .editorconfig file to match prettier settings * Update licenses (tslib is 0BSD as of 1.11.2) microsoft/tslib#96 * Require node >= 10 * Use Node 10 to run e2e tests * Use 'ws' transport mode for dev server (will be default in next version) * Remove an 'any' * No sourcemaps in prod * Exclude .editorconfig from license checks * Try nvm for setting node version Co-authored-by: Charles Smith <38529548+techdocsmith@users.noreply.github.com> Co-authored-by: Suneet Saldanha <suneet@apache.org> Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: Gian Merlino <gianmerlino@gmail.com> Co-authored-by: Sandeep <isandeep41@gmail.com> Co-authored-by: John Gozde <john@gozde.ca>

Vectorized theta sketch aggregator.

80d0a20

Also a refactoring of BufferAggregator and VectorAggregator such that they share a common interface, BaseBufferAggregator. This allows implementing both in the same file with an abstract + dual subclass structure.

gianm added Performance Area - Querying labels Jan 15, 2021

gianm added 2 commits January 15, 2021 11:08

Merge branch 'master' into theta-vectorize

67ce7ae

Rework implementation to use composition instead of inheritance.

86429a0

abhishekagarwal87 approved these changes Jan 16, 2021

View reviewed changes

gianm added 3 commits January 19, 2021 14:12

Merge branch 'master' into theta-vectorize

98fccd1

Rework things to enable working properly for both complex types and

f298cb7

regular types. Involved finally moving makeVectorProcessor from DimensionHandlerUtils into ColumnProcessors and harmonizing the two things.

Add missing method.

582c71c

gianm changed the title ~~Vectorized theta sketch aggregator.~~ Vectorized theta sketch aggregator + rework of VectorColumnProcessorFactory. Jan 20, 2021

clintropolis reviewed Jan 20, 2021

View reviewed changes

Style and name changes.

fee14af

clintropolis approved these changes Jan 26, 2021

View reviewed changes

gianm added 3 commits January 26, 2021 10:57

Merge branch 'master' into theta-vectorize

d91b4ac

Fix issues from inspections.

ff3ef2f

Fix style issue.

fd93498

gianm merged commit 6c0c6e6 into apache:master Jan 29, 2021

gianm deleted the theta-vectorize branch January 29, 2021 17:30

gianm mentioned this pull request Apr 14, 2021

Vectorized versions of HllSketch aggregators. #11115

Merged

clintropolis added this to the 0.22.0 milestone Aug 12, 2021

clintropolis mentioned this pull request Sep 3, 2021

[Draft] 0.22.0 Release Notes #11657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vectorized theta sketch aggregator + rework of VectorColumnProcessorFactory. #10767

Vectorized theta sketch aggregator + rework of VectorColumnProcessorFactory. #10767

gianm commented Jan 15, 2021 •

edited

Loading

abhishekagarwal87 commented Jan 15, 2021

gianm commented Jan 15, 2021

abhishekagarwal87 commented Jan 15, 2021

gianm commented Jan 15, 2021

abhishekagarwal87 commented Jan 15, 2021

gianm commented Jan 15, 2021

abhishekagarwal87 left a comment

gianm commented Jan 20, 2021

clintropolis left a comment

clintropolis Jan 20, 2021

gianm Jan 20, 2021

gianm commented Jan 20, 2021

clintropolis left a comment

Vectorized theta sketch aggregator + rework of VectorColumnProcessorFactory. #10767

Vectorized theta sketch aggregator + rework of VectorColumnProcessorFactory. #10767

Conversation

gianm commented Jan 15, 2021 • edited Loading

abhishekagarwal87 commented Jan 15, 2021

gianm commented Jan 15, 2021

abhishekagarwal87 commented Jan 15, 2021

gianm commented Jan 15, 2021

abhishekagarwal87 commented Jan 15, 2021

gianm commented Jan 15, 2021

abhishekagarwal87 left a comment

Choose a reason for hiding this comment

gianm commented Jan 20, 2021

clintropolis left a comment

Choose a reason for hiding this comment

clintropolis Jan 20, 2021

Choose a reason for hiding this comment

gianm Jan 20, 2021

Choose a reason for hiding this comment

gianm commented Jan 20, 2021

clintropolis left a comment

Choose a reason for hiding this comment

gianm commented Jan 15, 2021 •

edited

Loading