clintropolis commented on code in PR #18328:
URL: https://github.com/apache/druid/pull/18328#discussion_r2237936053
##########
processing/src/test/java/org/apache/druid/segment/TestIndex.java:
##########
@@ -221,51 +231,52 @@ public class TestIndex
ComplexMetrics.registerSerde(HyperUniquesSerde.TYPE_NAME, new
HyperUniquesSerde());
}
- private static Supplier<IncrementalIndex> realtimeIndex = Suppliers.memoize(
+ private static Supplier<IncrementalIndex> rtIndex = Suppliers.memoize(
Review Comment:
nit: why the name changes? I guess some are more concise, but more
descriptive variable names always feel better to me :shrug:
##########
processing/src/test/java/org/apache/druid/segment/projections/ProjectionsTest.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.projections;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.math.expr.ExprMacroTable;
+import org.apache.druid.query.Druids;
+import org.apache.druid.query.QueryContexts;
+import org.apache.druid.query.QueryRunnerTestHelper;
+import org.apache.druid.query.RestrictedDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.UnnestDataSource;
+import org.apache.druid.query.aggregation.CountAggregatorFactory;
+import org.apache.druid.query.aggregation.LongMaxAggregatorFactory;
+import org.apache.druid.query.dimension.DefaultDimensionSpec;
+import org.apache.druid.query.expression.TimestampFloorExprMacro;
+import org.apache.druid.query.expression.TimestampFormatExprMacro;
+import org.apache.druid.query.filter.SelectorDimFilter;
+import org.apache.druid.query.groupby.GroupByQuery;
+import org.apache.druid.query.policy.RowFilterPolicy;
+import org.apache.druid.query.timeseries.TimeseriesQuery;
+import org.apache.druid.query.topn.TopNQuery;
+import org.apache.druid.query.topn.TopNQueryBuilder;
+import org.apache.druid.segment.CursorBuildSpec;
+import org.apache.druid.segment.IncrementalIndexSegment;
+import org.apache.druid.segment.QueryableIndex;
+import org.apache.druid.segment.QueryableIndexSegment;
+import org.apache.druid.segment.ReferenceCountedSegmentProvider;
+import org.apache.druid.segment.Segment;
+import org.apache.druid.segment.TestIndex;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.incremental.IncrementalIndex;
+import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
+import org.apache.druid.testing.TestQueryRunnerKit;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import static org.mockito.Mockito.times;
Review Comment:
in general we've been trying to avoid using Mockito and other mock
frameworks unless absolutely necessary... is it really necessary for these
tests? a lot of these tests look like they could just be testing the match
function directly since they aren't actually scanning a cursor? is this more
tedious than i'm imagining?
##########
processing/src/test/java/org/apache/druid/segment/projections/ProjectionsTest.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.projections;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.math.expr.ExprMacroTable;
+import org.apache.druid.query.Druids;
+import org.apache.druid.query.QueryContexts;
+import org.apache.druid.query.QueryRunnerTestHelper;
+import org.apache.druid.query.RestrictedDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.UnnestDataSource;
+import org.apache.druid.query.aggregation.CountAggregatorFactory;
+import org.apache.druid.query.aggregation.LongMaxAggregatorFactory;
+import org.apache.druid.query.dimension.DefaultDimensionSpec;
+import org.apache.druid.query.expression.TimestampFloorExprMacro;
+import org.apache.druid.query.expression.TimestampFormatExprMacro;
+import org.apache.druid.query.filter.SelectorDimFilter;
+import org.apache.druid.query.groupby.GroupByQuery;
+import org.apache.druid.query.policy.RowFilterPolicy;
+import org.apache.druid.query.timeseries.TimeseriesQuery;
+import org.apache.druid.query.topn.TopNQuery;
+import org.apache.druid.query.topn.TopNQueryBuilder;
+import org.apache.druid.segment.CursorBuildSpec;
+import org.apache.druid.segment.IncrementalIndexSegment;
+import org.apache.druid.segment.QueryableIndex;
+import org.apache.druid.segment.QueryableIndexSegment;
+import org.apache.druid.segment.ReferenceCountedSegmentProvider;
+import org.apache.druid.segment.Segment;
+import org.apache.druid.segment.TestIndex;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.incremental.IncrementalIndex;
+import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
+import org.apache.druid.testing.TestQueryRunnerKit;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import static org.mockito.Mockito.times;
+
+public class ProjectionsTest
+{
+ private static final TestQueryRunnerKit QUERY_RUNNER_KIT =
TestQueryRunnerKit.DEFAULT;
+ private static final Map<String, Object> NO_PROJECTIONS_CONTEXT =
Map.of(QueryContexts.NO_PROJECTIONS, "true");
+ private static final Map<String, Object> FORCE_PROJECTION_CONTEXT =
Map.of(QueryContexts.FORCE_PROJECTION, "true");
+
+ private static List<Arguments> getAllQueryableIndex()
+ {
+ return TestIndex.queryableIndexSupplierMap(true)
+ .entrySet()
+ .stream()
+ .map(entry -> {
+ // return a mmaped index that can be spied on and its
fileMapper can't be closed by the test framework.
+ QueryableIndex openIndex =
Mockito.spy(entry.getValue().get());
+ Mockito.doNothing().when(openIndex).close();
Review Comment:
what is closing stuff? (why is this needed?)
##########
processing/src/test/java/org/apache/druid/segment/TestIndex.java:
##########
@@ -221,51 +231,52 @@ public class TestIndex
ComplexMetrics.registerSerde(HyperUniquesSerde.TYPE_NAME, new
HyperUniquesSerde());
}
- private static Supplier<IncrementalIndex> realtimeIndex = Suppliers.memoize(
+ private static Supplier<IncrementalIndex> rtIndex = Suppliers.memoize(
TestIndex::makeSampleNumericIncrementalIndex
);
- private static Supplier<IncrementalIndex>
realtimeIndexPartialSchemaLegacyStringDiscovery = Suppliers.memoize(
- () -> fromJsonResource(
+ // PROJECTIONS are defined on string dimensions, they can't be used with
this index
+ private static Supplier<IncrementalIndex>
rtPartialSchemaStringDiscoveryIndex = Suppliers.memoize(
+ () -> fromJsonResourceWithNoProjection(
SAMPLE_NUMERIC_JSON,
true,
DIMENSIONS_SPEC_PARTIAL_NO_STRINGS
)
);
- private static Supplier<IncrementalIndex> nonTimeOrderedRealtimeIndex =
Suppliers.memoize(
+ private static Supplier<IncrementalIndex> nonTimeOrderedRtIndex =
Suppliers.memoize(
() -> fromJsonResource(SAMPLE_NUMERIC_JSON, true,
DIMENSIONS_SPEC_NON_TIME_ORDERED)
);
- private static Supplier<IncrementalIndex>
nonTimeOrderedNoRollupRealtimeIndex = Suppliers.memoize(
+ private static Supplier<IncrementalIndex> nonTimeOrderedNoRollupRtIndex =
Suppliers.memoize(
() -> fromJsonResource(SAMPLE_NUMERIC_JSON, false,
DIMENSIONS_SPEC_NON_TIME_ORDERED)
);
- private static Supplier<IncrementalIndex> noRollupRealtimeIndex =
Suppliers.memoize(
+ private static Supplier<IncrementalIndex> noRollupRtIndex =
Suppliers.memoize(
() -> fromJsonResource(SAMPLE_NUMERIC_JSON, false, DIMENSIONS_SPEC)
);
- private static Supplier<IncrementalIndex> noBitmapRealtimeIndex =
Suppliers.memoize(
+ private static Supplier<IncrementalIndex> noBitmapRtIndex =
Suppliers.memoize(
() -> fromJsonResource(SAMPLE_NUMERIC_JSON, false,
DIMENSIONS_SPEC_NO_BITMAPS)
);
- private static Supplier<QueryableIndex> mmappedIndex = Suppliers.memoize(
+ private static Supplier<QueryableIndex> mMappedIndex = Suppliers.memoize(
Review Comment:
nit: mmap seems more correct than mMap because the sys call is mmap
##########
processing/src/test/java/org/apache/druid/query/timeseries/TimeseriesQueryRunnerTest.java:
##########
@@ -2024,6 +2028,9 @@ public void
testTimeseriesWithMultiValueFilteringJavascriptAggregatorAndAlsoRegu
@Test
public void testTimeseriesWithFirstLastAggregator()
{
+ // some string dimensions are not included, and it changes the results
+
Assume.assumeFalse(runner.toString().equals("rtPartialSchemaStringDiscoveryIndex"));
+
Review Comment:
why is this needed? i think `rtPartialSchemaStringDiscovery` was intended to
have `includeAllDimensions` set to true and so should still pick up the
dimensions that are not explicitly defined. is this a side-effect of adding
some projections to stuff?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
