Adding double colums supports by b-slim · Pull Request #4491 · apache/druid

b-slim · 2017-06-30T14:39:02Z

Make double aggregators store and compute as double.
Adding Float based indexes for whom want to use 32bits as floating point precision.
Fix [#4449]

leventov · 2017-06-30T23:10:59Z


+  @Override
+  public double getDouble(ByteBuffer buf, int position)
+  {


getDouble(), space

leventov · 2017-06-30T23:12:10Z


+  @Override
+  public double getDouble(ByteBuffer buf, int position)
+  {


getDouble()

this is should be getLong

leventov · 2017-06-30T23:13:08Z

    return true;
  }

+  @Override


Please place this method near other similar methods

leventov · 2017-06-30T23:14:07Z

    return (float) most;
  }

+  @Override


Please rearrange getLong()

leventov · 2017-06-30T23:14:23Z

+  {
+    return (double) buf.getLong(position );
+  }
+


Extra space

leventov · 2017-06-30T23:18:44Z

-        } else {
+        } else if (type.equals("double")) {
+          event.put(metric, in.readDouble());
+        }


leventov · 2017-06-30T23:21:30Z


  long getLong();
+
+  default double getDouble() {


Why default?

leventov · 2017-06-30T23:22:35Z

+   *
+   * @param buf byte buffer storing the byte array representation of the aggregate
+   * @param position offset within the byte buffer at which the aggregate value is stored
+   * @return the double representation of the aggregate


It should be */ instead of * */

leventov · 2017-06-30T23:23:31Z

  }
+
+  @Override
+  public double getDouble()


Please rearrange along with other similar methods

leventov · 2017-06-30T23:24:45Z

