Gabriel39 commented on code in PR #68301: URL: https://github.com/apache/doris/pull/68301#discussion_r4060631923
########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ArrayFunctionUtils.java: ########## @@ -0,0 +1,42 @@ +// 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.Expression; +import org.apache.doris.nereids.types.ArrayType; +import org.apache.doris.nereids.types.DataType; + +/** Argument validation shared by array functions. */ +final class ArrayFunctionUtils { + private ArrayFunctionUtils() { + } + + static void checkNoVarBinaryArguments(ScalarFunction function) { + // Inspect original arguments before coercion can hide unsupported binary comparison/hash inputs. + for (Expression argument : function.getArguments()) { + DataType type = argument.getDataType(); + while (type instanceof ArrayType) { Review Comment: The referenced map-membership dispatchers already lacked VARBINARY support before this PR. Expanding function-local FE validation is outside this follow-up. ########## be/src/core/data_type_serde/data_type_varbinary_serde.cpp: ########## @@ -301,6 +305,82 @@ Status DataTypeVarbinarySerDe::deserialize_one_cell_from_json(IColumn& column, S return Status::OK(); } +Status DataTypeVarbinarySerDe::from_string(StringRef& str, IColumn& column, + const FormatOptions& options) const { + // Partition structs use the same hex representation as nested VARBINARY output. Decode it + // before appending so arbitrary bytes survive JSON transport instead of becoming NULL. Review Comment: The top-level and nested encodings differ, but the pre-PR VARBINARY SerDe inherited from_string() returning NotSupported. Thus this is not a regression of a previously supported Hive partition decoder. Adding that decoder is outside the current primitive changes. ########## be/src/core/value/timestamptz_value.cpp: ########## @@ -65,6 +73,14 @@ std::string TimestampTzValue::to_string(const cctz::time_zone& tz, int scale) co buffer[len++] = ':'; buffer[len++] = static_cast<char>('0' + offset_mins / 10); buffer[len++] = '0' + offset_mins % 10; + // Historical zones can have sub-minute offsets. Dropping their seconds changes the + // instant represented by the client-visible wall clock and offset when read back. + const int offset_seconds = abs_offset % 60; Review Comment: The separate to_iso8601 implementation and its historical-offset/year-boundary limitations predate this PR. It is unchanged here; repairing that formatter would broaden the follow-up beyond newly introduced defects. ########## regression-test/suites/datatype_p0/timestamptz/test_timestamptz_historical_offset.groovy: ########## @@ -0,0 +1,53 @@ +// 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. + +suite("test_timestamptz_historical_offset") { + def originalZone = sql("select @@time_zone")[0][0] + def originalStrict = sql("select @@enable_strict_cast")[0][0] + def cases = [ + ["Asia/Shanghai", "1890-01-01 00:00:00.123456+00:00", "1890-01-01 08:05:43.123456+08:05:43"], + ["America/New_York", "1880-01-01 00:00:00.123456+00:00", "1879-12-31 19:03:58.123456-04:56:02"], + ["Asia/Shanghai", "2024-01-01 00:00:00.123456+00:00", "2024-01-01 08:00:00.123456+08:00"], + ["America/New_York", "2024-01-01 00:00:00.123456+00:00", "2023-12-31 19:00:00.123456-05:00"], + ["Asia/Kathmandu", "2024-01-01 00:00:00.123456+00:00", "2024-01-01 05:45:00.123456+05:45"] + ] + try { + for (def testCase : cases) { + sql "set time_zone = '${testCase[0]}'" + for (def strict : [false, true]) { + sql "set enable_strict_cast = ${strict}" + // A nonconstant input exercises BE protocol formatting and parsing instead of Review Comment: On this master branch, TimestampTzLiteral.uncheckedCastTo has no string conversion branch and throws AnalysisException. FoldConstantRuleOnFE catches it and retains the CAST, so it does not fold through getStringValue() as described. The distinct branch-4.1 local-year folding issue is being fixed separately in #68297. ########## be/src/exprs/function/cast/cast_to_string.h: ########## @@ -581,7 +583,22 @@ class CastToStringFunction { limited_col = col_from.cut(0, input_rows_count); col_to_serialize = limited_col.get(); } - type.get_serde()->to_string_batch(*col_to_serialize, *col_to, options); + const auto serde = type.get_serde(); + if (null_map != nullptr && std::any_of(null_map, null_map + input_rows_count, Review Comment: The reported batch-formatting concern is a performance optimization request. The null-payload fix remains correct, and this follow-up is restricted to newly introduced correctness/stability defects; no broader formatting refactor is included. ########## be/src/util/raw_value.h: ########## @@ -44,6 +45,11 @@ class RawValue { // Because crc32 hardware is not equal with zlib crc32 inline uint32_t RawValue::zlib_crc32(const void* v, size_t len, const PrimitiveType& type, uint32_t seed) { + // Reject binary even for NULL instead of reaching the default-type assertion or hash path. + if (type == TYPE_VARBINARY) { Review Comment: crc32_internal VARBINARY was already unsupported before the explicit BE rejection. Adding another FE function guard or a binary hash implementation is outside this follow-up. -- 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]
