github-actions[bot] commented on code in PR #66860: URL: https://github.com/apache/doris/pull/66860#discussion_r3828879893
########## be/src/exprs/function/function_timezone_hour_minute.cpp: ########## @@ -0,0 +1,142 @@ +// 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; + +// TIMESTAMPTZ values are stored as UTC instants without the input zone, so the +// offset extracted here is the offset of the session time zone at the instant. +// See TimestampTzValue for the storage design. +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; + // Unwrap nullable and const wrappers in any nesting order so that + // ColumnNullable(ColumnConst(...)) and ColumnConst(ColumnNullable(...)) + // inputs both reach the plain ColumnTimeStampTz data below. + col = remove_nullable(col); + if (is_column_const(*col)) { + col = assert_cast<const ColumnConst&>(*col).convert_to_full_column(); Review Comment: [P2] Preserve block-local constness without enabling cross-request caching Because the framework constant path is disabled, this expands a one-value `ColumnConst` to `input_rows_count`, allocates a full result column, and performs the identical cctz lookup for every scanned row. A projection such as `timezone_hour(CAST('2024-01-15 12:00:00' AS TIMESTAMPTZ))` over a large table therefore does O(N) timezone work for one per-execution value. Keep `use_default_implementation_for_constants()` false so `VectorizedFnCall::is_constant()` cannot cache across requests, but detect the const argument here, evaluate its nested value once, and return a block-local `ColumnConst`; the const-input test can assert that physical shape. ########## be/src/exprs/function/function_timezone_hour_minute.cpp: ########## @@ -0,0 +1,142 @@ +// 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; + +// TIMESTAMPTZ values are stored as UTC instants without the input zone, so the +// offset extracted here is the offset of the session time zone at the instant. +// See TimestampTzValue for the storage design. +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; + // Unwrap nullable and const wrappers in any nesting order so that + // ColumnNullable(ColumnConst(...)) and ColumnConst(ColumnNullable(...)) + // inputs both reach the plain ColumnTimeStampTz data below. + col = remove_nullable(col); + 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(); + for (size_t i = 0; i < input_rows_count; ++i) { + int64_t offset = tz_data[i].utc_offset(timezone); + result_data[i] = extract_hour ? offset / SECONDS_PER_HOUR + : (offset % SECONDS_PER_HOUR) / SECONDS_PER_MINUTE; + } + + block.get_by_position(result).column = std::move(result_column); + return Status::OK(); +} +} // namespace + +class FunctionTimezoneHour : public IFunction { +public: + static constexpr auto name = "timezone_hour"; + + static FunctionPtr create() { return std::make_shared<FunctionTimezoneHour>(); } + + 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 std::make_shared<DataTypeInt64>(); + } + + // The result depends on the session time_zone, so a constant result must + // never be cached: the point-query short-circuit executor opens output + // expressions with the default timezone and later reuses cached constant + // columns without re-evaluating them (VectorizedFnCall::is_constant + // consults this flag). Disable it like other nondeterministic functions + // (e.g. random, uuid). + bool use_default_implementation_for_constants() const override { return false; } Review Comment: [P1] Apply the request timezone before constant children open This override only makes the outer call nonconstant. `VectorizedFnCall::open()` still opens its children first, and a constant `VCastExpr` caches itself through `VExpr::get_const_col()`. In the point-query path, `Reusable::init()` opens the expression tree while its `RuntimeState` still has Doris's default `+08:00`; `PointQueryExecutor::init()` applies `request->time_zone` only afterward. For example, with session `America/New_York`, `timezone_hour(CAST(least('2024-03-10 03:30:00','2024-03-11 03:30:00') AS TIMESTAMPTZ))` survives FE folding because `least` has no FE evaluator. The child cast is cached as `2024-03-09 19:30 UTC` under `+08:00`, so the outer call returns `-5`; parsing the selected value in New York gives `2024-03-10 07:30 UTC` and should return `-4`. This is distinct from the existing outer-result cache thread: the stale value is a constant descendant and is wrong on the first point-query request. Please install the request timezone before opening or caching the poin t-query expression tree and add this nested-constant DST regression. -- 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]
