github-actions[bot] commented on code in PR #68524:
URL: https://github.com/apache/doris/pull/68524#discussion_r4121151736
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:
##########
@@ -2058,4 +2059,75 @@ public static Expression
microsecondFromUnixtime(DecimalV3Literal unixTime) {
}
return new IntegerLiteral((int) fraction);
}
+
+ /**
+ * Constant folding for human_readable_seconds(BigIntLiteral)
+ */
+ @ExecFunction(name = "human_readable_seconds")
+ public static Expression humanReadableSeconds(BigIntLiteral seconds) {
+ return formatHumanReadableSeconds(seconds.getValue());
+ }
+
+ /**
+ * Constant folding for human_readable_seconds(IntegerLiteral)
+ */
+ @ExecFunction(name = "human_readable_seconds")
+ public static Expression humanReadableSeconds(IntegerLiteral seconds) {
+ return formatHumanReadableSeconds(seconds.getValue());
+ }
+
+ /**
+ * Constant folding for human_readable_seconds(SmallIntLiteral)
+ */
+ @ExecFunction(name = "human_readable_seconds")
+ public static Expression humanReadableSeconds(SmallIntLiteral seconds) {
+ return formatHumanReadableSeconds(seconds.getValue());
+ }
+
+ /**
+ * Constant folding for human_readable_seconds(TinyIntLiteral)
+ */
+ @ExecFunction(name = "human_readable_seconds")
+ public static Expression humanReadableSeconds(TinyIntLiteral seconds) {
+ return formatHumanReadableSeconds(seconds.getValue());
+ }
+
+ private static Expression formatHumanReadableSeconds(long seconds) {
+ if (seconds < 0) {
+ return new NullLiteral(VarcharType.SYSTEM_DEFAULT);
+ }
+ if (seconds == 0) {
+ return new VarcharLiteral("0s");
+ }
+ long days = seconds / 86400L;
+ long rem = seconds % 86400L;
+ long hours = rem / 3600L;
+ rem %= 3600L;
+ long minutes = rem / 60L;
+ long secs = rem % 60L;
+
+ StringBuilder sb = new StringBuilder();
+ if (days > 0) {
+ sb.append(days).append('d');
Review Comment:
[P1] Match the stated Trino function semantics in both execution paths. The
PR calls this Trino-compatible and issue #48203 lists it for Trino/Presto
migration, but [Trino documents
`human_readable_seconds(double)`](https://trino.io/docs/current/functions/datetime.html#duration-function):
96 returns `1 minute, 36 seconds` and larger values include weeks. Here FE
folding and BE execution return compact `1m 36s` and only decompose through
days; the FE signatures coerce inputs to INT/BIGINT. Existing Trino SQL will
therefore produce different values. Please reconcile the signature, both
formatters, and expected tests with that migration contract.
##########
be/src/exprs/function/function_human_readable_seconds.cpp:
##########
@@ -0,0 +1,198 @@
+// 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 <charconv>
+#include <cstdint>
+#include <cstring>
+#include <memory>
+#include <utility>
+
+#include "common/cast_set.h"
+#include "common/status.h"
+#include "core/assert_cast.h"
+#include "core/block/block.h"
+#include "core/block/column_numbers.h"
+#include "core/block/column_with_type_and_name.h"
+#include "core/column/column.h"
+#include "core/column/column_nullable.h"
+#include "core/column/column_string.h"
+#include "core/column/column_vector.h"
+#include "core/data_type/data_type.h"
+#include "core/data_type/data_type_nullable.h"
+#include "core/data_type/data_type_string.h"
+#include "core/types.h"
+#include "exprs/function/function.h"
+#include "exprs/function/simple_function_factory.h"
+
+namespace doris {
+
+class FunctionHumanReadableSeconds : public IFunction {
+public:
+ static constexpr auto name = "human_readable_seconds";
+ static FunctionPtr create() { return
std::make_shared<FunctionHumanReadableSeconds>(); }
+
+ String get_name() const override { return name; }
+
+ size_t get_number_of_arguments() const override { return 1; }
+
+ DataTypePtr get_return_type_impl(const DataTypes& /*arguments*/) const
override {
+ return make_nullable(std::make_shared<DataTypeString>());
+ }
+
+ // We must return false here to manually handle both input nulls and
computed nulls
+ // (negative values -> NULL). If default null implementation is used, it
strips nulls
+ // before execution, conflicting with our explicit output null map
creation.
+ bool use_default_implementation_for_nulls() const override { return false;
}
+
+ Status execute_impl(FunctionContext* /*context*/, Block& block, const
ColumnNumbers& arguments,
+ uint32_t result, size_t input_rows_count) const
override {
+ const auto& col_with_type = block.get_by_position(arguments[0]);
+ const auto* source_col = col_with_type.column.get();
+
+ const NullMap* null_map = nullptr;
+ const IColumn* actual_col = source_col;
+
+ // Explicitly unnest ColumnNullable to handle NULL inputs correctly
+ if (source_col->is_nullable()) {
+ const auto* nullable_col = assert_cast<const
ColumnNullable*>(source_col);
+ actual_col = nullable_col->get_nested_column_ptr().get();
+ null_map = &nullable_col->get_null_map_data();
+ }
+
+ auto res_column = ColumnString::create();
+ auto null_column = ColumnUInt8::create(input_rows_count);
+
+ bool success = false;
+ if (execute_typed<ColumnInt64>(actual_col, null_map, *res_column,
*null_column,
+ input_rows_count)) {
+ success = true;
+ } else if (execute_typed<ColumnInt32>(actual_col, null_map,
*res_column, *null_column,
+ input_rows_count)) {
+ success = true;
+ }
+
+ if (!success) [[unlikely]] {
+ return Status::InvalidArgument("Unsupported column type {} for
function {}",
+ col_with_type.type->get_name(),
name);
+ }
+
+ block.replace_by_position(
+ result, ColumnNullable::create(std::move(res_column),
std::move(null_column)));
+ return Status::OK();
+ }
+
+private:
+ /**
+ * Formats seconds into human-readable format:
+ * - Omit zero values (e.g., "1d 1s" instead of "1d 0h 0m 1s")
+ * - Special case 0 -> "0s"
+ * - Stack buffer ensures zero dynamic heap allocation
+ */
+ static inline size_t format_seconds(int64_t seconds, char* buf) {
+ if (seconds == 0) {
+ buf[0] = '0';
+ buf[1] = 's';
+ return 2;
+ }
+
+ char* ptr = buf;
+ auto append_unit = [&ptr](int64_t val, char unit, bool need_space) {
+ if (need_space) {
+ *ptr++ = ' ';
+ }
+ auto [next, _] = std::to_chars(ptr, ptr + 24, val);
Review Comment:
[P2] Bound `to_chars` by the actual stack buffer. For the covered
`BIGINT_MAX` input, days/hours/minutes use 24 bytes and the seconds separator
advances `ptr` to `buf + 25`; `ptr + 24` then forms `buf + 49`, past the valid
one-past end of `char buf[48]`. Even though this value's digits fit, passing an
invalid `[first, last)` range to `std::to_chars` is undefined behavior. Capture
and pass `buf + sizeof(buf)` as the end.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java:
##########
@@ -0,0 +1,76 @@
+// 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.AlwaysNullable;
+import
org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
+import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression;
+import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
+import org.apache.doris.nereids.types.BigIntType;
+import org.apache.doris.nereids.types.IntegerType;
+import org.apache.doris.nereids.types.VarcharType;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+
+/**
+ * ScalarFunction 'human_readable_seconds'.
+ */
+public class HumanReadableSeconds extends ScalarFunction
+ implements UnaryExpression, ExplicitlyCastableSignature,
AlwaysNullable {
Review Comment:
[P1] Make literal NULL propagate during FE folding. The added
`FoldConstantTest` expects `human_readable_seconds(null)` to rewrite to `null`,
but the default FE-only path folds a NULL argument only for
`PropagateNullLiteral` or `PropagateNullable`. `AlwaysNullable` does not
trigger that rule, and the evaluator has no `NullLiteral` overload, so the
function call remains and this assertion fails. Add `PropagateNullLiteral` here
while retaining `AlwaysNullable` for negative inputs.
--
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]