+  private DoubleColumnSelector getDoubleColumnSelector(ColumnSelectorFactory metricFactory)
  {
-    return AggregatorUtil.getFloatColumnSelector(metricFactory, macroTable, fieldName, expression, Float.MIN_VALUE);
+    return AggregatorUtil.getDoubleColumnSelector(metricFactory, macroTable, fieldName, expression, Float.MIN_VALUE);


Double.NEGATIVE_INFINITY

leventov

Also I have a general question about this PR - why do we need to add float-precision aggregators?

leventov · 2017-07-03T19:29:47Z

+  private DoubleColumnSelector getDoubleColumnSelector(ColumnSelectorFactory metricFactory)
  {
-    return AggregatorUtil.getFloatColumnSelector(metricFactory, macroTable, fieldName, expression, Float.MAX_VALUE);
+    return AggregatorUtil.getDoubleColumnSelector(metricFactory, macroTable, fieldName, expression, Float.MAX_VALUE);


Double.POSITIVE_INFINITY

leventov · 2017-07-03T19:30:37Z

+  private DoubleColumnSelector getDoubleColumnSelector(ColumnSelectorFactory metricFactory)
  {
-    return AggregatorUtil.getFloatColumnSelector(metricFactory, macroTable, fieldName, expression, 0f);
+    return AggregatorUtil.getDoubleColumnSelector(metricFactory, macroTable, fieldName, expression, 0f);


leventov · 2017-07-03T19:31:33Z

+    // no resources to cleanup
+  }
+
+  @Override


Please arrange similar methods together

leventov · 2017-07-03T19:31:54Z

+
+  private FloatColumnSelector getFloatColumnSelector(ColumnSelectorFactory metricFactory)
+  {
+    return AggregatorUtil.getFloatColumnSelector(metricFactory, macroTable, fieldName, expression, Float.MIN_VALUE);


NEGATIVE_INFINITY

leventov · 2017-07-03T19:33:39Z

+  @Override
+  public int getMaxIntermediateSize()
+  {
+    return Floats.BYTES;


Can use Float.BYTES

leventov · 2017-07-03T20:36:15Z

+  ) throws SegmentValidationException
+  {
+    if (!lhs.equals(rhs)) {
+      throw new SegmentValidationException("Dim [%s] value not equal. Expected [%s] found [%s]", lhs, rhs);


Three formatting params, two args

leventov · 2017-07-03T20:36:59Z

+/**
+ * Created by sbouguerra on 6/20/17.
+ */
+public class DoubleDimensionIndexer<EncodedType extends Comparable<EncodedType>, EncodedKeyComponentType, ActualType extends Comparable<ActualType>> implements DimensionIndexer<Double, Double, Double>


Looks like incomplete stub class

leventov · 2017-07-03T20:37:13Z

+import io.druid.segment.incremental.IncrementalIndexStorageAdapter;
+
+/**
+ * Created by sbouguerra on 6/20/17.


Please remove this comment

leventov · 2017-07-03T20:37:59Z

+  @Override
+  public void writeMergedValueMetadata(List<IndexableAdapter> adapters) throws IOException
+  {
+


If this method is empty on purpose please leave a comment

leventov · 2017-07-03T20:38:05Z

+  @Override
+  public void writeIndexes(List<IntBuffer> segmentRowNumConversions, Closer closer) throws IOException
+  {
+


b-slim · 2017-07-09T20:45:21Z

Also I have a general question about this PR - why do we need to add float-precision aggregators?

@leventov If you don't need double precision then it is a wast of storage to store numbers as 64bits instead of 32bits

b-slim · 2017-07-10T13:08:40Z

@leventov thanks for reviewing this, any more comments ?

b-slim · 2017-07-10T15:18:25Z

unrelated error

testStartStopStart(io.druid.query.lookup.KafkaLookupExtractorFactoryTest)  Time elapsed: 0.971 sec  <<< FAILURE!

java.lang.AssertionError: 

  Unexpected method call ConsumerConnector.shutdown():

    ConsumerConnector.createMessageStreamsByFilter(<any>, <any>, io.druid.query.lookup.KafkaLookupExtractorFactory$1@8dfe921, io.druid.query.lookup.KafkaLookupExtractorFactory$1@8dfe921): expected: 1, actual: 0

	at org.easymock.internal.MockInvocationHandler.invoke(MockInvocationHandler.java:44)

	at org.easymock.internal.ObjectMethodsFilter.invoke(ObjectMethodsFilter.java:94)

	at com.sun.proxy.$Proxy37.shutdown(Unknown Source)

	at io.druid.query.lookup.KafkaLookupExtractorFactory.close(KafkaLookupExtractorFactory.java:314)

	at io.druid.query.lookup.KafkaLookupExtractorFactoryTest.testStartStopStart(KafkaLookupExtractorFactoryTest.java:407)

	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)

	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

	at java.lang.reflect.Method.invoke(Method.java:483)

	at org.junit.internal.runners.TestMethod.invoke(TestMethod.java:68)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.runTestMethod(PowerMockJUnit44RunnerDelegateImpl.java:316)

	at org.junit.internal.runners.MethodRoadie$2.run(MethodRoadie.java:88)

	at org.junit.internal.runners.MethodRoadie.runBeforesThenTestThenAfters(MethodRoadie.java:96)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.executeTest(PowerMockJUnit44RunnerDelegateImpl.java:300)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTestInSuper(PowerMockJUnit47RunnerDelegateImpl.java:131)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.access$100(PowerMockJUnit47RunnerDelegateImpl.java:59)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner$TestExecutorStatement.evaluate(PowerMockJUnit47RunnerDelegateImpl.java:147)

	at org.junit.rules.ExpectedException$ExpectedExceptionStatement.evaluate(ExpectedException.java:168)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.evaluateStatement(PowerMockJUnit47RunnerDelegateImpl.java:107)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTest(PowerMockJUnit47RunnerDelegateImpl.java:82)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.runBeforesThenTestThenAfters(PowerMockJUnit44RunnerDelegateImpl.java:288)

	at org.junit.internal.runners.MethodRoadie.runTest(MethodRoadie.java:86)

	at org.junit.internal.runners.MethodRoadie.run(MethodRoadie.java:49)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.invokeTestMethod(PowerMockJUnit44RunnerDelegateImpl.java:208)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.runMethods(PowerMockJUnit44RunnerDelegateImpl.java:147)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$1.run(PowerMockJUnit44RunnerDelegateImpl.java:121)

	at org.junit.internal.runners.ClassRoadie.runUnprotected(ClassRoadie.java:33)

	at org.junit.internal.runners.ClassRoadie.runProtected(ClassRoadie.java:45)

	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.run(PowerMockJUnit44RunnerDelegateImpl.java:123)

	at org.powermock.modules.junit4.common.internal.impl.JUnit4TestSuiteChunkerImpl.run(JUnit4TestSuiteChunkerImpl.java:121)

	at org.powermock.modules.junit4.common.internal.impl.AbstractCommonPowerMockRunner.run(AbstractCommonPowerMockRunner.java:53)

	at org.powermock.modules.junit4.PowerMockRunner.run(PowerMockRunner.java:59)

	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:367)

	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:274)

	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)

	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:161)

	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:290)

	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:242)

	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:121)

Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 22.084 sec - in io.druid.query.lookup.TestKafkaExtractionCluster

Results :

Failed tests: 

  KafkaLookupExtractorFactoryTest.testStartStopStart:407 

  Unexpected method call ConsumerConnector.shutdown():

    ConsumerConnector.createMessageStreamsByFilter(<any>, <any>, io.druid.query.lookup.KafkaLookupExtractorFactory$1@8dfe921, io.druid.query.lookup.KafkaLookupExtractorFactory$1@8dfe921): expected: 1, actual: 0

leventov · 2017-07-11T19:43:48Z

  public double getDouble(ByteBuffer buf, int position)
  {
-    return (double) buf.getLong(position );
+    return buf.getDouble(position);


Seems that this change is wrong (I suggested it - sorry). Also DistinctCountBufferAggregator could extend LongBufferAggregator

leventov · 2017-07-11T19:44:39Z

  public double getDouble(ByteBuffer buf, int position)
  {
-    return (double) buf.getLong(position);
+    return buf.getDouble(position);


Seems that this change is wrong (I suggested it - sorry). Also TimestampBufferAggregator could extend LongBufferAggregator

leventov · 2017-07-11T19:48:53Z

+   *
+   * @param buf byte buffer storing the byte array representation of the aggregate
+   * @param position offset within the byte buffer at which the aggregate value is stored
+   * @return the double representation of the aggregate


It should be */ instead of * */

leventov · 2017-07-11T19:54:10Z

+    );
+  }
+
+  protected DoubleColumnSelector getDoubleColumnSelector(ColumnSelectorFactory metricFactory, Double nullValue)


nullValue param should be primitive double

Thanks, but soon with the null support work this will be an object so i will keep it this way.

leventov · 2017-07-11T19:54:35Z

+  @Override
+  public int getMaxIntermediateSize()
+  {
+    return Doubles.BYTES;


Could be Double.BYTES

leventov · 2017-07-11T20:02:32Z


 public class TopNMapFn
 {
+  public static final long ZERO_LONG = 0L;


If this is to avoid boxing, type should be Long, not long, otherwise it will still be boxing. Also suggested to pull this to some global place, like druid common "Primitives".

And more likely in the form of methods nullToZero(Long), nullToZero(Double), nullToZero(Float) directly, because those constants are not used in any other context.

will make is as an object right now and will stop there, i think this will be changed soon with the upcoming null support soon.

leventov · 2017-07-11T20:06:39Z

      }
-      Arrays.stream(aggregators).forEach(aggregator -> aggregator.aggregate());
+      for (Aggregator aggregator:
+           aggregators) {


No need to break the line

leventov · 2017-07-11T20:06:52Z

  )
  {
-    return new Aggregator[0][];
+    return null;


Why this change?

that per the interface contract.

A dimension type that does not have integer values should return null.

leventov · 2017-07-11T20:17:02Z

 public final class DimensionHandlerUtils
 {
+
+  public static final double ZERO_DOUBLE = 0.0d;


See above, needs to be Double.

leventov · 2017-07-11T20:18:17Z

  @Override
  public boolean canSkip()
  {
+    // can not be all null


This comment doesn't add clarity

leventov · 2017-07-11T20:31:30Z

-      return String.valueOf(o);
-    }
-  };
+  private static final Function<Object, String> TO_STRING_INCLUDING_NULL = o -> String.valueOf(o);


Could be method ref String::valueOf

leventov · 2017-07-11T20:33:01Z

+import io.druid.segment.incremental.IncrementalIndex;
+import io.druid.segment.incremental.IncrementalIndexStorageAdapter;
+
+public class DoubleDimensionIndexer implements DimensionIndexer<Double, Double, Double>


This class looks like uncompleted stub

leventov · 2017-07-11T20:35:03Z

+      {
+        final Object[] dims = currEntry.getKey().getDims();
+
+        if (dimIndex >= dims.length) {


leventov · 2017-07-11T20:37:03Z

+        case DOUBLE:
+          builder.setValueType(ValueType.DOUBLE);
+          builder.addSerde(
+              DoubleGenericColumnPartSerde.serializerBuilder()


It uses different alignment than code above. Also I suggest to break the first method too:

DoubleGenericColumnPartSerde .serializerBuilder() .withByteOrder(IndexIO.BYTE_ORDER) .withDelegate((DoubleColumnSerializer) writer) .build()

leventov · 2017-07-11T20:37:29Z

+      {
+        final Object[] dims = currEntry.getKey().getDims();
+
+        if (dimIndex >= dims.length) {


leventov · 2017-07-11T21:01:21Z

+
      @Override
      public float getFloatMetric(String metric)
      {


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

+    public DoubleGenericColumnPartSerde build()
+    {
+      return new DoubleGenericColumnPartSerde(
+          byteOrder, new Serializer()


Each arg should be on a new line

leventov · 2017-07-11T21:03:00Z

        supplier = supplierFromDimensionSelector(
            columnSelectorFactory.makeDimensionSelector(new DefaultDimensionSpec(columnName, columnName))
        );
+      } else if (nativeType == ValueType.DOUBLE) {


Move this condition higher

leventov · 2017-07-11T21:03:42Z

    }
  }
+
+  public static DoubleColumnSelector makeDoubleColumnSelector(


Arrange this method along with other similar methods

leventov · 2017-07-11T21:04:16Z

    }
  }
+
+  @Override


Arrange this method...

b-slim · 2017-07-12T04:40:20Z

@leventov thanks for the second pass, i have updated the patch.

leventov · 2017-07-12T10:30:56Z

-  public static final double ZERO_DOUBLE = 0.0d;
-  public static final float ZERO_FLOAT = 0.0f;
-  public static final long ZERO_LONG = 0L;
+  public static final Double ZERO_DOUBLE = 0.0d;


Please add documentation to these fields, explaining why they are added

leventov · 2017-07-12T10:33:13Z

+        final Object[] dims = currEntry.getKey().getDims();
+
+        if (dimIndex >= dims.length) {
+          return 0.0;


Could use ZERO_DOUBLE

leventov · 2017-07-12T10:34:17Z

  public int compareUnsortedEncodedKeyComponents(Double lhs, Double rhs)
  {
-    return 0;
+    return lhs.compareTo(rhs);


please fix according to #4509

leventov · 2017-07-12T10:34:25Z

  public boolean checkUnsortedEncodedKeyComponentsEqual(Double lhs, Double rhs)
  {
-    return false;
+    return lhs.equals(rhs);


leventov · 2017-07-12T10:34:30Z

  public int getUnsortedEncodedKeyComponentHashCode(Double key)
  {
-    return 0;
+    return key.hashCode();


leventov · 2017-07-12T10:35:06Z


        if (dimIndex >= dims.length) {
-          return 0L;
+          return 0.0f;


Could use ZERO_FLOAT

nishantmonu51

👍

nishantmonu51 · 2017-07-14T05:39:00Z

+            } else if (typeName.equals("float") || typeName.equals("double")) {
              // called "float", but the aggs really return doubles
              if (cmpIsNumeric) {
                serdeHelper = new DoubleRowBasedKeySerdeHelper(aggOffset);


do we need a FloatRowBasedKeySerdeHelper too ?

b-slim · 2017-07-14T17:08:28Z

@leventov any more comments on this one ?

leventov · 2017-07-14T17:13:44Z

+  @Override
+  public int compareUnsortedEncodedKeyComponents(Double lhs, Double rhs)
+  {
+    return Double.compare(lhs == null ? 0.0: lhs, rhs == null ? 0.0 : rhs);


Let's extract nullToZero as a method

Also please make the same change in LongDimensionIndexer and FloatDimensionIndexer.

leventov · 2017-07-14T17:14:50Z

+  }
+
+  @Override
+  public int compareUnsortedEncodedKeyComponents(Double lhs, Double rhs)


Please mark parameters @Nullable, as well as in the implemented interface. As well as in similar methods i. e. checkUnsortedEncodedKeyComponentsEqual, getUnsortedEncodedKeyComponentHashCode.

b-slim · 2017-07-17T23:03:38Z

@leventov thanks i have addressed the last comments.

leventov · 2017-07-18T10:31:46Z

@@ -242,7 +256,7 @@ public static Long convertObjectToLong(Object valObj)
  public static Float convertObjectToFloat(Object valObj)


This method doesn't match convertObjectToDouble() in null handling of the result of tryParse(). Also, please locate those methods next to each other.

Please make parameter @Nullable

leventov · 2017-07-18T10:32:48Z

@@ -225,7 +239,7 @@ private static <ColumnSelectorStrategyClass extends ColumnSelectorStrategy> Colu
  public static Long convertObjectToLong(Object valObj)


This method doesn't match convertObjectToDouble() in null handling of the result of getExactLongFromDecimalString().

Please make parameter @Nullable

leventov · 2017-07-18T10:35:14Z

    }
  }
+
+  public static Double convertObjectToDouble(Object valObj)


Please make parameter @Nullable

leventov · 2017-07-18T10:36:31Z

+  public int compareUnsortedEncodedKeyComponents(@Nullable Double lhs, @Nullable Double rhs)
+  {
+    // DimensionHandlerUtils.convertObjectToDouble is used to convert null to DoubleZero
+    return Double.compare(DimensionHandlerUtils.convertObjectToDouble(lhs), DimensionHandlerUtils.convertObjectToDouble(rhs));


IMO it's better to extract a method with shorter and more specific name like "nullToZero()", that accepts @Nullable Double, rather than Object. It's better for type safety and simplicity, because compareUnsortedEncodedKeyComponents() and similar methods could be called often. Also it's similar to Guava's nullToEmpty().

Same comment for Long and Float

leventov · 2017-07-18T10:38:43Z

+  @Override
+  public boolean checkUnsortedEncodedKeyComponentsEqual(@Nullable Double lhs, @Nullable Double rhs)
+  {
+    return Objects.equals(lhs, rhs);


If we decide to make null totally equivalent to zero, it should probably be nullToZero(lhs).equals(nullToZero(rhs). Otherwise null won't match zero.

Same comment for Long and Float

leventov · 2017-07-18T10:39:29Z

+  @Override
+  public int getUnsortedEncodedKeyComponentHashCode(@Nullable Double key)
+  {
+    return Objects.hashCode(key);


Same as above, should probably be nullToZero(key).hashCode(). And same for Long and Float.

b-slim · 2017-07-18T19:23:51Z

@leventov thanks done.

leventov · 2017-07-18T21:44:14Z

  }

-  public static Long convertObjectToLong(Object valObj)
+  public static Long convertObjectToLong(@Nullable Object valObj)


Could you comment on not handling null from getExactLongFromDecimalString(), that is not aligned with convertObjectToDouble()?

this is out of the scope of this PR and it will fixed by https://github.com/druid-io/druid/pull/4509/files

leventov · 2017-07-18T21:44:36Z

  }

-  public static Float convertObjectToFloat(Object valObj)
+  public static Float convertObjectToFloat(@Nullable Object valObj)


Same for Floats.tryParse()

same as above.

leventov · 2017-07-18T21:46:00Z

+  public boolean checkUnsortedEncodedKeyComponentsEqual(@Nullable Float lhs, @Nullable Float rhs)
  {
-    return lhs.equals(rhs);
+    return Objects.equals(DimensionHandlerUtils.nullToZero(lhs), DimensionHandlerUtils.nullToZero(rhs));


DimensionHandlerUtils.nullToZero(lhs) is non-null, Objects.equals() is designed to accepts nullable arguments, so it's less confusing to use DimensionHandlerUtils.nullToZero(lhs).equals(DimensionHandlerUtils.nullToZero(rhs)) here. + Same for Double, Long.

leventov · 2017-07-18T21:46:40Z

+  public int getUnsortedEncodedKeyComponentHashCode(@Nullable Float key)
  {
-    return key.hashCode();
+    return Objects.hashCode(DimensionHandlerUtils.nullToZero(key));


Similar to above, suggested DimensionHandlerUtils.nullToZero(key).hashCode(). + Same for Double, Long.

b-slim · 2017-07-18T23:05:01Z

@leventov done

leventov

@b-slim thanks for patience

b-slim · 2017-07-18T23:09:48Z

@leventov thanks for reviewing this !

gianm · 2017-07-20T17:07:38Z

This patch is a great step forwards, but it broke a lot of Druid SQL stuff. Please, consider Druid SQL when reviewing design of PRs. I will try to fix the problems before 0.11.0 is released.

b-slim · 2017-07-20T17:20:43Z

@gianm not sure what is exactly broken all the UTs are passing including the SQL one. I can help fixing stuff if it is clear what is broken.

gianm · 2017-07-20T17:25:17Z

Broken in the sense that if you have double columns, SQL stuff won't work right. The UTs are passing since they don't test double columns. But there's code like this:

  public static StringComparator getStringComparatorForSqlTypeName(SqlTypeName sqlTypeName)
  {
    final ValueType valueType = getValueTypeForSqlTypeName(sqlTypeName);
    if (valueType == ValueType.LONG || valueType == ValueType.FLOAT) {
      return StringComparators.NUMERIC;
    } else {
      return StringComparators.LEXICOGRAPHIC;
    }
  }

Which needs to get cleaned up since it would lead to wrong results. Probably also this kind of code should be changed to throw an error on unrecognized ValueTypes, in case we add new ones in the future, since errors are better than wrong results.

gianm · 2017-07-20T17:31:05Z

Sorry, I'm wrong, it looks like the UTs do test double columns. But they're not testing float columns, and there's still some wires crossed where it treats doubles as floats. I'll take a closer look…

b-slim requested a review from nishantmonu51 June 30, 2017 14:39

b-slim assigned jon-wei Jun 30, 2017

b-slim requested review from jon-wei and leventov June 30, 2017 14:40

b-slim unassigned jon-wei Jun 30, 2017

b-slim added 3 commits June 30, 2017 10:01

add double columns support

8ae5dd6

Fix numbers and expected results in UTs

589f14c

adding float aggregators

da75541

b-slim force-pushed the add_double_columns branch from e9346e0 to da75541 Compare June 30, 2017 17:01

fix IT expected test results

774503d

leventov requested changes Jun 30, 2017

View reviewed changes

leventov requested changes Jul 3, 2017

View reviewed changes

b-slim added 7 commits July 7, 2017 13:50

fix comments

4f2fc8c

Merge branch 'master' into add_double_columns

6d18c6e

more fixes

e9a085c

fix comp

d23ab72

fix test

4041d00

refactor double and float aggregator factories

d2ca27b

fix

a001ed3

fix UTs

58e9185

leventov requested changes Jul 11, 2017

View reviewed changes

b-slim added 2 commits July 11, 2017 21:15

fix comments

88eea4f

clean unused code

76ffaf9

leventov reviewed Jul 12, 2017

View reviewed changes

fix more comments

38393e6

nishantmonu51 approved these changes Jul 14, 2017

View reviewed changes

leventov reviewed Jul 14, 2017

View reviewed changes

handle the null case in DimesionIndexer

9d67f4a

leventov requested changes Jul 18, 2017

View reviewed changes

more null fixing

23d24c3

leventov reviewed Jul 18, 2017

View reviewed changes

cosmetic changes

0fda10d

leventov approved these changes Jul 18, 2017

View reviewed changes

b-slim closed this Jul 19, 2017

b-slim reopened this Jul 19, 2017

leventov merged commit 71e7a4c into apache:master Jul 20, 2017

gianm mentioned this pull request Jul 26, 2017

Migration path for double columns #4605

Closed

b-slim mentioned this pull request Aug 4, 2017

[Proposal] Adding double presion mertrics. #4449

Closed

xanec mentioned this pull request Aug 8, 2017

Possible unhandled null value in ExpressionSelectors #4666

Closed

leventov mentioned this pull request Aug 9, 2017

FloatSumAggregatorFactory, FloatMinAggregatorFactory and FloatMaxAggregatorFactory have identical equals() that could be moved to SimpleFloatAggregatorFactory #4671

Closed

leventov mentioned this pull request Sep 14, 2017

Replace HistoricalFloatColumnSelector with more generic HistoricalColumnSelector #4796

Merged

jon-wei added this to the 0.11.0 milestone Sep 28, 2017

jon-wei mentioned this pull request Oct 10, 2017

Druid 0.11.0 release notes #4876

Closed

b-slim mentioned this pull request Mar 2, 2018

Make Doubles aggregators use 64bits by default. #5462

Closed

b-slim deleted the add_double_columns branch April 26, 2018 01:27

dylwylie mentioned this pull request Feb 7, 2019

Support double type metric #3294

Closed

		@@ -242,7 +256,7 @@ public static Long convertObjectToLong(Object valObj)
		public static Float convertObjectToFloat(Object valObj)

		@@ -225,7 +239,7 @@ private static <ColumnSelectorStrategyClass extends ColumnSelectorStrategy> Colu
		public static Long convertObjectToLong(Object valObj)

Conversation

b-slim commented Jun 30, 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!

leventov left a comment

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!

b-slim commented Jul 9, 2017

Uh oh!

b-slim commented Jul 10, 2017

Uh oh!

b-slim commented Jul 10, 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

b-slim commented Jun 30, 2017 •

edited

Loading