github-actions[bot] commented on code in PR #66860:
URL: https://github.com/apache/doris/pull/66860#discussion_r3802156908
##########
pytest/qe/palo2/src/test_query_datetime_function.py:
##########
@@ -1007,6 +1007,37 @@ def test_query_time_convert_tz():
runner.check2(line1, line2)
+def test_query_timezone_hour_minute():
+ """
+ {
+ "title": "test_query_datetime_function.test_query_timezone_hour_minute",
+ "describe": "test for timezone_hour and timezone_minute",
+ "tag": "function,p0"
+ }
+ """
+ # UTC+08:00 has no DST, the offset of the session timezone is the same
+ # for every instant, so timezone_hour always returns 8 here.
+ runner.init("set time_zone = '+08:00'")
Review Comment:
[P2] Apply `SET time_zone` on the SELECT's connection
`runner.init()` sends the Doris `SET` through `PaloQE.do_sql()`, which opens
and closes a connection for that call. `get_sql_result()` then opens a fresh
session, so neither SELECT sees the timezone set above it; with the default
`+08:00`, the New York block returns `8`/`0`, not `-5`/`-4`. Use the existing
`do_set_properties_sql(select_sql, ["set time_zone = ..."])` pattern (and avoid
modifying the unrelated MySQL session), then add the claimed
fractional/nullable and input-zone-vs-session-zone cases in the standard
regression suite.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/TimezoneHour.java:
##########
@@ -0,0 +1,73 @@
+// 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.ExplicitlyCastableSignature;
+import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable;
+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.TimeStampTzType;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+
+/**
+ * ScalarFunction 'timezone_hour'.
+ */
+public class TimezoneHour extends ScalarFunction
Review Comment:
[P1] Model the session-timezone dependency
Both new functions read session `time_zone` but inherit `isDeterministic()
== true`. With BE folding enabled, a constant result can be serialized into a
reusable prepared point-query plan and remain stale after `SET time_zone`. The
same classification also admits these expressions into synchronous and async
materialized views, whose persisted result/rewrite identity does not retain
this execution-only variable. Please represent the session dependency so
folding, prepared-plan reuse, and MV admission/rewrite all account for it for
both classes, and add prepared-query and MV tests across timezone changes.
##########
be/src/exprs/function/function_timezone_hour_minute.cpp:
##########
@@ -0,0 +1,124 @@
+// 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 <cctz/time_zone.h>
+
+#include <cstdint>
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "common/status.h"
+#include "core/assert_cast.h"
+#include "core/block/block.h"
+#include "core/block/column_numbers.h"
+#include "core/column/column.h"
+#include "core/column/column_const.h"
+#include "core/column/column_nullable.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_number.h"
+#include "core/data_type/data_type_timestamptz.h"
+#include "core/data_type/primitive_type.h"
+#include "core/value/timestamptz_value.h"
+#include "exprs/function_context.h"
+#include "exprs/function/function.h"
+#include "exprs/function/simple_function_factory.h"
+#include "runtime/runtime_state.h"
+
+namespace doris {
+
+namespace {
+constexpr int64_t SECONDS_PER_HOUR = 3600;
+constexpr int64_t SECONDS_PER_MINUTE = 60;
+
+Status execute_timezone_offset_part(FunctionContext* context, Block& block,
+ const ColumnNumbers& arguments, uint32_t
result,
+ size_t input_rows_count, bool
extract_hour) {
+ ColumnPtr col = block.get_by_position(arguments[0]).column;
+ if (is_column_const(*col)) {
+ col = assert_cast<const ColumnConst&>(*col).convert_to_full_column();
+ }
+ col = remove_nullable(col);
+ const auto* tz_column = assert_cast<const ColumnTimeStampTz*>(col.get());
+ const auto& tz_data = tz_column->get_data();
+
+ auto result_column = ColumnInt64::create();
+ auto& result_data = result_column->get_data();
+ result_data.resize(input_rows_count);
+
+ const cctz::time_zone& timezone = context->state()->timezone_obj();
Review Comment:
[P1] Extract the input value's zone, not the session zone
Trino's `timestamp with time zone` retains a zone key, and
`timezone_hour`/`timezone_minute` extract that value's offset. Doris converts
an explicit input zone to UTC and discards it, then this line substitutes the
session zone. For example, with session `+08:00`, `CAST('2024-01-15
12:00:00-04:30' AS TIMESTAMPTZ)` returns `8`/`0` here instead of Trino's
`-4`/`-30`. That silently breaks the advertised migration compatibility. Please
resolve the contract by retaining/extracting the input zone (including
serialization compatibility), or explicitly scope/rename the feature as
session-offset extraction, and add an end-to-end case where the input and
session zones differ.
--
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]
