Remove makeMathExpressionSelector from ColumnSelectorFactory. by gianm · Pull Request #3815 · apache/druid

gianm · 2017-01-03T23:48:38Z

Half of the second part of https://groups.google.com/forum/#!msg/druid-development/_Sd78s7yU5U/RJ8qLOJ5DwAJ, skipping the aggregator api changes.

leventov · 2017-01-04T11:08:14Z

+        // Unknown ValueType. Try making an Object selector and see if that gives us anything useful.
+        final ObjectColumnSelector selector = columnSelectorFactory.makeObjectColumnSelector(columnName);
+        final Class clazz = selector == null ? null : selector.classOfObject();
+        if (selector == null || (clazz != Object.class && Number.class.isAssignableFrom(clazz))) {


Shouldn't it be !Number.class.isAssignableFrom(clazz)?

Oops, you're right, will fix.

leventov · 2017-01-04T11:11:08Z

-    };
-  }
-
  @Override


Could you please label this method and the interface method @Nullable?

leventov · 2017-01-04T11:14:42Z

+      final Supplier<Number> supplier;
+
+      if (nativeType == ValueType.FLOAT) {
+        final FloatColumnSelector selector = columnSelectorFactory.makeFloatColumnSelector(columnName);


Do you think FloatColumnSelector could just extend Supplier<Float> and then "unsafe" cast is done here instead of wrapping?

(This also requires renaming of float get() to avoid name clash)

It could but I'm not sure that change is warranted.

Is your concern performance or is it the amount of code in this method? If it's the latter then how about adding a method like FloatColumnSelector.asSupplier(floatColumnSelector)?

If it's performance, I guess I'm not super worried about performance in the Expressions related code since it's already doing a lot of sub-optimal stuff anyway (like interpreting expressions on top of boxed numbers). Changing that might involve no longer using Suppliers anyway.

It was about performance, but given that proper optimization might involve moving away from Supplier<Number>, the change that I suggested will become pointless, so let's not make it.

leventov · 2017-01-04T11:15:32Z

+          }
+        };
+      } else if (nativeType == ValueType.LONG) {
+        final LongColumnSelector selector = columnSelectorFactory.makeLongColumnSelector(columnName);


Same question as about FloatColumnSelector a few lines above

leventov · 2017-01-04T11:25:24Z

+    return new ExpressionObjectSelector(createBindings(columnSelectorFactory, expression), expression);
+  }
+
+  private static Expr.ObjectBinding createBindings(ColumnSelectorFactory columnSelectorFactory, Expr expression)


Consider splitting this 50-line method into some smaller methods

It doesn't feel to me like there's much going on in this method; there are a lot of lines but most of them aren't doing much. I guess I could break out the nativeType == null branch into a method like createNumberSupplierFromObjectColumnSelector but I'm not sure that would actually make it easier to understand.

Do you think it would?

I think it would be easier to understand this method if all three blocks (float, long, object) are extracted as static methods

All right, I'll trust your opinion on readability more than mine since you didn't write the code.

gianm · 2017-01-04T16:59:16Z

@leventov, I pushed some updates you suggested, please let me know what you think about the others. Thanks for the review.

leventov · 2017-01-04T17:07:42Z

-          // We know there are no numbers here. Use a null supplier.
-          supplier = null;
-        } else {
+        if (selector != null && (clazz == Object.class || Number.class.isAssignableFrom(clazz))) {


Could you please add a test for this condition? Also clazz.isAssignableFrom(Number.class) feels clearer than clazz == Object.class

Sure I'll add a test.

clazz.isAssignableFrom(Number.class) is not exactly what I'm trying to go for, though. This code path might also be parsing Strings (see tryParse). I guess this could be clazz.isAssignableFrom(Number.class) || clazz.isAssignableFrom(String.class).

jon-wei · 2017-01-04T19:55:37Z

+          supplier = null;
+        }
+      } else {
+        // Unhandleable ValueType (possibly STRING or COMPLEX).


Could this attempt to handle String/Complex by going through the nativeType == null tryParse block above (e.g., for columns that contain numbers as strings)?

It could, and we could actually also do an impl based on DimensionSelector, but I avoided that on purpose because that would lead to expressions and field accesses behaving differently for aggregators: longSum over fieldName "x" and expression "x" would behave differently if "x" were a string column. There's a test case in SchemaEvolutionTest that verifies this and would fail if the behavior were different.

I guess we could go in the direction of having aggregators work over string columns though?

Hm, okay, let's leave that area in this PR as-is for consistency then

gianm · 2017-01-05T00:11:51Z

@leventov, I broke up that big method and added tests.

leventov

LGTM

jon-wei · 2017-01-06T01:44:32Z

👍

fjy · 2017-01-06T02:06:36Z

👍

…#3815) * Remove makeMathExpressionSelector from ColumnSelectorFactory. * Add @nullable annotations in places, fix Number.class check. * Break up createBindings, add tests. * Add null check.

Remove makeMathExpressionSelector from ColumnSelectorFactory.

c2e5bae

gianm added the Improvement label Jan 3, 2017

gianm added this to the 0.10.0 milestone Jan 3, 2017

leventov reviewed Jan 4, 2017

View reviewed changes

Add @nullable annotations in places, fix Number.class check.

27b7e72

leventov reviewed Jan 4, 2017

View reviewed changes

jon-wei reviewed Jan 4, 2017

View reviewed changes

Break up createBindings, add tests.

cd15885

Add null check.

cd83c22

leventov reviewed Jan 5, 2017

View reviewed changes

fjy merged commit 3c63cff into apache:master Jan 6, 2017

gianm mentioned this pull request Jan 6, 2017

Add virtual column types, holder serde, and safety features. #3823

Merged

gianm deleted the expression-csf-split branch January 8, 2017 00:45

Conversation

gianm commented Jan 3, 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!

gianm commented Jan 4, 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!

gianm commented Jan 5, 2017

Uh oh!

leventov left a comment

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Jan 6, 2017

Uh oh!

fjy commented Jan 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gianm commented Jan 4, 2017 •

edited

Loading