Jackie-Jiang commented on code in PR #11886: URL: https://github.com/apache/pinot/pull/11886#discussion_r1391851287
########## pinot-core/src/main/java/org/apache/pinot/core/operator/filter/CombinedFilteredAggregationContext.java: ########## @@ -0,0 +1,78 @@ +/** + * 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.pinot.core.operator.filter; + +import java.util.ArrayList; +import java.util.List; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import org.apache.commons.lang3.tuple.Pair; +import org.apache.pinot.common.request.context.FilterContext; +import org.apache.pinot.common.request.context.predicate.Predicate; +import org.apache.pinot.core.operator.filter.predicate.PredicateEvaluator; +import org.apache.pinot.core.query.aggregation.function.AggregationFunction; + + +public class CombinedFilteredAggregationContext { + private final BaseFilterOperator _baseFilterOperator; + private final FilterContext _mainFilterContext; + private final FilterContext _subFilterContext; + private final List<Pair<Predicate, PredicateEvaluator>> _predicateEvaluators; + private final List<AggregationFunction> _aggregationFunctions; + + public CombinedFilteredAggregationContext(BaseFilterOperator baseFilterOperator, + List<Pair<Predicate, PredicateEvaluator>> predicateEvaluators, @Nullable FilterContext mainFilterContext, + @Nonnull FilterContext subFilterContext, List<AggregationFunction> aggregationFunctions) { Review Comment: (minor, convention) We don't usually use `@Nonnull`, but only annotate `@Nullable` and assume everything else as non-null ########## pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/StarTreeClusterIntegrationTest.java: ########## @@ -38,7 +38,9 @@ import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; +import static org.junit.Assert.assertFalse; Review Comment: Wrong import ########## pinot-core/src/main/java/org/apache/pinot/core/operator/query/OperatorUtils.java: ########## @@ -0,0 +1,89 @@ +/** + * 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.pinot.core.operator.query; + +import java.util.List; +import java.util.Map; +import java.util.Set; +import javax.annotation.Nullable; +import org.apache.commons.lang3.tuple.Pair; +import org.apache.pinot.common.request.context.ExpressionContext; +import org.apache.pinot.common.request.context.FilterContext; +import org.apache.pinot.common.request.context.predicate.Predicate; +import org.apache.pinot.core.operator.filter.BaseFilterOperator; +import org.apache.pinot.core.operator.filter.predicate.PredicateEvaluator; +import org.apache.pinot.core.plan.DocIdSetPlanNode; +import org.apache.pinot.core.plan.ProjectPlanNode; +import org.apache.pinot.core.plan.ProjectionPlanNode; +import org.apache.pinot.core.query.aggregation.function.AggregationFunction; +import org.apache.pinot.core.query.aggregation.function.AggregationFunctionUtils; +import org.apache.pinot.core.query.request.context.QueryContext; +import org.apache.pinot.core.startree.CompositePredicateEvaluator; +import org.apache.pinot.core.startree.StarTreeUtils; +import org.apache.pinot.core.startree.plan.StarTreeProjectPlanNode; +import org.apache.pinot.segment.spi.IndexSegment; +import org.apache.pinot.segment.spi.index.startree.AggregationFunctionColumnPair; +import org.apache.pinot.segment.spi.index.startree.StarTreeV2; + + +public class OperatorUtils { + //TODO(egalpin): This should be merged with org.apache.pinot.core.operator.ProjectionOperatorUtils + private OperatorUtils() { + // Prevent instantiation, make checkstyle happy + } + + public static ProjectionPlanNode maybeGetStartreeProjectionOperator( Review Comment: I'd suggest moving this into the `StarTreeUtils` as ``` @Nullable public static BaseProjectOperator<?> createStarTreeBasedProjectOperator(...) ``` It returns `null` when star-tree cannot be used. This way we don't need to worry about using `instanceof` to identify whether it is star-tree based, and we don't need to create `PlanNode` out of `BaseFilterOperator` which should be avoided ########## pinot-core/src/main/java/org/apache/pinot/core/operator/filter/CombinedFilteredAggregationContext.java: ########## @@ -0,0 +1,78 @@ +/** + * 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.pinot.core.operator.filter; + +import java.util.ArrayList; +import java.util.List; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import org.apache.commons.lang3.tuple.Pair; +import org.apache.pinot.common.request.context.FilterContext; +import org.apache.pinot.common.request.context.predicate.Predicate; +import org.apache.pinot.core.operator.filter.predicate.PredicateEvaluator; +import org.apache.pinot.core.query.aggregation.function.AggregationFunction; + + +public class CombinedFilteredAggregationContext { Review Comment: This is used as a inner helper wrapper class for `AggregationFunctionUtils.buildFilteredAggregateProjectOperators()`. Suggest making it a private inner class, and we can directly access the fields within the class ########## pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java: ########## @@ -1419,6 +1442,57 @@ public void testStarTreeTriggering() assertEquals(secondQueryResponse.get("resultTable").get("rows").get(0).get(0).asInt(), secondQueryResult); assertEquals(secondQueryResponse.get("totalDocs").asLong(), numTotalDocs); assertEquals(secondQueryResponse.get("numDocsScanned").asInt(), secondQueryResult); + + // Enforce a sleep here since segment reload is async and there is another back-to-back reload below. + // Otherwise, there is no way to tell whether the 1st reload on server side is finished, + // which may hit the race condition that the 1st reload finishes after the 2nd reload is fully done. + // 10 seconds are still better than hitting race condition which will time out after 10 minutes. + Thread.sleep(10_000L); Review Comment: We should avoid this kind of wait because it will waste resource on fast machine but flaky on slow machine. We don't need to add test in star-tree triggering test because the intention for this test is to just verify star-tree is created. We should add more tests into the `StarTreeClusterIntegrationTest` ########## pinot-core/src/main/java/org/apache/pinot/core/plan/ProjectionPlanNode.java: ########## @@ -0,0 +1,33 @@ +/** + * 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.pinot.core.plan; + +import org.apache.pinot.core.operator.BaseProjectOperator; + + +public interface ProjectionPlanNode extends PlanNode { Review Comment: Suggest not adding this interface. We can do cast if necessary -- 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
