From 567e38170500d3649cbfaa28cf7aa6f5275d02e7 Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Tue, 13 Oct 2020 01:34:55 +0530 Subject: [PATCH] Any virtual column on "__time" should be a pre-join virtual column (#10451) * Virtual column on __time should be in pre-join * Add unit test --- .../join/HashJoinSegmentStorageAdapter.java | 2 + ...BaseHashJoinSegmentStorageAdapterTest.java | 17 ++++++ .../HashJoinSegmentStorageAdapterTest.java | 53 ++++++++++++++----- 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java b/processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java index 03f3f946d46..d6517c1bfbc 100644 --- a/processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java +++ b/processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java @@ -34,6 +34,7 @@ import org.apache.druid.segment.StorageAdapter; import org.apache.druid.segment.VirtualColumn; import org.apache.druid.segment.VirtualColumns; import org.apache.druid.segment.column.ColumnCapabilities; +import org.apache.druid.segment.column.ColumnHolder; import org.apache.druid.segment.data.Indexed; import org.apache.druid.segment.data.ListIndexed; import org.apache.druid.segment.join.filter.JoinFilterAnalyzer; @@ -305,6 +306,7 @@ public class HashJoinSegmentStorageAdapter implements StorageAdapter ) { final Set baseColumns = new HashSet<>(); + baseColumns.add(ColumnHolder.TIME_COLUMN_NAME); Iterables.addAll(baseColumns, baseAdapter.getAvailableDimensions()); Iterables.addAll(baseColumns, baseAdapter.getAvailableMetrics()); diff --git a/processing/src/test/java/org/apache/druid/segment/join/BaseHashJoinSegmentStorageAdapterTest.java b/processing/src/test/java/org/apache/druid/segment/join/BaseHashJoinSegmentStorageAdapterTest.java index 6a6af72dc39..d5dc9a2f1f8 100644 --- a/processing/src/test/java/org/apache/druid/segment/join/BaseHashJoinSegmentStorageAdapterTest.java +++ b/processing/src/test/java/org/apache/druid/segment/join/BaseHashJoinSegmentStorageAdapterTest.java @@ -27,7 +27,9 @@ import org.apache.druid.query.QueryContexts; import org.apache.druid.query.filter.Filter; import org.apache.druid.query.lookup.LookupExtractor; import org.apache.druid.segment.QueryableIndexSegment; +import org.apache.druid.segment.VirtualColumn; import org.apache.druid.segment.VirtualColumns; +import org.apache.druid.segment.column.ValueType; import org.apache.druid.segment.join.filter.JoinFilterAnalyzer; import org.apache.druid.segment.join.filter.JoinFilterPreAnalysis; import org.apache.druid.segment.join.filter.JoinFilterPreAnalysisKey; @@ -242,4 +244,19 @@ public class BaseHashJoinSegmentStorageAdapterTest ) ); } + + protected VirtualColumn makeExpressionVirtualColumn(String expression) + { + return makeExpressionVirtualColumn(expression, "virtual"); + } + + protected VirtualColumn makeExpressionVirtualColumn(String expression, String columnName) + { + return new ExpressionVirtualColumn( + columnName, + expression, + ValueType.STRING, + ExprMacroTable.nil() + ); + } } diff --git a/processing/src/test/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapterTest.java b/processing/src/test/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapterTest.java index 6406d7afe09..55469624ac8 100644 --- a/processing/src/test/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapterTest.java +++ b/processing/src/test/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapterTest.java @@ -31,6 +31,7 @@ import org.apache.druid.query.filter.ExpressionDimFilter; import org.apache.druid.query.filter.Filter; import org.apache.druid.query.filter.OrDimFilter; import org.apache.druid.query.filter.SelectorDimFilter; +import org.apache.druid.segment.VirtualColumn; import org.apache.druid.segment.VirtualColumns; import org.apache.druid.segment.column.ColumnCapabilities; import org.apache.druid.segment.column.ValueType; @@ -38,10 +39,10 @@ import org.apache.druid.segment.filter.SelectorFilter; import org.apache.druid.segment.join.filter.JoinFilterPreAnalysis; import org.apache.druid.segment.join.lookup.LookupJoinable; import org.apache.druid.segment.join.table.IndexedTableJoinable; -import org.apache.druid.segment.virtual.ExpressionVirtualColumn; import org.junit.Assert; import org.junit.Test; +import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -1502,12 +1503,7 @@ public class HashJoinSegmentStorageAdapterTest extends BaseHashJoinSegmentStorag VirtualColumns virtualColumns = VirtualColumns.create( Collections.singletonList( - new ExpressionVirtualColumn( - "virtual", - "concat(substring(countryIsoCode, 0, 1),'L')", - ValueType.STRING, - ExprMacroTable.nil() - ) + makeExpressionVirtualColumn("concat(substring(countryIsoCode, 0, 1),'L')") ) ); @@ -1563,12 +1559,7 @@ public class HashJoinSegmentStorageAdapterTest extends BaseHashJoinSegmentStorag VirtualColumns virtualColumns = VirtualColumns.create( Collections.singletonList( - new ExpressionVirtualColumn( - "virtual", - "concat(substring(countryIsoCode, 0, 1),'L')", - ValueType.STRING, - ExprMacroTable.nil() - ) + makeExpressionVirtualColumn("concat(substring(countryIsoCode, 0, 1),'L')") ) ); @@ -2028,4 +2019,40 @@ public class HashJoinSegmentStorageAdapterTest extends BaseHashJoinSegmentStorag ); } + @Test + public void test_determineBaseColumnsWithPreAndPostJoinVirtualColumns() + { + List joinableClauses = ImmutableList.of(factToCountryOnIsoCode(JoinType.LEFT)); + JoinFilterPreAnalysis analysis = makeDefaultConfigPreAnalysis(null, joinableClauses, VirtualColumns.EMPTY); + HashJoinSegmentStorageAdapter adapter = new HashJoinSegmentStorageAdapter( + factSegment.asStorageAdapter(), + joinableClauses, + analysis + ); + List expectedPreJoin = ImmutableList.of( + makeExpressionVirtualColumn("concat(countryIsoCode,'L')", "v0"), + makeExpressionVirtualColumn("concat(countryIsoCode, countryNumber)", "v1"), + makeExpressionVirtualColumn("channel_uniques - 1", "v2"), + makeExpressionVirtualColumn("channel_uniques - __time", "v3") + ); + + List expectedPostJoin = ImmutableList.of( + makeExpressionVirtualColumn("concat(countryIsoCode, dummyColumn)", "v4"), + makeExpressionVirtualColumn("dummyMetric - __time", "v5") + ); + List actualPreJoin = new ArrayList<>(); + List actualPostJoin = new ArrayList<>(); + List allVirtualColumns = new ArrayList<>(); + allVirtualColumns.addAll(expectedPreJoin); + allVirtualColumns.addAll(expectedPostJoin); + adapter.determineBaseColumnsWithPreAndPostJoinVirtualColumns( + VirtualColumns.create(allVirtualColumns), + actualPreJoin, + actualPostJoin + ); + + Assert.assertEquals(expectedPreJoin, actualPreJoin); + Assert.assertEquals(expectedPostJoin, actualPostJoin); + } + }