github-actions[bot] commented on code in PR #67784: URL: https://github.com/apache/doris/pull/67784#discussion_r4011457867
########## be/src/format_v2/parquet/parquet_timestamp_semantics.h: ########## @@ -0,0 +1,40 @@ +// 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. + +#pragma once + +#include <cstdint> +#include <optional> +#include <string> + +#include "gen_cpp/PlanNodes_types.h" + +namespace doris::format::parquet { + +inline constexpr int32_t PARQUET_TIMESTAMP_SEMANTICS_VERSION_1 = 1; + +inline std::optional<std::string> get_int96_timezone_override(const TFileScanRangeParams* params) { + // Only the marker can distinguish a new FE's explicit wall-clock semantics from an old FE + // plan whose missing timezone field must keep legacy session-timezone decoding. + if (params == nullptr || !params->__isset.parquet_timestamp_semantics_version || Review Comment: [P1] Honor an explicit field-36 timezone even when this new marker is absent. Current master FE already sends `hive_parquet_time_zone` for a configured catalog/TVF zone, but it has no field 39; this early return discards that value on a branch-4.1 BE and falls back to the session timezone, so mixed-version splits can decode the same INT96 value differently. The legacy case from the earlier thread has both fields absent. Please reserve `nullopt` for that case, honor a present field 36 without the marker, and add the intermediate-schema compatibility test. ########## be/src/exec/operator/file_scan_operator.cpp: ########## @@ -147,9 +148,22 @@ bool FileScanLocalState::should_use_file_scanner_v2(const TQueryOptions& query_o const bool is_transactional_hive = scan_params.__isset.table_format_params && scan_params.table_format_params.table_format_type == "transactional_hive"; - return query_options.__isset.enable_file_scanner_v2 && query_options.enable_file_scanner_v2 && - !is_load && scan_params.format_type != TFileFormatType::FORMAT_ES_HTTP && - !is_transactional_hive; + const bool is_paimon_native_scan = scan_params.format_type == TFileFormatType::FORMAT_JNI && Review Comment: [P1] Limit this forced-V2 proxy to native Parquet. `PaimonScanNode` populates `history_schema_info` for native ORC splits too, while the scan-level format remains `FORMAT_JNI`, so every version-1 native ORC plan now overrides `enable_file_scanner_v2=false`. That is observable here because this PR keeps V1 ORC truncation but changes V2 to half-up/carry rounding; ORC results can change solely because the Parquet-only marker forced a different scanner. Please transport/derive an explicit native-Parquet signal and add the corresponding ORC routing check. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertOverwriteTableCommand.java: ########## @@ -487,7 +488,10 @@ private void setStaticPartitionToContext(UnboundIcebergTableSink<?> sink, for (Map.Entry<String, Expression> entry : staticPartitions.entrySet()) { Expression expr = entry.getValue(); if (expr instanceof Literal) { - staticPartitionValues.put(entry.getKey(), ((Literal) expr).getStringValue()); + // Binary literals expose bare hex, while BE partition parsing and FE commit + // conversion require an explicit byte representation, distinct from text. + String value = ((Literal) expr).getStringValue(); + staticPartitionValues.put(entry.getKey(), expr instanceof VarBinaryLiteral ? "0x" + value : value); Review Comment: [P1] Preserve static partition nullness separately from binary text. For `PARTITION(key=NULL)`, `getStringValue()` produces the ordinary string `"null"`, and this branch forwards it unchanged because it is not a `VarBinaryLiteral`. UUID then rejects it during BE open; nullable BINARY/FIXED turn the invalid hex into a null/default value and an empty path component, but the overwrite filter later rejects the original `"null"` at commit. Thus nullable binary identity partitions cannot be statically overwritten (and NULL is not distinguished from empty bytes). Please transport nullness explicitly and cover full-static/hybrid BINARY, FIXED, and UUID cases. ########## gensrc/thrift/DataSinks.thrift: ########## @@ -384,6 +384,8 @@ struct THiveTableSink { 10: optional bool overwrite 11: optional THiveSerDeProperties serde_properties 12: optional list<Types.TNetworkAddress> broker_addresses; + // Absent: legacy session timezone; empty: wall-clock INT96; otherwise: named catalog timezone. + 13: optional string hive_parquet_time_zone Review Comment: [P1] Avoid reusing `THiveTableSink` field 13 across maintained branches. Current master already defines id 13 as `optional bool supports_deferred_azure_multipart`, while this branch adds a string at the same id. During a branch-4.1/master mixed-version rollout, Thrift skips the mismatched wire type: the new Hive timezone cannot cross schemas and the writer falls back to legacy session semantics (shifting INT96 values). Please assign the timezone a non-conflicting id and add a mixed-schema serialization check. -- 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]
