From 84ef8b819e961cf9e2684197c534875e82bf3461 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 18 Dec 2019 13:30:34 -0800 Subject: [PATCH] fix druid-sql issue with filtering numeric columns by null values (#9061) * fix druid-sql issue with filtering numeric columns by null values * fix tests * fix tests for reals --- .../druid/sql/calcite/table/RowSignature.java | 8 +- .../sql/avatica/DruidAvaticaHandlerTest.java | 15 +- .../druid/sql/calcite/CalciteQueryTest.java | 171 ++++++++++++++++-- 3 files changed, 166 insertions(+), 28 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/table/RowSignature.java b/sql/src/main/java/org/apache/druid/sql/calcite/table/RowSignature.java index c6051c8d784..d5ed8509ef9 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/table/RowSignature.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/table/RowSignature.java @@ -26,6 +26,7 @@ import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rel.type.RelDataTypeField; import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.Pair; @@ -146,6 +147,7 @@ public class RowSignature public RelDataType getRelDataType(final RelDataTypeFactory typeFactory) { final RelDataTypeFactory.Builder builder = typeFactory.builder(); + final boolean nullNumeric = !NullHandling.replaceWithDefault(); for (final String columnName : columnNames) { final ValueType columnType = getColumnType(columnName); final RelDataType type; @@ -159,13 +161,13 @@ public class RowSignature type = Calcites.createSqlTypeWithNullability(typeFactory, SqlTypeName.VARCHAR, true); break; case LONG: - type = Calcites.createSqlType(typeFactory, SqlTypeName.BIGINT); + type = Calcites.createSqlTypeWithNullability(typeFactory, SqlTypeName.BIGINT, nullNumeric); break; case FLOAT: - type = Calcites.createSqlType(typeFactory, SqlTypeName.FLOAT); + type = Calcites.createSqlTypeWithNullability(typeFactory, SqlTypeName.FLOAT, nullNumeric); break; case DOUBLE: - type = Calcites.createSqlType(typeFactory, SqlTypeName.DOUBLE); + type = Calcites.createSqlTypeWithNullability(typeFactory, SqlTypeName.DOUBLE, nullNumeric); break; case COMPLEX: // Loses information about exactly what kind of complex column this is. diff --git a/sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java b/sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java index 195e523325d..5d318689abf 100644 --- a/sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java +++ b/sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java @@ -35,6 +35,7 @@ import org.apache.calcite.avatica.AvaticaClientRuntimeException; import org.apache.calcite.avatica.Meta; import org.apache.calcite.avatica.MissingResultsException; import org.apache.calcite.avatica.NoSuchStatementException; +import org.apache.druid.common.config.NullHandling; import org.apache.druid.guice.GuiceInjectors; import org.apache.druid.initialization.Initialization; import org.apache.druid.java.util.common.DateTimes; @@ -118,6 +119,8 @@ public class DruidAvaticaHandlerTest extends CalciteTestBase private static QueryRunnerFactoryConglomerate conglomerate; private static Closer resourceCloser; + private final boolean nullNumeric = !NullHandling.replaceWithDefault(); + @BeforeClass public static void setUpClass() { @@ -496,7 +499,7 @@ public class DruidAvaticaHandlerTest extends CalciteTestBase Pair.of("COLUMN_NAME", "cnt"), Pair.of("DATA_TYPE", Types.BIGINT), Pair.of("TYPE_NAME", "BIGINT"), - Pair.of("IS_NULLABLE", "NO") + Pair.of("IS_NULLABLE", nullNumeric ? "YES" : "NO") ), row( Pair.of("TABLE_SCHEM", "druid"), @@ -528,7 +531,7 @@ public class DruidAvaticaHandlerTest extends CalciteTestBase Pair.of("COLUMN_NAME", "m1"), Pair.of("DATA_TYPE", Types.FLOAT), Pair.of("TYPE_NAME", "FLOAT"), - Pair.of("IS_NULLABLE", "NO") + Pair.of("IS_NULLABLE", nullNumeric ? "YES" : "NO") ), row( Pair.of("TABLE_SCHEM", "druid"), @@ -536,7 +539,7 @@ public class DruidAvaticaHandlerTest extends CalciteTestBase Pair.of("COLUMN_NAME", "m2"), Pair.of("DATA_TYPE", Types.DOUBLE), Pair.of("TYPE_NAME", "DOUBLE"), - Pair.of("IS_NULLABLE", "NO") + Pair.of("IS_NULLABLE", nullNumeric ? "YES" : "NO") ), row( Pair.of("TABLE_SCHEM", "druid"), @@ -587,7 +590,7 @@ public class DruidAvaticaHandlerTest extends CalciteTestBase Pair.of("COLUMN_NAME", "cnt"), Pair.of("DATA_TYPE", Types.BIGINT), Pair.of("TYPE_NAME", "BIGINT"), - Pair.of("IS_NULLABLE", "NO") + Pair.of("IS_NULLABLE", nullNumeric ? "YES" : "NO") ), row( Pair.of("TABLE_SCHEM", "druid"), @@ -611,7 +614,7 @@ public class DruidAvaticaHandlerTest extends CalciteTestBase Pair.of("COLUMN_NAME", "m1"), Pair.of("DATA_TYPE", Types.FLOAT), Pair.of("TYPE_NAME", "FLOAT"), - Pair.of("IS_NULLABLE", "NO") + Pair.of("IS_NULLABLE", nullNumeric ? "YES" : "NO") ), row( Pair.of("TABLE_SCHEM", "druid"), @@ -619,7 +622,7 @@ public class DruidAvaticaHandlerTest extends CalciteTestBase Pair.of("COLUMN_NAME", "m2"), Pair.of("DATA_TYPE", Types.DOUBLE), Pair.of("TYPE_NAME", "DOUBLE"), - Pair.of("IS_NULLABLE", "NO") + Pair.of("IS_NULLABLE", nullNumeric ? "YES" : "NO") ), row( Pair.of("TABLE_SCHEM", "druid"), diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index 65e2d0ff1e2..88b3d588f71 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -384,12 +384,12 @@ public class CalciteQueryTest extends BaseCalciteQueryTest ImmutableList.of(), ImmutableList.of( new Object[]{"__time", "TIMESTAMP", "NO"}, - new Object[]{"cnt", "BIGINT", "NO"}, + new Object[]{"cnt", "BIGINT", useDefault ? "NO" : "YES"}, new Object[]{"dim1", "VARCHAR", "YES"}, new Object[]{"dim2", "VARCHAR", "YES"}, new Object[]{"dim3", "VARCHAR", "YES"}, - new Object[]{"m1", "FLOAT", "NO"}, - new Object[]{"m2", "DOUBLE", "NO"}, + new Object[]{"m1", "FLOAT", useDefault ? "NO" : "YES"}, + new Object[]{"m2", "DOUBLE", useDefault ? "NO" : "YES"}, new Object[]{"unique_dim1", "OTHER", "YES"} ) ); @@ -415,11 +415,11 @@ public class CalciteQueryTest extends BaseCalciteQueryTest ImmutableList.of(), ImmutableList.of( new Object[]{"__time", "TIMESTAMP", "NO"}, - new Object[]{"cnt", "BIGINT", "NO"}, + new Object[]{"cnt", "BIGINT", useDefault ? "NO" : "YES"}, new Object[]{"dim1", "VARCHAR", "YES"}, new Object[]{"dim2", "VARCHAR", "YES"}, - new Object[]{"m1", "FLOAT", "NO"}, - new Object[]{"m2", "DOUBLE", "NO"}, + new Object[]{"m1", "FLOAT", useDefault ? "NO" : "YES"}, + new Object[]{"m2", "DOUBLE", useDefault ? "NO" : "YES"}, new Object[]{"unique_dim1", "OTHER", "YES"} ) ); @@ -1705,7 +1705,16 @@ public class CalciteQueryTest extends BaseCalciteQueryTest .setInterval(querySegmentSpec(Filtration.eternity())) .setGranularity(Granularities.ALL) .setDimensions(dimensions(new DefaultDimensionSpec("d0", "_d0", ValueType.STRING))) - .setAggregatorSpecs(aggregators(new CountAggregatorFactory("a0"))) + .setAggregatorSpecs( + aggregators( + useDefault + ? new CountAggregatorFactory("a0") + : new FilteredAggregatorFactory( + new CountAggregatorFactory("a0"), + not(selector("d1", null, null)) + ) + ) + ) .setHavingSpec( having( bound( @@ -2265,6 +2274,82 @@ public class CalciteQueryTest extends BaseCalciteQueryTest ); } + @Test + public void testNullLongFilter() throws Exception + { + // bug https://github.com/apache/incubator-druid/issues/9062 prevents this test from passing + skipVectorize(); + testQuery( + "SELECT COUNT(*)\n" + + "FROM druid.numfoo\n" + + "WHERE l1 IS NULL", + useDefault ? ImmutableList.of() : ImmutableList.of( + Druids.newTimeseriesQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .granularity(Granularities.ALL) + .filters(selector("l1", null, null)) + .aggregators(aggregators(new CountAggregatorFactory("a0"))) + .context(TIMESERIES_CONTEXT_DEFAULT) + .build() + ), + ImmutableList.of( + useDefault ? new Object[]{0L} : new Object[]{3L} + ) + ); + } + + @Test + public void testNullDoubleFilter() throws Exception + { + // bug https://github.com/apache/incubator-druid/issues/9062 prevents this test from passing + skipVectorize(); + testQuery( + "SELECT COUNT(*)\n" + + "FROM druid.numfoo\n" + + "WHERE d1 IS NULL", + useDefault ? ImmutableList.of() : ImmutableList.of( + Druids.newTimeseriesQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .granularity(Granularities.ALL) + .filters(selector("d1", null, null)) + .aggregators(aggregators(new CountAggregatorFactory("a0"))) + .context(TIMESERIES_CONTEXT_DEFAULT) + .build() + ), + ImmutableList.of( + useDefault ? new Object[]{0L} : new Object[]{3L} + ) + ); + } + + + @Test + public void testNullFloatFilter() throws Exception + { + // bug https://github.com/apache/incubator-druid/issues/9062 prevents this test from passing + skipVectorize(); + testQuery( + "SELECT COUNT(*)\n" + + "FROM druid.numfoo\n" + + "WHERE f1 IS NULL", + useDefault ? ImmutableList.of() : ImmutableList.of( + Druids.newTimeseriesQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .granularity(Granularities.ALL) + .filters(selector("f1", null, null)) + .aggregators(aggregators(new CountAggregatorFactory("a0"))) + .context(TIMESERIES_CONTEXT_DEFAULT) + .build() + ), + ImmutableList.of( + useDefault ? new Object[]{0L} : new Object[]{3L} + ) + ); + } + @Test public void testEmptyStringEquality() throws Exception { @@ -2568,7 +2653,16 @@ public class CalciteQueryTest extends BaseCalciteQueryTest .dataSource(CalciteTests.DATASOURCE1) .intervals(querySegmentSpec(Filtration.eternity())) .granularity(Granularities.ALL) - .aggregators(aggregators(new CountAggregatorFactory("a0"))) + .aggregators( + aggregators( + useDefault + ? new CountAggregatorFactory("a0") + : new FilteredAggregatorFactory( + new CountAggregatorFactory("a0"), + not(selector("cnt", null, null)) + ) + ) + ) .context(TIMESERIES_CONTEXT_DEFAULT) .build() ), @@ -2930,12 +3024,14 @@ public class CalciteQueryTest extends BaseCalciteQueryTest testQuery( "SELECT COUNT(*), COUNT(cnt), COUNT(dim1), AVG(cnt), SUM(cnt), SUM(cnt) + MIN(cnt) + MAX(cnt), COUNT(dim2) FROM druid.foo", ImmutableList.of( + Druids.newTimeseriesQueryBuilder() .dataSource(CalciteTests.DATASOURCE1) .intervals(querySegmentSpec(Filtration.eternity())) .granularity(Granularities.ALL) .aggregators( - aggregators( + useDefault + ? aggregators( new CountAggregatorFactory("a0"), new FilteredAggregatorFactory( new CountAggregatorFactory("a1"), @@ -2951,17 +3047,40 @@ public class CalciteQueryTest extends BaseCalciteQueryTest not(selector("dim2", null, null)) ) ) + : aggregators( + new CountAggregatorFactory("a0"), + new FilteredAggregatorFactory( + new CountAggregatorFactory("a1"), + not(selector("cnt", null, null)) + ), + new FilteredAggregatorFactory( + new CountAggregatorFactory("a2"), + not(selector("dim1", null, null)) + ), + new LongSumAggregatorFactory("a3:sum", "cnt"), + new CountAggregatorFactory("a3:count"), + new LongSumAggregatorFactory("a4", "cnt"), + new LongMinAggregatorFactory("a5", "cnt"), + new LongMaxAggregatorFactory("a6", "cnt"), + new FilteredAggregatorFactory( + new CountAggregatorFactory("a7"), + not(selector("dim2", null, null)) + ) + ) ) .postAggregators( new ArithmeticPostAggregator( - "a2", + useDefault ? "a2" : "a3", "quotient", ImmutableList.of( - new FieldAccessPostAggregator(null, "a2:sum"), - new FieldAccessPostAggregator(null, "a2:count") + new FieldAccessPostAggregator(null, useDefault ? "a2:sum" : "a3:sum"), + new FieldAccessPostAggregator(null, useDefault ? "a2:count" : "a3:count") ) ), - expressionPostAgg("p0", "((\"a3\" + \"a4\") + \"a5\")") + expressionPostAgg( + "p0", + useDefault ? "((\"a3\" + \"a4\") + \"a5\")" : "((\"a4\" + \"a5\") + \"a6\")" + ) ) .context(TIMESERIES_CONTEXT_DEFAULT) .build() @@ -4773,9 +4892,16 @@ public class CalciteQueryTest extends BaseCalciteQueryTest new DefaultDimensionSpec("v0", "v0", ValueType.LONG), new DefaultDimensionSpec("d0", "_d0", ValueType.STRING) )) - .setAggregatorSpecs(aggregators( - new CountAggregatorFactory("_a0") - )) + .setAggregatorSpecs( + aggregators( + useDefault + ? new CountAggregatorFactory("_a0") + : new FilteredAggregatorFactory( + new CountAggregatorFactory("_a0"), + not(selector("d1", null, null)) + ) + ) + ) .setContext(QUERY_CONTEXT_DEFAULT) .build() ), @@ -8031,9 +8157,16 @@ public class CalciteQueryTest extends BaseCalciteQueryTest new DefaultDimensionSpec("d0", "_d0", ValueType.LONG), new DefaultDimensionSpec("d1", "_d1", ValueType.STRING) )) - .setAggregatorSpecs(aggregators( - new CountAggregatorFactory("a0") - )) + .setAggregatorSpecs( + aggregators( + useDefault + ? new CountAggregatorFactory("a0") + : new FilteredAggregatorFactory( + new CountAggregatorFactory("a0"), + not(selector("d2", null, null)) + ) + ) + ) .setContext(QUERY_CONTEXT_DEFAULT) .build() ),