mayankshriv commented on a change in pull request #8029: URL: https://github.com/apache/pinot/pull/8029#discussion_r785350006
########## File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BlockValSetImpl.java ########## @@ -0,0 +1,171 @@ +/** + * 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.query.reduce; + +import java.util.List; +import javax.annotation.Nullable; +import org.apache.pinot.common.utils.DataSchema; +import org.apache.pinot.core.common.BlockValSet; +import org.apache.pinot.segment.spi.index.reader.Dictionary; +import org.apache.pinot.spi.data.FieldSpec; + + +/** + * Helper class to convert the result rows to BlockValSet. + */ +@SuppressWarnings({"rawtypes", "unchecked"}) +public class BlockValSetImpl implements BlockValSet { + + private final FieldSpec.DataType _dataType; + private final List<Object[]> _rows; + private final int _columnIndex; + + public BlockValSetImpl(DataSchema.ColumnDataType columnDataType, List<Object[]> rows, int columnIndex) { + _dataType = columnDataType.toDataType(); + _rows = rows; + _columnIndex = columnIndex; + } + + @Override + public FieldSpec.DataType getValueType() { + return _dataType; + } + + @Override + public boolean isSingleValue() { + return true; + } + + @Nullable + @Override + public Dictionary getDictionary() { + throw new UnsupportedOperationException("Not supported"); + } + + @Override + public int[] getDictionaryIdsSV() { + throw new UnsupportedOperationException("Not supported"); + } + + @Override + public int[] getIntValuesSV() { + if (_dataType == FieldSpec.DataType.INT) { + int [] result = new int[_rows.size()]; + for (int i = 0; i < result.length; i++) { + result[i] = (Integer) _rows.get(i)[_columnIndex]; + } + return result; + } + throw new UnsupportedOperationException("Not supported"); + } + + @Override + public long[] getLongValuesSV() { + if (_dataType == FieldSpec.DataType.LONG) { + long [] result = new long[_rows.size()]; + for (int i = 0; i < result.length; i++) { + result[i] = (Long) _rows.get(i)[_columnIndex]; + } + return result; + } + throw new UnsupportedOperationException("Not supported"); Review comment: IIRC, other parts of the code allow for read `int` as `long` and other such upcasting? ########## File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BlockValSetImpl.java ########## @@ -0,0 +1,171 @@ +/** + * 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.query.reduce; + +import java.util.List; +import javax.annotation.Nullable; +import org.apache.pinot.common.utils.DataSchema; +import org.apache.pinot.core.common.BlockValSet; +import org.apache.pinot.segment.spi.index.reader.Dictionary; +import org.apache.pinot.spi.data.FieldSpec; + + +/** + * Helper class to convert the result rows to BlockValSet. + */ +@SuppressWarnings({"rawtypes", "unchecked"}) +public class BlockValSetImpl implements BlockValSet { Review comment: Could we choose a better name for the class and also add more java docs. For example, unclear to me how it is different from other impls of `BlockValSet` without having to read the code. ########## File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java ########## @@ -117,21 +117,50 @@ private static String removeTerminatingSemicolon(String sql) { return sql; } + private static SqlNode parse(String sql) { + SqlParser sqlParser = SqlParser.create(sql, PARSER_CONFIG); + try { + return sqlParser.parseQuery(); + } catch (SqlParseException e) { + throw new SqlCompilationException("Caught exception while parsing query: " + sql, e); + } + } + + public static PinotQuery compileToPinotQueryWithSubquery(String sql) + throws SqlCompilationException { + return compileToPinotQuery(sql, true); + } + public static PinotQuery compileToPinotQuery(String sql) throws SqlCompilationException { - // Remove the comments from the query - sql = removeComments(sql); + return compileToPinotQuery(sql, false); + } - // Remove the terminating semicolon from the query + private static PinotQuery compileToPinotQuery(String sql, boolean enablePreAggregateGapfillQuery) + throws SqlCompilationException { + // Removes the terminating semicolon if any sql = removeTerminatingSemicolon(sql); // Extract OPTION statements from sql as Calcite Parser doesn't parse it. List<String> options = extractOptionsFromSql(sql); if (!options.isEmpty()) { sql = removeOptionsFromSql(sql); } + + SqlNode sqlNode = parse(sql); + // Compile Sql without OPTION statements. - PinotQuery pinotQuery = compileCalciteSqlToPinotQuery(sql); + PinotQuery pinotQuery = compileSqlNodeToPinotQuery(sqlNode); + + if (enablePreAggregateGapfillQuery) { Review comment: Can we avoid this special-casing? We already have the `IN_SUBQUERY` feature: https://docs.pinot.apache.org/users/user-guide-query/filtering-with-idset#in_subquery ########## File path: pinot-core/src/main/java/org/apache/pinot/core/plan/PreAggGapFillSelectionPlanNode.java ########## @@ -0,0 +1,86 @@ +/** + * 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 com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.List; +import org.apache.pinot.common.request.context.ExpressionContext; +import org.apache.pinot.core.common.Operator; +import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock; +import org.apache.pinot.core.operator.query.SelectionOnlyOperator; +import org.apache.pinot.core.operator.transform.TransformOperator; +import org.apache.pinot.core.query.request.context.QueryContext; +import org.apache.pinot.core.util.GapfillUtils; +import org.apache.pinot.segment.spi.IndexSegment; + + +/** + * The <code>PreAggGapFillSelectionPlanNode</code> class provides the execution + * plan for pre-aggregate gapfill query on a single segment. + */ +public class PreAggGapFillSelectionPlanNode implements PlanNode { + private final IndexSegment _indexSegment; + private final QueryContext _queryContext; + + public PreAggGapFillSelectionPlanNode(IndexSegment indexSegment, QueryContext queryContext) { + _indexSegment = indexSegment; + _queryContext = queryContext.getPreAggregateGapFillQueryContext(); + } + + @Override + public Operator<IntermediateResultsBlock> run() { + int limit = _queryContext.getLimit(); + + ExpressionContext gapFillSelection = null; + for (ExpressionContext expressionContext : _queryContext.getSelectExpressions()) { + if (GapfillUtils.isPreAggregateGapfill(expressionContext)) { + gapFillSelection = expressionContext; + break; + } + } + + List<ExpressionContext> args = gapFillSelection.getFunction().getArguments(); + ExpressionContext timeSeriesOn = null; + for (int i = 5; i < args.size(); i++) { Review comment: The use of `5` hear reads like a magic number. Can we make it more readable and/or add comments? -- 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
