Refactor ColumnSelectorFactory; Rely on ColumnValueSelector's polymorphism by leventov · Pull Request #4886 · apache/druid

leventov · 2017-10-02T00:13:12Z

Fixes #4800. Removes a lot of long/float/double/object code repetition across the project.

…phism

…elector-factory

leventov · 2017-10-02T04:46:20Z

This PR fails on tests in SchemaEvolutionTest. According to them, numeric aggregation (e. g. longSum) on string dimension column should result to zeros, but currently with this PR it leads to exception. @himanshug @gianm what do you think it should do?

himanshug · 2017-10-02T15:28:56Z

@leventov IIRC, yes Druid tries to be as flexible as much possible and longSum works on string columns if they are parsable to long values or else assumed zeros. typically aggregators try to accommodate different column types if possible. i think we should try and preserve the behavior.

leventov · 2017-10-02T16:55:13Z

@himanshug in the long term/general, what do you think about Druid to be "weakly typed", a-la MySQL, vs "strongly typed", a-la PostgreSQL?

…Selector

leventov · 2017-10-02T17:21:52Z

#4888

gianm · 2017-10-02T19:19:05Z

@leventov, re:

This PR fails on tests in SchemaEvolutionTest. According to them, numeric aggregation (e. g. longSum) on string dimension column should result to zeros, but currently with this PR it leads to exception. @himanshug @gianm what do you think it should do?

I commented on #4888 (comment) in more detail but wanted to add here that SchemaEvolutionTest's purpose is to make sure you can change types of columns for newer segments and have the query system handle that elegantly. It isn't flawless right now (for example you would probably expect a "sum" aggregator of a string column to parse the strings, rather than return zero) but it works well for dimensions.

in the long term/general, what do you think about Druid to be "weakly typed", a-la MySQL, vs "strongly typed", a-la PostgreSQL?

I see it as being even weaker than MySQL on a table level. MySQL has a table-wide schema and I don't see Druid as ever requiring that.

leventov · 2017-10-05T21:51:53Z

Could someone please review this PR?

himanshug · 2017-10-05T22:00:18Z

i'll try and review it tomorrow.

leventov · 2017-10-05T22:01:11Z

@himanshug thank you

…elector-factory

himanshug · 2017-10-06T20:28:36Z

-      catch (Exception e) {
-        throw new ParseException(e, "Unable to parse metrics[%s], value[%s]", metric, metricValue);
+    if (metricValueString.charAt(0) == '+') {
+      if (metricValueString.length() > 1) {


could do if (metricValueString.length() > 1 && metricValueString.charAt(0) == '+') {.. and also eliminate the other if condition checking for metricValueString.isEmpty()

himanshug · 2017-10-06T20:35:56Z

+      }
+    }
+    return s;
+  }


why is this impl better than String.replace(char, '') ?

'' is impossible in Java :)

I meant using https://docs.oracle.com/javase/8/docs/api/java/lang/String.html#replace-char-char-

newChar is char, '' is not a char

ah... dint realize that. i'm surprised to know that there is no standard util to remove a character from a string :)

himanshug · 2017-10-06T20:43:04Z

-
-  @Nullable
-  ObjectColumnSelector makeObjectColumnSelector(String columnName);
+  ColumnValueSelector makeColumnValueSelector(String columnName);


this is definitely going to be incompatible for aggregator extensions. we need to mark ColumnSelectorFactory with @PublicApi

Do you have custom impls of ColumnSelectorFactory? This interface is not extended in any extensions in extensions-core or contrib

No-one is expected to extend ColumnSelectorFactory but it is used in custom extensions in implementations of AggregatorFactory.factorize/factorizedBuffered which would be calling removed methods like ColumnSelectorFactory.makeFloatColumnSelector(..) etc. That is why ColumnSelectorFactory needs to have @PublicApi and not @ExtensionPoint annotation.

Added @PublicApi to this interface and some others. So the next released version of Druid needs to be 0.12, I think.

himanshug · 2017-10-10T20:49:22Z

  private Union union;

-  public SketchAggregator(ObjectColumnSelector selector, int size)
+  public SketchAggregator(ColumnValueSelector selector, int size)


why is the type not BaseObjectColumnValueSelector as done in TimestampAggregator ?

himanshug · 2017-10-10T20:56:46Z

-      return new SketchAggregator(selector, size);
-    }
+    ColumnValueSelector selector = metricFactory.makeColumnValueSelector(fieldName);
+    return new SketchAggregator(selector, size);


null checks are removed, so in the new world it is guaranteed that for absent columns we get a non-null selector which would just return nulls/zeros from getXXX() methods ?
and I see you adjusted SketchAggregator to lazily create union so there shouldn't be any perf impacts there.

null checks are removed, so in the new world it is guaranteed that for absent columns we get a non-null selector which would just return nulls/zeros from getXXX() methods ?

Yes, added some javadoc about this

himanshug · 2017-10-10T21:13:18Z

