github-actions[bot] commented on code in PR #66734: URL: https://github.com/apache/doris/pull/66734#discussion_r3774832080
########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/generator/Stack.java: ########## @@ -0,0 +1,146 @@ +// 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.doris.nereids.trees.expressions.functions.generator; + +import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.functions.AlwaysNullable; +import org.apache.doris.nereids.trees.expressions.functions.ComputePrecision; +import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; +import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral; +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.IntegerType; +import org.apache.doris.nereids.types.NullType; +import org.apache.doris.nereids.types.StructField; +import org.apache.doris.nereids.types.StructType; +import org.apache.doris.nereids.util.ExpressionUtils; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; + +import java.util.ArrayList; +import java.util.List; + +/** + * stack(n, expr1, ..., exprk) separates the expressions into n rows in row-major order. + * Missing values in the last row are padded with nulls. + */ +public class Stack extends TableGeneratingFunction implements CustomSignature, ComputePrecision, AlwaysNullable { + + /** constructor with two or more arguments. */ + public Stack(Expression numRows, Expression argument, Expression... otherArguments) { + super("stack", ExpressionUtils.mergeArguments(numRows, argument, otherArguments)); + } + + /** constructor for withChildren and reuse signature. */ + private Stack(GeneratorFunctionParams functionParams) { + super(functionParams); + } + + @Override + public Stack withChildren(List<Expression> children) { + Preconditions.checkArgument(children.size() >= 2); + return new Stack(getFunctionParams(children)); + } + + @Override + public void checkLegalityBeforeTypeCoercion() { + getColumnTypes(); + } + + @Override + public FunctionSignature computePrecision(FunctionSignature signature) { + return signature; + } + + @Override + public FunctionSignature searchSignature(List<FunctionSignature> signatures) { + return super.searchSignature(signatures); + } + + @Override + public FunctionSignature customSignature() { + List<DataType> columnTypes = getColumnTypes(); + List<DataType> argumentTypes = new ArrayList<>(arity()); + argumentTypes.add(IntegerType.INSTANCE); + for (int i = 1; i < arity(); i++) { + argumentTypes.add(columnTypes.get((i - 1) % columnTypes.size())); + } + + if (columnTypes.size() == 1) { + return FunctionSignature.of(columnTypes.get(0), argumentTypes); + } + ImmutableList.Builder<StructField> fields = ImmutableList.builder(); + for (int i = 0; i < columnTypes.size(); i++) { + fields.add(new StructField("col" + i, columnTypes.get(i), true, "")); + } + return FunctionSignature.of(new StructType(fields.build()), argumentTypes); + } + + private int getNumRows() { + Expression numRowsArgument = getArgument(0); + Expression evaluated = FoldConstantRuleOnFE.evaluate(numRowsArgument, null); Review Comment: [P1] Make stack's constant evaluation honor its input contract This null-context fold call violates both sides of the row-count contract. A nonconstant `connection_id()` dispatches to `FoldConstantRuleOnFE.visitConnectionId()`, which dereferences `context.cascadesContext`, so it throws an internal NPE before the documented analysis error. Conversely, the valid constant integer `cardinality([1, 2])` is `isConstant()`, but `ExpressionEvaluator` has no `cardinality` method; folding returns the original expression and stack rejects it as nonconstant. An `isConstant()` guard alone fixes only the crash. Please reject nonconstants before invoking the visitor, then evaluate supported constant integer expressions through a path that reliably reduces them; cover both cases. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/generator/Stack.java: ########## @@ -0,0 +1,146 @@ +// 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.doris.nereids.trees.expressions.functions.generator; + +import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.functions.AlwaysNullable; +import org.apache.doris.nereids.trees.expressions.functions.ComputePrecision; +import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; +import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral; +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.IntegerType; +import org.apache.doris.nereids.types.NullType; +import org.apache.doris.nereids.types.StructField; +import org.apache.doris.nereids.types.StructType; +import org.apache.doris.nereids.util.ExpressionUtils; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; + +import java.util.ArrayList; +import java.util.List; + +/** + * stack(n, expr1, ..., exprk) separates the expressions into n rows in row-major order. + * Missing values in the last row are padded with nulls. + */ +public class Stack extends TableGeneratingFunction implements CustomSignature, ComputePrecision, AlwaysNullable { + + /** constructor with two or more arguments. */ + public Stack(Expression numRows, Expression argument, Expression... otherArguments) { + super("stack", ExpressionUtils.mergeArguments(numRows, argument, otherArguments)); + } + + /** constructor for withChildren and reuse signature. */ + private Stack(GeneratorFunctionParams functionParams) { + super(functionParams); + } + + @Override + public Stack withChildren(List<Expression> children) { + Preconditions.checkArgument(children.size() >= 2); + return new Stack(getFunctionParams(children)); + } + + @Override + public void checkLegalityBeforeTypeCoercion() { + getColumnTypes(); + } + + @Override + public FunctionSignature computePrecision(FunctionSignature signature) { + return signature; + } + + @Override + public FunctionSignature searchSignature(List<FunctionSignature> signatures) { + return super.searchSignature(signatures); + } + + @Override + public FunctionSignature customSignature() { + List<DataType> columnTypes = getColumnTypes(); + List<DataType> argumentTypes = new ArrayList<>(arity()); + argumentTypes.add(IntegerType.INSTANCE); + for (int i = 1; i < arity(); i++) { + argumentTypes.add(columnTypes.get((i - 1) % columnTypes.size())); + } + + if (columnTypes.size() == 1) { + return FunctionSignature.of(columnTypes.get(0), argumentTypes); + } + ImmutableList.Builder<StructField> fields = ImmutableList.builder(); + for (int i = 0; i < columnTypes.size(); i++) { + fields.add(new StructField("col" + i, columnTypes.get(i), true, "")); + } + return FunctionSignature.of(new StructType(fields.build()), argumentTypes); Review Comment: [P1] Validate the dynamic stack alias count before expansion `stack` can derive a different number of struct fields for each call, but the generic binder later iterates every field and indexes the supplied alias list without checking its size. For example, `lateral view stack(2, 1, 2, 3, 4, 5) s as a, b` derives three fields and reaches `aliases.get(2)`, producing an internal `IndexOutOfBoundsException`; supplying four aliases silently drops the fourth. Please validate the alias count against the derived stack schema (while preserving the intended one-alias whole-result form if required) and return an `AnalysisException`; add short/long cardinality tests. ########## be/src/exprs/table_function/vstack.cpp: ########## @@ -0,0 +1,132 @@ +// 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. + +#include "exprs/table_function/vstack.h" + +#include <algorithm> + +#include "core/assert_cast.h" +#include "core/block/block.h" +#include "core/column/column_const.h" +#include "core/column/column_nullable.h" +#include "core/column/column_struct.h" +#include "exprs/vexpr.h" + +namespace doris { + +VStackTableFunction::VStackTableFunction() { + _fn_name = "stack"; +} + +Status VStackTableFunction::process_init(Block* block, RuntimeState* /*state*/) { + const auto& children = _expr_context->root()->children(); + DORIS_CHECK_GE(children.size(), 2); + + int column_index = -1; + RETURN_IF_ERROR(children[0]->execute(_expr_context.get(), block, &column_index)); + const auto& num_rows_column = block->get_by_position(column_index).column; + DORIS_CHECK(is_column_const(*num_rows_column)); + const auto num_rows = assert_cast<const ColumnConst&>(*num_rows_column).get_int(0); + DORIS_CHECK_GT(num_rows, 0); + _num_rows = static_cast<size_t>(num_rows); + _num_fields = (children.size() - 2) / _num_rows + 1; + + _value_columns.clear(); + _value_columns.reserve(children.size() - 1); + for (size_t i = 1; i < children.size(); ++i) { + RETURN_IF_ERROR(children[i]->execute(_expr_context.get(), block, &column_index)); + _value_columns.emplace_back( + block->get_by_position(column_index).column->convert_to_full_column_if_const()); Review Comment: [P1] Keep constant stack arguments compact per input block `convert_to_full_column_if_const()` expands each literal to `block->rows()` physical values and `_value_columns` retains all of them until the child block is exhausted. Literal-heavy stack calls therefore multiply live memory/CPU by `input_rows * constant_arguments * value_size` (for example, 100 constant 1 KiB strings over 4096 rows is about 400 MiB of duplicated source payload). Preserve/unpack the const column and read row 0 for constants versus `_row_idx` for ordinary columns; add a multi-row constant-value case so the common constant form does not amplify per block. -- 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]
