From 566cb4449f7cdf008020ead9fe86a721ea70f317 Mon Sep 17 00:00:00 2001 From: jon-wei Date: Wed, 5 Jul 2017 17:24:49 -0700 Subject: [PATCH 1/2] Fix NPE when indexing unparseable/missing numeric values when sortFacts=false --- .../druid/segment/DimensionHandlerUtils.java | 6 ++-- .../druid/segment/FloatDimensionIndexer.java | 5 ++- .../druid/segment/LongDimensionIndexer.java | 5 ++- .../incremental/IncrementalIndexTest.java | 36 +++++++++++++++---- 4 files changed, 42 insertions(+), 10 deletions(-) diff --git a/processing/src/main/java/io/druid/segment/DimensionHandlerUtils.java b/processing/src/main/java/io/druid/segment/DimensionHandlerUtils.java index 324fc218392e..20c9ae857e87 100644 --- a/processing/src/main/java/io/druid/segment/DimensionHandlerUtils.java +++ b/processing/src/main/java/io/druid/segment/DimensionHandlerUtils.java @@ -233,7 +233,8 @@ public static Long convertObjectToLong(Object valObj) } else if (valObj instanceof Number) { return ((Number) valObj).longValue(); } else if (valObj instanceof String) { - return DimensionHandlerUtils.getExactLongFromDecimalString((String) valObj); + Long parsedVal = DimensionHandlerUtils.getExactLongFromDecimalString((String) valObj); + return parsedVal == null ? 0L : parsedVal; } else { throw new ParseException("Unknown type[%s]", valObj.getClass()); } @@ -250,7 +251,8 @@ public static Float convertObjectToFloat(Object valObj) } else if (valObj instanceof Number) { return ((Number) valObj).floatValue(); } else if (valObj instanceof String) { - return Floats.tryParse((String) valObj); + Float parsedVal = Floats.tryParse((String) valObj); + return parsedVal == null ? 0.0f : parsedVal; } else { throw new ParseException("Unknown type[%s]", valObj.getClass()); } diff --git a/processing/src/main/java/io/druid/segment/FloatDimensionIndexer.java b/processing/src/main/java/io/druid/segment/FloatDimensionIndexer.java index a7f5f75450c5..b5af884358dd 100644 --- a/processing/src/main/java/io/druid/segment/FloatDimensionIndexer.java +++ b/processing/src/main/java/io/druid/segment/FloatDimensionIndexer.java @@ -199,13 +199,16 @@ public int compareUnsortedEncodedKeyComponents(Float lhs, Float rhs) @Override public boolean checkUnsortedEncodedKeyComponentsEqual(Float lhs, Float rhs) { + if (lhs == null) { + return rhs == null; + } return lhs.equals(rhs); } @Override public int getUnsortedEncodedKeyComponentHashCode(Float key) { - return key.hashCode(); + return key == null ? 0 : key.hashCode(); } @Override diff --git a/processing/src/main/java/io/druid/segment/LongDimensionIndexer.java b/processing/src/main/java/io/druid/segment/LongDimensionIndexer.java index 0b24837db1c8..5a1c371e319f 100644 --- a/processing/src/main/java/io/druid/segment/LongDimensionIndexer.java +++ b/processing/src/main/java/io/druid/segment/LongDimensionIndexer.java @@ -199,13 +199,16 @@ public int compareUnsortedEncodedKeyComponents(Long lhs, Long rhs) @Override public boolean checkUnsortedEncodedKeyComponentsEqual(Long lhs, Long rhs) { + if (lhs == null) { + return rhs == null; + } return lhs.equals(rhs); } @Override public int getUnsortedEncodedKeyComponentHashCode(Long key) { - return key.hashCode(); + return key == null ? 0 : key.hashCode(); } @Override diff --git a/processing/src/test/java/io/druid/segment/incremental/IncrementalIndexTest.java b/processing/src/test/java/io/druid/segment/incremental/IncrementalIndexTest.java index afac0e2a6d3a..d51e60080b71 100644 --- a/processing/src/test/java/io/druid/segment/incremental/IncrementalIndexTest.java +++ b/processing/src/test/java/io/druid/segment/incremental/IncrementalIndexTest.java @@ -28,6 +28,8 @@ import io.druid.data.input.Row; import io.druid.data.input.impl.DimensionSchema; import io.druid.data.input.impl.DimensionsSpec; +import io.druid.data.input.impl.FloatDimensionSchema; +import io.druid.data.input.impl.LongDimensionSchema; import io.druid.data.input.impl.StringDimensionSchema; import io.druid.java.util.common.ISE; import io.druid.java.util.common.granularity.Granularities; @@ -76,8 +78,8 @@ public static Collection constructorFeeder() throws IOException DimensionsSpec dimensions = new DimensionsSpec( Arrays.asList( new StringDimensionSchema("string"), - new StringDimensionSchema("float"), - new StringDimensionSchema("long") + new FloatDimensionSchema("float"), + new LongDimensionSchema("long") ), null, null ); AggregatorFactory[] metrics = { @@ -206,6 +208,28 @@ public void controlTest() throws IndexSizeExceededException @Test public void testNullDimensionTransform() throws IndexSizeExceededException + { + IncrementalIndex index = closer.closeLater(indexCreator.createIndex()); + index.add( + new MapBasedInputRow( + new DateTime().minus(1).getMillis(), + Lists.newArrayList("string", "float", "long"), + ImmutableMap.of( + "string", Arrays.asList("A", null, "") + ) + ) + ); + + Row row = index.iterator().next(); + + Assert.assertEquals(Arrays.asList(new String[]{"", "", "A"}), row.getRaw("string")); + Assert.assertEquals(0.0f, row.getRaw("float")); + Assert.assertEquals(0L, row.getRaw("long")); + } + + + @Test + public void testUnparseableNumerics() throws IndexSizeExceededException { IncrementalIndex index = closer.closeLater(indexCreator.createIndex()); index.add( @@ -214,8 +238,8 @@ public void testNullDimensionTransform() throws IndexSizeExceededException Lists.newArrayList("string", "float", "long"), ImmutableMap.of( "string", Arrays.asList("A", null, ""), - "float", Arrays.asList(Float.MAX_VALUE, null, ""), - "long", Arrays.asList(Long.MIN_VALUE, null, "") + "float", "hello", + "long", "world" ) ) ); @@ -223,8 +247,8 @@ public void testNullDimensionTransform() throws IndexSizeExceededException Row row = index.iterator().next(); Assert.assertEquals(Arrays.asList(new String[]{"", "", "A"}), row.getRaw("string")); - Assert.assertEquals(Arrays.asList(new String[]{"", "", String.valueOf(Float.MAX_VALUE)}), row.getRaw("float")); - Assert.assertEquals(Arrays.asList(new String[]{"", "", String.valueOf(Long.MIN_VALUE)}), row.getRaw("long")); + Assert.assertEquals(0.0f, row.getRaw("float")); + Assert.assertEquals(0L, row.getRaw("long")); } @Test From c93401edc33f47b7ad152a37510a0cd634ec10f1 Mon Sep 17 00:00:00 2001 From: jon-wei Date: Wed, 5 Jul 2017 18:00:52 -0700 Subject: [PATCH 2/2] Add constants for boxed zeros in DimensionHandlerUtils --- .../io/druid/segment/DimensionHandlerUtils.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/processing/src/main/java/io/druid/segment/DimensionHandlerUtils.java b/processing/src/main/java/io/druid/segment/DimensionHandlerUtils.java index 20c9ae857e87..abc5441fa2e6 100644 --- a/processing/src/main/java/io/druid/segment/DimensionHandlerUtils.java +++ b/processing/src/main/java/io/druid/segment/DimensionHandlerUtils.java @@ -40,6 +40,11 @@ public final class DimensionHandlerUtils { + // use these values to ensure that convertObjectToLong() and convertObjectToFloat() return the same + // boxed object when returning a constant zero + private final static Long LONG_ZERO = 0L; + private final static Float FLOAT_ZERO = 0.0f; + private DimensionHandlerUtils() {} public final static ColumnCapabilities DEFAULT_STRING_CAPABILITIES = @@ -225,7 +230,7 @@ private static Colu public static Long convertObjectToLong(Object valObj) { if (valObj == null) { - return 0L; + return LONG_ZERO; } if (valObj instanceof Long) { @@ -234,7 +239,7 @@ public static Long convertObjectToLong(Object valObj) return ((Number) valObj).longValue(); } else if (valObj instanceof String) { Long parsedVal = DimensionHandlerUtils.getExactLongFromDecimalString((String) valObj); - return parsedVal == null ? 0L : parsedVal; + return parsedVal == null ? LONG_ZERO : parsedVal; } else { throw new ParseException("Unknown type[%s]", valObj.getClass()); } @@ -243,7 +248,7 @@ public static Long convertObjectToLong(Object valObj) public static Float convertObjectToFloat(Object valObj) { if (valObj == null) { - return 0.0f; + return FLOAT_ZERO; } if (valObj instanceof Float) { @@ -252,7 +257,7 @@ public static Float convertObjectToFloat(Object valObj) return ((Number) valObj).floatValue(); } else if (valObj instanceof String) { Float parsedVal = Floats.tryParse((String) valObj); - return parsedVal == null ? 0.0f : parsedVal; + return parsedVal == null ? FLOAT_ZERO : parsedVal; } else { throw new ParseException("Unknown type[%s]", valObj.getClass()); }