-    ObjectColumnSelector selector = metricFactory.makeObjectColumnSelector(fieldName);
-    if (selector == null) {
+    ColumnValueSelector<?> selector = metricFactory.makeColumnValueSelector(fieldName);
+    if (selector instanceof NilColumnValueSelector) {


so, this is the new way for checking absent column ?

himanshug · 2017-10-10T21:29:52Z

+  @Override
+  public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+  {
+    // Usually AggregateCombiner has nothing to inspect


why is this not the default impl in AggregateCombiner then ?

himanshug · 2017-10-11T21:01:49Z

  {
-    throw new UnsupportedOperationException("DimensionSelector cannot be operated as numeric ColumnValueSelector");
+    // This is controversial, see https://github.com/druid-io/druid/issues/4888
+    return 0.0f;


i thought we were gonna try and parse if possible.

I preserved current behaviour. We can try to parse in a separate PR. Actually I tried to change this to parsing and it broke existing tests in SchemaEvolutionTest

sounds good then

himanshug · 2017-10-11T21:01:57Z

  {
-    throw new UnsupportedOperationException("DimensionSelector cannot be operated as numeric ColumnValueSelector");
+    // This is controversial, see https://github.com/druid-io/druid/issues/4888
+    return 0.0;


himanshug · 2017-10-11T21:02:03Z

-  {
-    throw new UnsupportedOperationException("DimensionSelector cannot be operated as object ColumnValueSelector");
+    // This is controversial, see https://github.com/druid-io/druid/issues/4888
+    return 0L;


…elector-factory

leventov · 2017-10-13T14:39:43Z

@himanshug addressed comments

…elector-factory

himanshug · 2017-10-14T02:45:17Z

@leventov given that this is incompatible and we haven't released 0.11.0 , is it possible to backport this to 0.11.0 ?

leventov · 2017-10-14T02:46:30Z

@himanshug thanks for review and merge, but this PR is tagged Design Review so it needed another one review from a committer

leventov · 2017-10-14T02:48:30Z

Anyway IMO it's too big and risky for backport at this point. I like the idea of making the next version 0.12 more, moreover I'm pretty sure there going to be more PRs that would break compatibility

leventov · 2017-10-14T02:49:23Z

E. g. somebody may want to implement #4888

himanshug · 2017-10-16T15:44:55Z

ok, next release gets to be 0.12.0 then. My apologies, I dint realize that you tagged it for additional reviews.

Refactor ColumnSelectorFactory; Rely on ColumnValueSelector's polymor…

fc1b919

…phism

leventov added Design Review Development Blocker Improvement WIP labels Oct 2, 2017

leventov added 4 commits October 1, 2017 19:14

Merge remote-tracking branch 'upstream/master' into refactor-column-s…

368ac3c

…elector-factory

Fix MapVirtualColumn.makeColumnValueSelector()

31226c9

Minor fixes

c843943

Fix IndexGeneratorCombinerTest

58a7fb3

DimensionSelector to return zeros when treated as numeric ColumnValue…

6609d15

…Selector

leventov added 2 commits October 2, 2017 13:34

Fix IncrementalIndexTest

eefca44

Fix IncrementalIndex.makeColumnSelectorFactory()

b8992dc

leventov added 3 commits October 2, 2017 14:39

Optimize MapBasedRow.getMetric()

a7937e3

Fix VarianceAggregatorTest

e2b6186

Simplify IncrementalIndex.makeColumnSelectorFactory()

9d544c2

leventov removed the WIP label Oct 2, 2017

leventov mentioned this pull request Oct 5, 2017

Make NULL handling in Druid more compatible to SQL standard #4754

Closed

Merge remote-tracking branch 'upstream/master' into refactor-column-s…

2bb6ac7

…elector-factory

himanshug reviewed Oct 11, 2017

View reviewed changes

leventov added 3 commits October 12, 2017 19:06

Merge remote-tracking branch 'upstream/master' into refactor-column-s…

aa1b9e6

…elector-factory

Address comments

4190afd

More comments

09505be

Test

7e3d267

Merge remote-tracking branch 'upstream/master' into refactor-column-s…

a494558

…elector-factory

himanshug merged commit dc7cb11 into apache:master Oct 14, 2017

himanshug added the Incompatible label Oct 14, 2017

leventov deleted the refactor-column-selector-factory branch October 18, 2017 18:24

leventov mentioned this pull request Dec 5, 2017

Bump Druid version to 0.12.0 #5138

Merged

gianm added this to the 0.12.0 milestone Jan 4, 2018

jon-wei mentioned this pull request Jan 5, 2018

[WIP] Druid 0.12.0 release notes #5211

Closed

leventov mentioned this pull request Jan 7, 2018

New ColumnSelectorFactory affects downstream project #5228

Closed

Conversation

leventov commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leventov commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

himanshug commented Oct 2, 2017

Uh oh!

leventov commented Oct 2, 2017

Uh oh!

leventov commented Oct 2, 2017

Uh oh!

gianm commented Oct 2, 2017

Uh oh!

leventov commented Oct 5, 2017

Uh oh!

himanshug commented Oct 5, 2017

Uh oh!

leventov commented Oct 5, 2017

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!

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!

leventov commented Oct 13, 2017

Uh oh!

himanshug commented Oct 14, 2017

Uh oh!

leventov commented Oct 14, 2017

Uh oh!

leventov commented Oct 14, 2017

Uh oh!

leventov commented Oct 14, 2017

Uh oh!

himanshug commented Oct 16, 2017

leventov commented Oct 2, 2017 •

edited

Loading

leventov commented Oct 2, 2017 •

edited

Loading