github-actions[bot] commented on code in PR #68301:
URL: https://github.com/apache/doris/pull/68301#discussion_r4059809850
##########
be/src/exprs/aggregate/aggregate_function_min_max_impl.h:
##########
@@ -141,6 +141,10 @@ AggregateFunctionPtr
create_aggregate_function_single_value(const String& name,
return creator_without_type::create_unary_arguments<
AggregateFunctionsSingleValue<Data<SingleValueDataComplexType>>>(
argument_types, result_is_nullable, attr);
+ case PrimitiveType::TYPE_VARBINARY:
Review Comment:
[P2] Add the matching FE rejection for every aggregate routed here. Nereids
still accepts `min(VARBINARY)`, `max(VARBINARY)`, and `any_value(VARBINARY)`
(`Min`/`Max` only reject metric types and `AnyValue` has an unrestricted
signature), but all three names are registered through this factory and now
throw while the BE builds the aggregate. Please reject them before coercion and
add analysis tests for the public names.
##########
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) {
Review Comment:
[P2] Cover the remaining unsupported collection entry points with this
pre-coercion check. `array_except_all`, `array_min`, and `array_max` still
accept `ARRAY<VARBINARY>` in Nereids: `array_except_all` then reaches
`dispatch_switch_all`, which has no VARBINARY case, while
`array_min`/`array_max` reach the newly added VARBINARY throw in the
single-value aggregate factory. Please invoke this guard from those three
functions and add them to the FE rejection tests so these queries fail during
analysis instead of BE preparation/execution.
##########
be/src/exprs/function/in.h:
##########
@@ -105,6 +105,10 @@ class FunctionIn : public IFunction {
if (scope == FunctionContext::THREAD_LOCAL) {
return Status::OK();
}
+ // Binary IO must not route IN through the shared string/storage
predicate implementation.
+ if (context->get_arg_type(0)->get_primitive_type() == TYPE_VARBINARY) {
Review Comment:
[P2] Reject this type in Nereids as well.
`InPredicate.checkLegalityBeforeTypeCoercion` excludes object/complex types,
and `supportCompare` accepts VARBINARY as an ordinary primitive, so same-typed
`IN`/`NOT IN` expressions still plan successfully and fail only when this BE
function opens. Please add the pre-coercion check and same-/mixed-type analysis
coverage.
##########
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:
[P1] Preserve the top-level partition-string contract here. With
`enable.mapping.varbinary=true`, `HiveScanRange` copies each non-null HMS
partition value directly into `columns_from_path`, and
`FileScannerV2::_parse_partition_value` calls this `from_string`; any ordinary
value not starting with `0x` now fails with `Invalid VARBINARY hex
representation`. The `0x` grammar is symmetric only with nested serialization
(`to_string` emits it at nesting level >= 2), so please restrict hex decoding
to that context or use a dedicated nested decoder, and add a Hive partition
scan test.
##########
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:
[P1] Update the parallel `to_iso8601(TIMESTAMPTZ)` formatter too. Its
specialization still emits only `+HH:MM` (and derives the sign from truncated
`offset_hours`), so a historical offset such as `+08:05:43` is rendered as
`+08:05`, denoting a different instant; it also unchecked-casts the
session-local year instead of applying this new boundary guard. Please share
this formatter or mirror both fixes, increase its 32-byte maximum for `:SS`,
and add historical-offset and boundary coverage.
##########
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:
[P1] Add a folded counterpart and align FE cast-to-string evaluation with
this result. The current case deliberately prevents folding, but
`FoldConstantRuleOnFE` folds a TIMESTAMPTZ literal cast to string through
`TimestampTzLiteral.getStringValue()`, which is UTC-based; the equivalent
nonconstant cast now runs the BE formatter and returns the session-local wall
clock with the full historical offset. Thus enabling folding can change the
string result (and lets boundary values evade the new local-year error). Please
make the FE literal cast use the same session-zone contract and test folded
versus nonfolded historical and boundary cases.
##########
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) {
+ type = ((ArrayType) type).getItemType();
+ }
+ if (type.isVarBinaryType()) {
Review Comment:
[P2] Include the remaining scalar ordering entry points in the pre-coercion
rejection sweep. `least` and `greatest` preserve VARBINARY as their common
type, but with two or more arguments the BE creates a `ColumnVarbinary`, misses
the string branch, and reaches `dispatch_switch_scalar`, which has no VARBINARY
case. Please add a shared legality check for both names and two-argument
analysis tests; the deliberate unary passthrough can remain supported if
desired.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ArrayEnumerateUniq.java:
##########
@@ -65,6 +65,7 @@ private ArrayEnumerateUniq(ScalarFunctionParams
functionParams) {
*/
@Override
public void checkLegalityBeforeTypeCoercion() {
+ ArrayFunctionUtils.checkNoVarBinaryArguments(this);
Review Comment:
[P2] Restrict this rejection to the one-array form. The unary BE branch uses
`dispatch_switch_scalar` and cannot handle VARBINARY, but with two or more
arrays `FunctionArrayEnumerateUniq` directly selects `MethodSerialized`; that
path serializes every nested column, and this PR supplies the required
serialization methods for `ColumnVarbinary`. The blanket guard therefore turns
a supported multi-array form into an analysis error. Please preserve that arity
and replace the mixed-array rejection assertion with execution coverage for
long, empty, embedded-NUL, duplicate, and nullable binary tuple components.
##########
be/src/exprs/aggregate/aggregate_function_min_max_impl.h:
##########
@@ -141,6 +141,10 @@ AggregateFunctionPtr
create_aggregate_function_single_value(const String& name,
return creator_without_type::create_unary_arguments<
AggregateFunctionsSingleValue<Data<SingleValueDataComplexType>>>(
argument_types, result_is_nullable, attr);
+ case PrimitiveType::TYPE_VARBINARY:
+ // Owning binary values for IO must not implicitly enable single-value
aggregates.
Review Comment:
[P2] Extend the FE-before-BE rejection inventory beyond this factory. The
same mismatch remains for `min_by`/`max_by` with a VARBINARY ordering key,
`group_array_intersect`/`group_array_union` with `ARRAY<VARBINARY>`, and
VARBINARY inputs to `histogram`/`hist`, `linear_histogram`, `topn_array`,
`ndv`/`approx_count_distinct`, `map_agg_v1`, `map_agg_v2`, and alias `map_agg`:
their Nereids signatures admit the type, while the corresponding BE creator
dispatches omit it and return no function. Please add function-local
pre-coercion checks and analysis coverage for these public names, or add BE
support where byte ordering/copying is intended.
##########
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:
[P2] Apply the same restriction to the map membership entry points.
`map_contains_key`, `map_contains_value`, and `map_contains_entry` have broad
Nereids signatures and no legality check, but the first two forward to the
array-index dispatcher and the third uses its own `dispatch_switch_all`;
neither dispatches VARBINARY. Add equivalent recursive checks for VARBINARY
keys/values and FE analysis coverage.
##########
be/src/core/data_type/data_type_factory.cpp:
##########
@@ -635,6 +635,10 @@ DataTypePtr DataTypeFactory::create_data_type(
} else if (primitive_type == TYPE_AGG_STATE) {
// Do nothing
nested = std::make_shared<DataTypeAggState>();
+ } else if (primitive_type == TYPE_VARBINARY) {
Review Comment:
[P2] Complete execution support for this newly reconstructible type in the
default conditional evaluators. With the default
`short_circuit_evaluation=false`, a multi-row `coalesce` whose selected values
come from different arguments reaches `filled_result_column`, where
`dispatch_switch_scalar` omits VARBINARY; a two-or-more-`WHEN` CASE separately
reaches `VCaseExpr::_execute_update_result`, whose type switch also omits it.
Both expressions are FE-legal, and both work when short-circuit evaluation is
enabled through generic `insert_from`, so results currently depend on this
session setting. Please add the generic/VARBINARY paths and regression cases
for mixed-row COALESCE and multi-branch CASE under both settings.
##########
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:
[P2] Keep this null-safety fix from disabling batch formatting for every
nullable source type. A single NULL in a `Nullable<BIGINT>`/decimal/date block
now sends every non-null row through a virtual `serde->to_string` call and
reserves only one character per row, bypassing the typed `to_string_batch`
loops and their capacity estimates for the entire block. Please make the
optimized batch API null-aware or limit the row-wise skip to formatter types
whose masked payload can be invalid (notably TIMESTAMPTZ).
##########
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:
[P2] Fence the SQL caller as well. `crc32_internal` is a registered builtin
whose Nereids class accepts variadic `AnyDataType`, and its BE implementation
calls `RawValue::zlib_crc32` for every non-null argument, so
`crc32_internal(VARBINARY)` now plans successfully and throws here at
execution. Please reject VARBINARY in `Crc32Internal` before coercion (with an
FE analysis test), or implement the byte hash if the debug scalar should
support it.
--
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]
