From 92912a6a2b5cbea282137151dfa0314b97568988 Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Thu, 6 Apr 2023 16:55:58 +0530 Subject: [PATCH] JOIN or UNNEST queries over tombstone segment can fail (#14021) Join,Unnest queries over tombstone segment can fail --- .../druid/query/scan/ScanQueryEngine.java | 7 +- .../apache/druid/segment/QueryableIndex.java | 8 -- .../apache/druid/segment/StorageAdapter.java | 8 ++ .../loading/TombstoneSegmentizerFactory.java | 16 +-- ...> TombstoneSegmentStorageAdapterTest.java} | 101 ++++++++++++------ .../TombstoneSegmentizerFactoryTest.java | 18 ++-- .../server/coordination/ServerManager.java | 14 +-- .../loading/SegmentLocalCacheLoaderTest.java | 2 +- 8 files changed, 109 insertions(+), 65 deletions(-) rename processing/src/test/java/org/apache/druid/segment/{SimpleQueryableIndexTest.java => TombstoneSegmentStorageAdapterTest.java} (54%) diff --git a/processing/src/main/java/org/apache/druid/query/scan/ScanQueryEngine.java b/processing/src/main/java/org/apache/druid/query/scan/ScanQueryEngine.java index 82a33962e7c..efcbe51c0c4 100644 --- a/processing/src/main/java/org/apache/druid/query/scan/ScanQueryEngine.java +++ b/processing/src/main/java/org/apache/druid/query/scan/ScanQueryEngine.java @@ -66,9 +66,6 @@ public class ScanQueryEngine @Nullable final QueryMetrics queryMetrics ) { - if (segment.asQueryableIndex() != null && segment.asQueryableIndex().isFromTombstone()) { - return Sequences.empty(); - } // "legacy" should be non-null due to toolChest.mergeResults final boolean legacy = Preconditions.checkNotNull(query.isLegacy(), "Expected non-null 'legacy' parameter"); @@ -87,6 +84,10 @@ public class ScanQueryEngine ); } + if (adapter.isFromTombstone()) { + return Sequences.empty(); + } + final List allColumns = new ArrayList<>(); if (query.getColumns() != null && !query.getColumns().isEmpty()) { diff --git a/processing/src/main/java/org/apache/druid/segment/QueryableIndex.java b/processing/src/main/java/org/apache/druid/segment/QueryableIndex.java index 8011f1e47fd..b4cd503681c 100644 --- a/processing/src/main/java/org/apache/druid/segment/QueryableIndex.java +++ b/processing/src/main/java/org/apache/druid/segment/QueryableIndex.java @@ -70,12 +70,4 @@ public interface QueryableIndex extends Closeable, ColumnInspector //@Deprecated // This is still required for SimpleQueryableIndex. It should not go away until SimpleQueryableIndex is fixed @Override void close(); - - /** - * @return true if this index was created from a tombstone or false otherwise - */ - default boolean isFromTombstone() - { - return false; - } } diff --git a/processing/src/main/java/org/apache/druid/segment/StorageAdapter.java b/processing/src/main/java/org/apache/druid/segment/StorageAdapter.java index 0535d562517..ce23ae7d1cc 100644 --- a/processing/src/main/java/org/apache/druid/segment/StorageAdapter.java +++ b/processing/src/main/java/org/apache/druid/segment/StorageAdapter.java @@ -119,4 +119,12 @@ public interface StorageAdapter extends CursorFactory, ColumnInspector { return false; } + + /** + * @return true if this index was created from a tombstone or false otherwise + */ + default boolean isFromTombstone() + { + return false; + } } diff --git a/processing/src/main/java/org/apache/druid/segment/loading/TombstoneSegmentizerFactory.java b/processing/src/main/java/org/apache/druid/segment/loading/TombstoneSegmentizerFactory.java index 8b1987a0b5e..72958e8049d 100644 --- a/processing/src/main/java/org/apache/druid/segment/loading/TombstoneSegmentizerFactory.java +++ b/processing/src/main/java/org/apache/druid/segment/loading/TombstoneSegmentizerFactory.java @@ -36,6 +36,7 @@ import org.apache.druid.timeline.SegmentId; import org.joda.time.Interval; import javax.annotation.Nullable; + import java.io.File; import java.util.List; import java.util.Map; @@ -120,15 +121,16 @@ public class TombstoneSegmentizerFactory implements SegmentizerFactory throw new UnsupportedOperationException(); } - // mark this index to indicate that it comes from a tombstone: - @Override - public boolean isFromTombstone() - { - return true; - } }; - final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(queryableIndex); + final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(queryableIndex) + { + @Override + public boolean isFromTombstone() + { + return true; + } + }; Segment segmentObject = new Segment() { diff --git a/processing/src/test/java/org/apache/druid/segment/SimpleQueryableIndexTest.java b/processing/src/test/java/org/apache/druid/segment/TombstoneSegmentStorageAdapterTest.java similarity index 54% rename from processing/src/test/java/org/apache/druid/segment/SimpleQueryableIndexTest.java rename to processing/src/test/java/org/apache/druid/segment/TombstoneSegmentStorageAdapterTest.java index 68ccf1d54e6..c93abe864f1 100644 --- a/processing/src/test/java/org/apache/druid/segment/SimpleQueryableIndexTest.java +++ b/processing/src/test/java/org/apache/druid/segment/TombstoneSegmentStorageAdapterTest.java @@ -19,27 +19,41 @@ package org.apache.druid.segment; -import org.apache.druid.collections.bitmap.BitmapFactory; -import org.apache.druid.segment.column.ColumnHolder; +import org.apache.druid.java.util.common.granularity.Granularity; +import org.apache.druid.java.util.common.guava.Sequence; +import org.apache.druid.query.QueryMetrics; +import org.apache.druid.query.filter.Filter; +import org.apache.druid.segment.column.ColumnCapabilities; import org.apache.druid.segment.data.Indexed; +import org.joda.time.DateTime; import org.joda.time.Interval; import org.junit.Assert; import org.junit.Test; import javax.annotation.Nullable; -import java.util.List; -import java.util.Map; -public class SimpleQueryableIndexTest +public class TombstoneSegmentStorageAdapterTest { @Test public void testTombstoneDefaultInterface() { - QueryableIndex qi = new QueryableIndex() + StorageAdapter sa = new StorageAdapter() { @Override - public Interval getDataInterval() + public Sequence makeCursors( + @Nullable Filter filter, + Interval interval, + VirtualColumns virtualColumns, + Granularity gran, + boolean descending, + @Nullable QueryMetrics queryMetrics) + { + return null; + } + + @Override + public Interval getInterval() { return null; } @@ -50,6 +64,12 @@ public class SimpleQueryableIndexTest return 0; } + @Override + public DateTime getMaxIngestedEventTime() + { + return null; + } + @Override public Indexed getAvailableDimensions() { @@ -57,7 +77,46 @@ public class SimpleQueryableIndexTest } @Override - public BitmapFactory getBitmapFactoryForDimensions() + public Iterable getAvailableMetrics() + { + return null; + } + + @Override + public int getDimensionCardinality(String column) + { + return 0; + } + + @Override + public DateTime getMinTime() + { + return null; + } + + @Override + public DateTime getMaxTime() + { + return null; + } + + @Nullable + @Override + public Comparable getMinValue(String column) + { + return null; + } + + @Nullable + @Override + public Comparable getMaxValue(String column) + { + return null; + } + + @Nullable + @Override + public ColumnCapabilities getColumnCapabilities(String column) { return null; } @@ -69,33 +128,9 @@ public class SimpleQueryableIndexTest return null; } - @Override - public Map getDimensionHandlers() - { - return null; - } - - @Override - public void close() - { - - } - - @Override - public List getColumnNames() - { - return null; - } - - @Nullable - @Override - public ColumnHolder getColumnHolder(String columnName) - { - return null; - } }; - Assert.assertFalse(qi.isFromTombstone()); + Assert.assertFalse(sa.isFromTombstone()); } } diff --git a/processing/src/test/java/org/apache/druid/segment/loading/TombstoneSegmentizerFactoryTest.java b/processing/src/test/java/org/apache/druid/segment/loading/TombstoneSegmentizerFactoryTest.java index 5ad3e3e05f0..557bf4b8dfc 100644 --- a/processing/src/test/java/org/apache/druid/segment/loading/TombstoneSegmentizerFactoryTest.java +++ b/processing/src/test/java/org/apache/druid/segment/loading/TombstoneSegmentizerFactoryTest.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableMap; import org.apache.druid.java.util.common.Intervals; import org.apache.druid.segment.QueryableIndex; import org.apache.druid.segment.Segment; +import org.apache.druid.segment.StorageAdapter; import org.apache.druid.timeline.DataSegment; import org.apache.druid.timeline.partition.TombstoneShardSpec; import org.joda.time.Interval; @@ -55,14 +56,17 @@ public class TombstoneSegmentizerFactoryTest QueryableIndex queryableIndex = segment.asQueryableIndex(); Assert.assertNotNull(queryableIndex); - assertThrows(UnsupportedOperationException.class, () -> queryableIndex.getNumRows()); - assertThrows(UnsupportedOperationException.class, () -> queryableIndex.getAvailableDimensions()); - assertThrows(UnsupportedOperationException.class, () -> queryableIndex.getBitmapFactoryForDimensions()); - assertThrows(UnsupportedOperationException.class, () -> queryableIndex.getMetadata()); - assertThrows(UnsupportedOperationException.class, () -> queryableIndex.getDimensionHandlers()); - assertThrows(UnsupportedOperationException.class, () -> queryableIndex.getColumnNames()); + assertThrows(UnsupportedOperationException.class, queryableIndex::getNumRows); + assertThrows(UnsupportedOperationException.class, queryableIndex::getAvailableDimensions); + assertThrows(UnsupportedOperationException.class, queryableIndex::getBitmapFactoryForDimensions); + assertThrows(UnsupportedOperationException.class, queryableIndex::getMetadata); + assertThrows(UnsupportedOperationException.class, queryableIndex::getDimensionHandlers); + assertThrows(UnsupportedOperationException.class, queryableIndex::getColumnNames); assertThrows(UnsupportedOperationException.class, () -> queryableIndex.getColumnHolder(null)); - Assert.assertTrue(queryableIndex.isFromTombstone()); + + StorageAdapter storageAdapter = segment.asStorageAdapter(); + Assert.assertNotNull(storageAdapter); + Assert.assertTrue(storageAdapter.isFromTombstone()); } } diff --git a/server/src/main/java/org/apache/druid/server/coordination/ServerManager.java b/server/src/main/java/org/apache/druid/server/coordination/ServerManager.java index fa670e04399..c932f1bcc09 100644 --- a/server/src/main/java/org/apache/druid/server/coordination/ServerManager.java +++ b/server/src/main/java/org/apache/druid/server/coordination/ServerManager.java @@ -275,11 +275,7 @@ public class ServerManager implements QuerySegmentWalker ) { - // Short-circuit when the index comes from a tombstone (it has no data by definition), - // check for null also since no all segments (higher level ones) will have QueryableIndex... - if (segment.asQueryableIndex() != null && segment.asQueryableIndex().isFromTombstone()) { - return new NoopQueryRunner<>(); - } + final SpecificSegmentSpec segmentSpec = new SpecificSegmentSpec(segmentDescriptor); final SegmentId segmentId = segment.getId(); @@ -290,6 +286,13 @@ public class ServerManager implements QuerySegmentWalker if (segmentId == null || segmentInterval == null) { return new ReportTimelineMissingSegmentQueryRunner<>(segmentDescriptor); } + + StorageAdapter storageAdapter = segment.asStorageAdapter(); + // Short-circuit when the index comes from a tombstone (it has no data by definition), + // check for null also since no all segments (higher level ones) will have QueryableIndex... + if (storageAdapter.isFromTombstone()) { + return new NoopQueryRunner<>(); + } String segmentIdString = segmentId.toString(); MetricsEmittingQueryRunner metricsEmittingQueryRunnerInner = new MetricsEmittingQueryRunner<>( @@ -300,7 +303,6 @@ public class ServerManager implements QuerySegmentWalker queryMetrics -> queryMetrics.segment(segmentIdString) ); - StorageAdapter storageAdapter = segment.asStorageAdapter(); long segmentMaxTime = storageAdapter.getMaxTime().getMillis(); long segmentMinTime = storageAdapter.getMinTime().getMillis(); Interval actualDataInterval = Intervals.utc(segmentMinTime, segmentMaxTime + 1); diff --git a/server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheLoaderTest.java b/server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheLoaderTest.java index 18d4b184bee..39a1e47129b 100644 --- a/server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheLoaderTest.java +++ b/server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheLoaderTest.java @@ -83,7 +83,7 @@ public class SegmentLocalCacheLoaderTest Assert.assertEquals(interval, segment.getDataInterval()); Assert.assertNotNull(segment.asStorageAdapter()); - Assert.assertTrue(segment.asQueryableIndex().isFromTombstone()); + Assert.assertTrue(segment.asStorageAdapter().isFromTombstone()); Assert.assertEquals(interval, segment.asQueryableIndex().getDataInterval()); Assert.assertThrows(UnsupportedOperationException.class, () -> segment.asQueryableIndex().getMetadata());