From fe69da0d95a361708923fbe5b82ded0d461aa495 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Thu, 15 Nov 2018 22:13:32 -0800 Subject: [PATCH] Expressions: Fix improper supplier reuse with missing columns. (#6600) * Expressions: Fix improper supplier reuse with missing columns. ExpressionSelectors has an optimization that skips building a Map when there is only one input supplier. However, this optimization should not be used in the case where the is one input supplier but more than one input identifier (which can happen when only one input identifier corresponds to an actual column). Fixes #6556. * Add underscores to statics. --- .../segment/virtual/ExpressionSelectors.java | 8 ++- .../virtual/ExpressionVirtualColumnTest.java | 71 ++++++++++++++----- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java index b0d49bad47b..3e452406b06 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java @@ -236,7 +236,8 @@ public class ExpressionSelectors private static Expr.ObjectBinding createBindings(Expr expression, ColumnSelectorFactory columnSelectorFactory) { final Map> suppliers = new HashMap<>(); - for (String columnName : Parser.findRequiredBindings(expression)) { + final List columns = Parser.findRequiredBindings(expression); + for (String columnName : columns) { final ColumnCapabilities columnCapabilities = columnSelectorFactory .getColumnCapabilities(columnName); final ValueType nativeType = columnCapabilities != null ? columnCapabilities.getType() : null; @@ -274,8 +275,9 @@ public class ExpressionSelectors if (suppliers.isEmpty()) { return ExprUtils.nilBindings(); - } else if (suppliers.size() == 1) { - // If there's only one supplier, we can skip the Map and just use that supplier when asked for something. + } else if (suppliers.size() == 1 && columns.size() == 1) { + // If there's only one column (and it has a supplier), we can skip the Map and just use that supplier when + // asked for something. final String column = Iterables.getOnlyElement(suppliers.keySet()); final Supplier supplier = Iterables.getOnlyElement(suppliers.values()); diff --git a/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java b/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java index 37b6e5f86b6..382de0bd4c8 100644 --- a/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java +++ b/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java @@ -70,7 +70,7 @@ public class ExpressionVirtualColumnTest ImmutableMap.of("x", 2L, "y", 3L, "z", "foobar") ); - private static final ExpressionVirtualColumn XPLUSY = new ExpressionVirtualColumn( + private static final ExpressionVirtualColumn X_PLUS_Y = new ExpressionVirtualColumn( "expr", "x + y", ValueType.FLOAT, @@ -88,19 +88,25 @@ public class ExpressionVirtualColumnTest ValueType.FLOAT, TestExprMacroTable.INSTANCE ); - private static final ExpressionVirtualColumn ZLIKE = new ExpressionVirtualColumn( + private static final ExpressionVirtualColumn Z_LIKE = new ExpressionVirtualColumn( "expr", "like(z, 'f%')", ValueType.FLOAT, TestExprMacroTable.INSTANCE ); - private static final ExpressionVirtualColumn ZCONCATX = new ExpressionVirtualColumn( + private static final ExpressionVirtualColumn Z_CONCAT_X = new ExpressionVirtualColumn( "expr", "z + cast(x, 'string')", ValueType.STRING, TestExprMacroTable.INSTANCE ); - private static final ExpressionVirtualColumn TIMEFLOOR = new ExpressionVirtualColumn( + private static final ExpressionVirtualColumn Z_CONCAT_NONEXISTENT = new ExpressionVirtualColumn( + "expr", + "concat(z, nonexistent)", + ValueType.STRING, + TestExprMacroTable.INSTANCE + ); + private static final ExpressionVirtualColumn TIME_FLOOR = new ExpressionVirtualColumn( "expr", "timestamp_floor(__time, 'P1D')", ValueType.LONG, @@ -116,7 +122,7 @@ public class ExpressionVirtualColumnTest @Test public void testObjectSelector() { - final BaseObjectColumnValueSelector selector = XPLUSY.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); + final BaseObjectColumnValueSelector selector = X_PLUS_Y.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); CURRENT_ROW.set(ROW0); Assert.assertEquals(null, selector.getObject()); @@ -139,7 +145,7 @@ public class ExpressionVirtualColumnTest @Test public void testLongSelector() { - final BaseLongColumnValueSelector selector = XPLUSY.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); + final BaseLongColumnValueSelector selector = X_PLUS_Y.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); CURRENT_ROW.set(ROW0); if (NullHandling.replaceWithDefault()) { @@ -166,7 +172,7 @@ public class ExpressionVirtualColumnTest @Test public void testLongSelectorUsingStringFunction() { - final BaseLongColumnValueSelector selector = ZCONCATX.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); + final BaseLongColumnValueSelector selector = Z_CONCAT_X.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); CURRENT_ROW.set(ROW0); if (NullHandling.replaceWithDefault()) { @@ -201,7 +207,7 @@ public class ExpressionVirtualColumnTest @Test public void testFloatSelector() { - final BaseFloatColumnValueSelector selector = XPLUSY.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); + final BaseFloatColumnValueSelector selector = X_PLUS_Y.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); CURRENT_ROW.set(ROW0); if (NullHandling.replaceWithDefault()) { @@ -228,7 +234,7 @@ public class ExpressionVirtualColumnTest @Test public void testDimensionSelector() { - final DimensionSelector selector = XPLUSY.makeDimensionSelector( + final DimensionSelector selector = X_PLUS_Y.makeDimensionSelector( new DefaultDimensionSpec("expr", "expr"), COLUMN_SELECTOR_FACTORY ); @@ -273,7 +279,7 @@ public class ExpressionVirtualColumnTest @Test public void testDimensionSelectorUsingStringFunction() { - final DimensionSelector selector = ZCONCATX.makeDimensionSelector( + final DimensionSelector selector = Z_CONCAT_X.makeDimensionSelector( new DefaultDimensionSpec("expr", "expr"), COLUMN_SELECTOR_FACTORY ); @@ -300,10 +306,43 @@ public class ExpressionVirtualColumnTest Assert.assertEquals("foobar2", selector.lookupName(selector.getRow().get(0))); } + @Test + public void testDimensionSelectorUsingNonexistentColumn() + { + final DimensionSelector selector = Z_CONCAT_NONEXISTENT.makeDimensionSelector( + new DefaultDimensionSpec("expr", "expr"), + COLUMN_SELECTOR_FACTORY + ); + + Assert.assertNotNull(selector); + + CURRENT_ROW.set(ROW0); + Assert.assertEquals(1, selector.getRow().size()); + Assert.assertNull(selector.lookupName(selector.getRow().get(0))); + + CURRENT_ROW.set(ROW1); + Assert.assertEquals(1, selector.getRow().size()); + Assert.assertNull(selector.lookupName(selector.getRow().get(0))); + + CURRENT_ROW.set(ROW2); + Assert.assertEquals(1, selector.getRow().size()); + Assert.assertEquals( + NullHandling.replaceWithDefault() ? "foobar" : null, + selector.lookupName(selector.getRow().get(0)) + ); + + CURRENT_ROW.set(ROW3); + Assert.assertEquals(1, selector.getRow().size()); + Assert.assertEquals( + NullHandling.replaceWithDefault() ? "foobar" : null, + selector.lookupName(selector.getRow().get(0)) + ); + } + @Test public void testDimensionSelectorWithExtraction() { - final DimensionSelector selector = XPLUSY.makeDimensionSelector( + final DimensionSelector selector = X_PLUS_Y.makeDimensionSelector( new ExtractionDimensionSpec("expr", "x", new BucketExtractionFn(1.0, 0.0)), COLUMN_SELECTOR_FACTORY ); @@ -407,7 +446,7 @@ public class ExpressionVirtualColumnTest @Test public void testLongSelectorWithZLikeExprMacro() { - final ColumnValueSelector selector = ZLIKE.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); + final ColumnValueSelector selector = Z_LIKE.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); CURRENT_ROW.set(ROW0); Assert.assertEquals(0L, selector.getLong()); @@ -425,7 +464,7 @@ public class ExpressionVirtualColumnTest @Test public void testLongSelectorOfTimeColumn() { - final ColumnValueSelector selector = TIMEFLOOR.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); + final ColumnValueSelector selector = TIME_FLOOR.makeColumnValueSelector("expr", COLUMN_SELECTOR_FACTORY); CURRENT_ROW.set(ROW0); Assert.assertEquals(DateTimes.of("2000-01-01").getMillis(), selector.getLong()); @@ -447,9 +486,9 @@ public class ExpressionVirtualColumnTest @Test public void testRequiredColumns() { - Assert.assertEquals(ImmutableList.of("x", "y"), XPLUSY.requiredColumns()); + Assert.assertEquals(ImmutableList.of("x", "y"), X_PLUS_Y.requiredColumns()); Assert.assertEquals(ImmutableList.of(), CONSTANT_LIKE.requiredColumns()); - Assert.assertEquals(ImmutableList.of("z"), ZLIKE.requiredColumns()); - Assert.assertEquals(ImmutableList.of("z", "x"), ZCONCATX.requiredColumns()); + Assert.assertEquals(ImmutableList.of("z"), Z_LIKE.requiredColumns()); + Assert.assertEquals(ImmutableList.of("z", "x"), Z_CONCAT_X.requiredColumns()); } }