github-actions[bot] commented on code in PR #66968: URL: https://github.com/apache/doris/pull/66968#discussion_r3862566647
########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/MapLambdaFunctionUtils.java: ########## @@ -0,0 +1,129 @@ +// 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.scalar; + +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.trees.expressions.ArrayItemReference; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.Slot; +import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral; +import org.apache.doris.nereids.types.ArrayType; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.MapType; +import org.apache.doris.nereids.types.StructField; +import org.apache.doris.nereids.types.StructType; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** Utilities for building Map Lambda functions from one bound Map-entry array driver. */ +final class MapLambdaFunctionUtils { + + private MapLambdaFunctionUtils() { + } + + /** Build the function-specific body around a bound Map-entry Lambda. */ + static RewrittenMapLambda rewrite(Lambda lambda, EntryBodyBuilder bodyBuilder) { + Expression mapExpression = extractMapExpression(lambda); + List<ArrayItemReference> arguments = lambda.getLambdaArguments(); + ArrayItemReference entryArgument = arguments.get(0); + Slot entrySlot = entryArgument.toSlot(); + Expression key = new ElementAt(entrySlot, new IntegerLiteral(1)); + Expression value = new ElementAt(entrySlot, new IntegerLiteral(2)); + + Expression rewrittenBody = bodyBuilder.build(lambda.getLambdaFunction(), key, value, entrySlot); + Lambda rewrittenLambda = new Lambda( + ImmutableList.of(entryArgument.getName()), rewrittenBody, ImmutableList.of(entryArgument)); + return new RewrittenMapLambda(mapExpression, rewrittenLambda); + } + + /** Require a bound Lambda argument. */ + static Lambda requireLambda(String functionName, Expression expression) { + if (!(expression instanceof Lambda)) { + throw new AnalysisException(String.format( + "The 1st arg of %s must be lambda but is %s", functionName, expression)); + } + return (Lambda) expression; + } + + /** Fill only NULL_TYPE positions from the corresponding input Map field type. */ + static DataType mergeNestedNullTypes(DataType outputType, DataType inputType) { Review Comment: [P1] Normalize every residual NullType before Map validation This helper only fills `NullType` where output and input container shapes match, and some callers do not pass retained fields through it at all. For example, `transform_values((k,v) -> [], map(1,10))` leaves `ARRAY<NULL_TYPE>`, while `transform_keys((k,v) -> k, map(1,null))` carries a Null-typed value directly. Both wrappers call `validateDataType()` before the lowered `MapFromEntries` can apply its recursive `TINYINT` fallback, so valid advertised expressions fail analysis. Normalize all residual nested `NullType` fields across the complete result before validation, and cover mismatched shapes plus empty/untyped Maps. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/MapFilter.java: ########## @@ -0,0 +1,112 @@ +// 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.scalar; + +import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable; +import org.apache.doris.nereids.trees.expressions.functions.RewriteWhenAnalyze; +import org.apache.doris.nereids.trees.expressions.literal.NullLiteral; +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.ArrayType; +import org.apache.doris.nereids.types.BooleanType; +import org.apache.doris.nereids.types.MapType; +import org.apache.doris.nereids.types.coercion.AnyDataType; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** + * Scalar function map_filter. + * + * <p>The Map lambda is evaluated by an ArrayMap over the Map's entry array: + * + * <pre> + * map_filter((mapKey, mapValue) -> predicate, inputMap) + * -> + * %map_from_filtered_entries_unique%( + * array_map( + * entry -> if(predicate(entry[1], entry[2]), entry, null), + * map_entries(inputMap))) + * </pre> + */ +public class MapFilter extends ScalarFunction + implements HighOrderFunction, PropagateNullable, RewriteWhenAnalyze { + public static final List<FunctionSignature> SIGNATURES = ImmutableList.of( + FunctionSignature.retArgType(0).args( + MapType.of(new AnyDataType(0), new AnyDataType(1)), + ArrayType.of(BooleanType.INSTANCE))); + private static final List<FunctionSignature> MAP_LAMBDA_SIGNATURES = ImmutableList.of( + FunctionSignature.retArgType(0).args( + MapType.of(new AnyDataType(0), new AnyDataType(1)), + ArrayType.of(new AnyDataType(2)))); + + private final boolean validateMapLambdaInput; + + // The argument is a bound Lambda. + public MapFilter(Expression arg) { + this(MapLambdaFunctionUtils.requireLambda("map_filter", arg)); + } + + public MapFilter(Expression map, Expression filter) { + super("map_filter", map, filter); + validateMapLambdaInput = false; + } + + private MapFilter(Lambda lambda) { + this(MapLambdaFunctionUtils.rewrite(lambda, + (body, key, value, entry) -> new If( + body, entry, new NullLiteral(entry.getDataType())))); + } + + private MapFilter(MapLambdaFunctionUtils.RewrittenMapLambda rewrittenLambda) { + super("map_filter", + rewrittenLambda.getMapExpression(), rewrittenLambda.toArrayMap()); + validateMapLambdaInput = true; + } + + private MapFilter(ScalarFunctionParams functionParams, boolean validateMapLambdaInput) { + super(functionParams); + this.validateMapLambdaInput = validateMapLambdaInput; + } + + @Override + public MapFilter withChildren(List<Expression> children) { + Preconditions.checkArgument(children.size() == 2); + return new MapFilter(getFunctionParams(children), validateMapLambdaInput); + } + + @Override + public List<FunctionSignature> getImplSignature() { + return validateMapLambdaInput ? MAP_LAMBDA_SIGNATURES : SIGNATURES; + } + + @Override + public Expression rewriteWhenAnalyze() { + return validateMapLambdaInput + ? new MapFromFilteredEntriesUnique(getArgument(1)) Review Comment: [P1] Coerce the internal constructor introduced by this rewrite For `map_filter((k,v) -> true, map(null,10))`, wrapper coercion leaves the mapped child as `ARRAY<STRUCT<NULL_TYPE,TINYINT>>` because `AnyDataType` accepts it. This line creates `MapFromFilteredEntriesUnique` only after `processBoundFunction()` has finished; the inherited signature normalizes the key to `TINYINT`, but the actual child never gets cast. FE then serializes a normalized result with Null-typed physical entries, while BE constructs the Map from the actual Struct fields, so the descriptor and column disagree. Reanalyze/coerce the rewritten internal function or give the wrapper the exact resolved entry signature, and add null-key/value/empty-Map parity tests. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/MapLambdaFunctionUtils.java: ########## @@ -0,0 +1,129 @@ +// 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.scalar; + +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.trees.expressions.ArrayItemReference; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.Slot; +import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral; +import org.apache.doris.nereids.types.ArrayType; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.MapType; +import org.apache.doris.nereids.types.StructField; +import org.apache.doris.nereids.types.StructType; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** Utilities for building Map Lambda functions from one bound Map-entry array driver. */ +final class MapLambdaFunctionUtils { + + private MapLambdaFunctionUtils() { + } + + /** Build the function-specific body around a bound Map-entry Lambda. */ + static RewrittenMapLambda rewrite(Lambda lambda, EntryBodyBuilder bodyBuilder) { + Expression mapExpression = extractMapExpression(lambda); + List<ArrayItemReference> arguments = lambda.getLambdaArguments(); + ArrayItemReference entryArgument = arguments.get(0); + Slot entrySlot = entryArgument.toSlot(); + Expression key = new ElementAt(entrySlot, new IntegerLiteral(1)); + Expression value = new ElementAt(entrySlot, new IntegerLiteral(2)); + + Expression rewrittenBody = bodyBuilder.build(lambda.getLambdaFunction(), key, value, entrySlot); + Lambda rewrittenLambda = new Lambda( + ImmutableList.of(entryArgument.getName()), rewrittenBody, ImmutableList.of(entryArgument)); + return new RewrittenMapLambda(mapExpression, rewrittenLambda); + } + + /** Require a bound Lambda argument. */ + static Lambda requireLambda(String functionName, Expression expression) { + if (!(expression instanceof Lambda)) { + throw new AnalysisException(String.format( + "The 1st arg of %s must be lambda but is %s", functionName, expression)); + } + return (Lambda) expression; + } + + /** Fill only NULL_TYPE positions from the corresponding input Map field type. */ + static DataType mergeNestedNullTypes(DataType outputType, DataType inputType) { + if (outputType.isNullType()) { + return inputType; + } else if (outputType instanceof ArrayType && inputType instanceof ArrayType) { + return ArrayType.of(mergeNestedNullTypes( + ((ArrayType) outputType).getItemType(), ((ArrayType) inputType).getItemType())); + } else if (outputType instanceof MapType && inputType instanceof MapType) { + return MapType.of( + mergeNestedNullTypes( + ((MapType) outputType).getKeyType(), ((MapType) inputType).getKeyType()), + mergeNestedNullTypes( + ((MapType) outputType).getValueType(), ((MapType) inputType).getValueType())); + } else if (outputType instanceof StructType && inputType instanceof StructType) { + List<StructField> outputFields = ((StructType) outputType).getFields(); + List<StructField> inputFields = ((StructType) inputType).getFields(); + if (outputFields.size() != inputFields.size()) { + return outputType; + } + ImmutableList.Builder<StructField> fields + = ImmutableList.builderWithExpectedSize(outputFields.size()); + for (int i = 0; i < outputFields.size(); i++) { + fields.add(outputFields.get(i).withDataType(mergeNestedNullTypes( + outputFields.get(i).getDataType(), inputFields.get(i).getDataType()))); + } + return new StructType(fields.build()); + } + return outputType; + } + + private static Expression extractMapExpression(Lambda lambda) { + List<ArrayItemReference> arguments = lambda.getLambdaArguments(); + Preconditions.checkArgument(arguments.size() == 1, + "A bound Map Lambda must have one entry argument"); + Expression entries = arguments.get(0).getArrayExpression(); + Preconditions.checkArgument(entries instanceof MapEntries, + "A bound Map Lambda must use a map_entries argument"); + return entries.child(0); + } + + @FunctionalInterface + interface EntryBodyBuilder { + Expression build(Expression body, Expression key, Expression value, Slot entry); + } + + /** One original Map and its one-driver entry Lambda. */ + static final class RewrittenMapLambda { + private final Expression mapExpression; + private final Lambda lambda; + + private RewrittenMapLambda(Expression mapExpression, Lambda lambda) { + this.mapExpression = mapExpression; + this.lambda = lambda; + } + + Expression getMapExpression() { + return mapExpression; + } + + ArrayMap toArrayMap() { + return new ArrayMap(lambda); Review Comment: [P1] Do not evaluate entries hidden under null Map rows `MapEntries`' default nullable wrapper preserves nested payload and offsets for a null row, and `ArrayMap` executes the complete nested column before attaching the outer null map. A reachable `map_from_arrays(nullif([id],[1]), [id * 10])` keeps payload under `id=1`; `transform_values((k,v) -> assert_true(k > 1, 'bad'), ...)` then throws for a row whose output should be NULL. All six new Lambda forms lower through `toArrayMap()`. Remove null-row ranges before invoking the lambda (or make `ArrayMap` skip them), and add a mixed null/non-null payload regression. ########## be/src/exprs/function/function_map.cpp: ########## @@ -181,6 +181,124 @@ class FunctionMapFromArrays : public IFunction { } }; +// (MAP, ARRAY<BOOL>) -> MAP +class FunctionMapFilter : public IFunction { +public: + static constexpr auto name = "map_filter"; + static FunctionPtr create() { return std::make_shared<FunctionMapFilter>(); } + + String get_name() const override { return name; } + size_t get_number_of_arguments() const override { return 2; } + bool use_default_implementation_for_nulls() const override { return false; } + + DataTypePtr get_return_type_impl(const DataTypes& arguments) const override { + auto map_type = remove_nullable(arguments[0]); + return have_nullable(arguments) ? make_nullable(std::move(map_type)) : map_type; + } + + Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, + uint32_t result, size_t input_rows_count) const override { + const auto& [unpacked_map_column, map_is_const] = + unpack_if_const(block.get_by_position(arguments[0]).column); + const auto& [unpacked_predicate_column, predicate_is_const] = + unpack_if_const(block.get_by_position(arguments[1]).column); + + auto result_null_map = ColumnUInt8::create(input_rows_count, 0); + auto& result_null_map_data = result_null_map->get_data(); + auto merge_null_map = [&](const ColumnPtr& column, bool is_const) -> const IColumn& { + if (const auto* nullable = check_and_get_column<ColumnNullable>(column.get())) { + VectorizedUtils::update_null_map(result_null_map_data, + nullable->get_null_map_data(), is_const); + return nullable->get_nested_column(); + } + return *column; + }; + + const auto& map = + assert_cast<const ColumnMap&>(merge_null_map(unpacked_map_column, map_is_const)); + const auto& predicate = assert_cast<const ColumnArray&>( + merge_null_map(unpacked_predicate_column, predicate_is_const)); + RETURN_IF_ERROR(check_arguments(map, map_is_const, predicate, predicate_is_const, + result_null_map_data)); + + IColumn::Selector selector; + auto result_offsets = ColumnArray::ColumnOffsets::create(); + build_selector_and_offsets(map, map_is_const, predicate, predicate_is_const, + result_null_map_data, selector, *result_offsets); + + auto result_keys = map.get_keys().clone_empty(); + auto result_values = map.get_values().clone_empty(); + if (!map_is_const) { + map.get_keys().append_data_by_selector(result_keys, selector); + map.get_values().append_data_by_selector(result_values, selector); + } else if (!selector.empty()) { + result_keys->insert_indices_from(map.get_keys(), selector.data(), + selector.data() + selector.size()); + result_values->insert_indices_from(map.get_values(), selector.data(), + selector.data() + selector.size()); + } + auto result_map = ColumnMap::create(std::move(result_keys), std::move(result_values), + std::move(result_offsets)); + if (block.get_by_position(result).type->is_nullable()) { + block.replace_by_position(result, ColumnNullable::create(std::move(result_map), + std::move(result_null_map))); + } else { + block.replace_by_position(result, std::move(result_map)); + } + return Status::OK(); + } + +private: + static void build_selector_and_offsets(const ColumnMap& map, bool map_is_const, + const ColumnArray& predicate, bool predicate_is_const, + const NullMap& result_null_map, + IColumn::Selector& selector, + ColumnArray::ColumnOffsets& result_offsets) { + const auto& nullable_predicate = assert_cast<const ColumnNullable&>(predicate.get_data()); + const auto& predicate_null_map = nullable_predicate.get_null_map_data(); + const auto& predicate_values = + assert_cast<const ColumnUInt8&>(nullable_predicate.get_nested_column()).get_data(); + + if (!map_is_const) { + selector.reserve(map.get_keys().size()); + } + result_offsets.reserve(result_null_map.size()); + + for (size_t row = 0; row < result_null_map.size(); ++row) { + if (!result_null_map[row]) { + const size_t map_row = index_check_const(row, map_is_const); + const size_t map_begin = map.get_offsets()[map_row - 1]; + const size_t predicate_begin = + predicate.get_offsets()[index_check_const(row, predicate_is_const) - 1]; + const size_t entry_count = map.size_at(map_row); + for (size_t entry = 0; entry < entry_count; ++entry) { + const size_t predicate_entry = predicate_begin + entry; + const bool selected = predicate_null_map[predicate_entry] == 0 && + predicate_values[predicate_entry] != 0; + if (selected) { + selector.push_back(map_begin + entry); Review Comment: [P2] Avoid narrowing 64-bit Map offsets into Selector `ColumnMap`/Array positions are `Offset64`, but `IColumn::Selector` stores `UInt32`. This `push_back` implicitly truncates `map_begin + entry` (and the filtered-entry constructor explicitly truncates `entry`), so position 2^32 wraps to 0 while result offsets keep advancing, silently copying the wrong key/value. Preserve 64-bit positions with range copying or fail on a checked invariant before `UINT32_MAX`; add a focused boundary test. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/TransformValues.java: ########## @@ -0,0 +1,103 @@ +// 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.scalar; + +import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.PreferPushDownProject; +import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; +import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable; +import org.apache.doris.nereids.trees.expressions.functions.RewriteWhenAnalyze; +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.ArrayType; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.MapType; +import org.apache.doris.nereids.types.StructField; +import org.apache.doris.nereids.types.StructType; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** + * Scalar function transform_values. + * + * <p>The original keys are retained while the Map lambda produces the new value array: + * + * <pre> + * transform_values((mapKey, mapValue) -> newValue, inputMap) + * -> + * %map_from_entries_unique%( + * array_map( + * entry -> struct(entry[1], newValue(entry[1], entry[2])), + * map_entries(inputMap))) + * </pre> + */ +public class TransformValues extends ScalarFunction + implements CustomSignature, PropagateNullable, PreferPushDownProject, RewriteWhenAnalyze { Review Comment: [P1] Preserve independent Map field precision in these wrappers This class, `TransformKeys`, `MapApply`, and both `MapFilter` overloads omit the no-op `ComputePrecision` contract already used by `MapEntries`, `MapFromEntries`, and `MapFromArrays`. Default promotion derives one common DecimalV3/time scale from independent Map key/value fields and casts them before rewrite or execution. For identity `transform_values` on `MAP<DECIMALV3(10,2),DECIMALV3(20,10)>`, the retained key becomes `DECIMALV3(20,10)`; direct and Lambda `map_filter` likewise change the input Map type. Implement `ComputePrecision` with `return signature` in all four wrappers and test mixed DecimalV3/time scales. -- 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]
