Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -225,15 +230,16 @@ private static <ColumnSelectorStrategyClass extends ColumnSelectorStrategy> Colu
public static Long convertObjectToLong(Object valObj)
{
if (valObj == null) {
return 0L;
return LONG_ZERO;
}

if (valObj instanceof Long) {
return (Long) 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);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why null checks are needed in checkUnsortedEncodedKeyComponentsEqual() and checkUnsortedEncodedKeyComponentHashCode(), since you added it here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nulls could still appear in TimeAndDims for numeric fields when the input row is missing those columns completely, you can see an example in testNullDimensionTransform() in incremental/IncrementalIndexTest

return parsedVal == null ? LONG_ZERO : parsedVal;
} else {
throw new ParseException("Unknown type[%s]", valObj.getClass());
}
Expand All @@ -242,15 +248,16 @@ 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) {
return (Float) 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 ? FLOAT_ZERO : parsedVal;
} else {
throw new ParseException("Unknown type[%s]", valObj.getClass());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,16 @@ public int compareUnsortedEncodedKeyComponents(Float lhs, Float rhs)
@Override
public boolean checkUnsortedEncodedKeyComponentsEqual(Float lhs, Float rhs)
{
if (lhs == null) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use Objects.equals()

return rhs == null;
}
return lhs.equals(rhs);
}

@Override
public int getUnsortedEncodedKeyComponentHashCode(Float key)
{
return key.hashCode();
return key == null ? 0 : key.hashCode();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use Objects.hashCode()

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,16 @@ public int compareUnsortedEncodedKeyComponents(Long lhs, Long rhs)
@Override
public boolean checkUnsortedEncodedKeyComponentsEqual(Long lhs, Long rhs)
{
if (lhs == null) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

return rhs == null;
}
return lhs.equals(rhs);
}

@Override
public int getUnsortedEncodedKeyComponentHashCode(Long key)
{
return key.hashCode();
return key == null ? 0 : key.hashCode();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -76,8 +78,8 @@ public static Collection<?> constructorFeeder() throws IOException
DimensionsSpec dimensions = new DimensionsSpec(
Arrays.<DimensionSchema>asList(
new StringDimensionSchema("string"),
new StringDimensionSchema("float"),
new StringDimensionSchema("long")
new FloatDimensionSchema("float"),
new LongDimensionSchema("long")
), null, null
);
AggregatorFactory[] metrics = {
Expand Down Expand Up @@ -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.<String, Object>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(
Expand All @@ -214,17 +238,17 @@ public void testNullDimensionTransform() throws IndexSizeExceededException
Lists.newArrayList("string", "float", "long"),
ImmutableMap.<String, Object>of(
"string", Arrays.asList("A", null, ""),
"float", Arrays.asList(Float.MAX_VALUE, null, ""),
"long", Arrays.asList(Long.MIN_VALUE, null, "")
"float", "hello",
"long", "world"
)
)
);

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
Expand Down