github-actions[bot] commented on code in PR #68441:
URL: https://github.com/apache/doris/pull/68441#discussion_r4082486609
##########
gensrc/thrift/ExternalTableSchema.thrift:
##########
@@ -64,7 +64,10 @@ struct TField {
8: optional bool initial_default_value_is_base64,
// Version marker for authoritative Iceberg mapping semantics. Its absence
preserves the
// legacy name fallback when a new BE executes a plan produced by an older
FE during rollout.
- 9: optional bool name_mapping_is_authoritative
+ 9: optional bool name_mapping_is_authoritative,
+ // Table-format timestamp semantic used when the physical file encoding is
ambiguous. Paimon
+ // uses this to distinguish TIMESTAMP (false) from TIMESTAMP_LTZ (true),
including INT96.
+ 10: optional bool timestamp_is_adjusted_to_utc
}
Review Comment:
[P1] Populate Paimon's timestamp semantic on this wire field. The production
PaimonUtil.getSchemaInfo() recursion never sets it, so every native Paimon
history schema leaves __isset false. Since this patch now infers unannotated
INT96 as DATETIMEV2 and PaimonReader only gets the override from this field,
TIMESTAMP_LTZ is decoded as wall clock; in a non-UTC session the final cast
changes the instant (or exposes UTC as local DATETIME when mapping is
disabled). Please set this recursively for Paimon TIMESTAMP/TIMESTAMP_LTZ and
test the real FE-to-BE path, including a nested/filter-only LTZ field.
##########
be/src/exec/operator/file_scan_operator.cpp:
##########
@@ -147,9 +148,21 @@ 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;
+ // Version 1 introduces the explicit wall-clock/instant contract that
scanner V1 cannot honor.
+ const bool requires_parquet_timestamp_contract =
+ (scan_params.format_type == TFileFormatType::FORMAT_PARQUET ||
+ supports_iceberg_scan_semantics_v1(&scan_params)) &&
Review Comment:
[P1] Include hybrid native-Parquet plans in this V2 requirement. Paimon uses
common scan_params FORMAT_JNI and declares Parquet only on each native range;
Hudi MOR can do the same for log-free ranges. Because this decision runs before
range dispatch, enable_file_scanner_v2=false still selects V1 for a version-1
plan, even though V1 consumes neither hive_parquet_time_zone nor the new
per-column semantic. Please force these hybrid/native-Parquet cases onto V2 and
add a common-FORMAT_JNI selection test.
##########
be/src/format/parquet/parquet_arrow_block_convertor.cpp:
##########
@@ -31,10 +31,11 @@ Status ParquetArrowBlockConvertor::init() {
std::vector<std::shared_ptr<arrow::Field>> fields;
fields.reserve(_types.size());
// Retain the declared Arrow timezone label; cctz's fixed-offset name is
internal.
- // Preserve the existing timezone-bearing DATETIMEV2 schema for both
Parquet encodings.
+ // INT96 normalization and schema construction must use the same
instance's timezone.
for (size_t i = 0; i < _types.size(); ++i) {
std::shared_ptr<arrow::DataType> type;
- RETURN_IF_ERROR(convert_to_arrow_type(_types[i], &type,
_timezone_name));
+ RETURN_IF_ERROR(
+ convert_to_arrow_type(_types[i], &type, _timezone_name,
!_enable_int96_timestamps));
fields.emplace_back(arrow::field(_names[i], type,
_types[i]->is_nullable()));
Review Comment:
[P1] Version this INT64 DATETIME writer switch for rolling upgrades.
enable_int96_timestamps=false already exists in old FE OUTFILE plans, but old
BEs encode DATETIMEV2 as a session-normalized, isAdjustedToUTC=true INT64 while
this call makes new BEs write the wall-clock value with isAdjustedToUTC=false.
In a non-UTC session during a BE-first rolling upgrade, one old-FE parallel
OUTFILE can therefore produce different timestamp values/schema depending on
the selected BE. Please add an FE-to-BE writer-semantics marker and retain
legacy conversion until the plan opts in, with a mixed-version contract test.
##########
be/src/format_v2/parquet/parquet_reader.cpp:
##########
@@ -443,8 +445,9 @@ int timestamp_tz_scale(const ParquetTypeDescriptor&
type_descriptor) {
bool should_map_to_timestamp_tz(const ParquetColumnSchema& column_schema) {
const auto& type_descriptor = column_schema.type_descriptor;
- return type_descriptor.physical_type == tparquet::Type::INT96 ||
- (type_descriptor.is_timestamp &&
type_descriptor.timestamp_is_adjusted_to_utc);
+ // INT96 has no instant annotation. Reinterpreting it here disagrees with
FE schema inference
+ // and makes the table-level cast apply the session timezone a second time.
+ return type_descriptor.is_timestamp &&
type_descriptor.timestamp_is_adjusted_to_utc;
}
Review Comment:
[P1] Preserve the explicit INT96 mapping for schema-less scans. This
condition, together with the paired V1 schema change, now ignores
enable_mapping_timestamp_tz for unannotated INT96, but raw hdfs()/s3() TVFs
have no table-format TField semantic to replace that opt-in override. The
existing Paimon raw-file regression explicitly sets the option and expects
ts_ltz as TIMESTAMPTZ with +08:00 values; this code instead infers DATETIMEV2
and exposes the UTC carrier as wall clock. Please keep the override as a
fallback when no per-column semantic is available (with explicit metadata
taking precedence) and cover the production TVF path.
##########
be/src/format_v2/parquet/parquet_reader.cpp:
##########
@@ -510,6 +513,92 @@ void
apply_timestamp_tz_mapping_in_variants(ParquetColumnSchema* column_schema)
}
}
+const format::LocalColumnIndex* find_semantic_child(const
format::LocalColumnIndex& projection,
+ int32_t local_id) {
+ const auto it = std::ranges::find_if(projection.children,
+ [local_id](const
format::LocalColumnIndex& child) {
+ return child.local_id() ==
local_id;
+ });
+ return it == projection.children.end() ? nullptr : &*it;
+}
+
+DataTypePtr apply_projection_timestamp_semantics(ParquetColumnSchema*
column_schema,
+ const
format::LocalColumnIndex& projection) {
+ DORIS_CHECK(column_schema != nullptr);
+ column_schema->timestamp_is_adjusted_to_utc =
projection.timestamp_is_adjusted_to_utc;
+ if (column_schema->kind == ParquetColumnSchemaKind::PRIMITIVE) {
+ const auto& descriptor = column_schema->type_descriptor;
+ const bool physical_timestamp =
+ descriptor.physical_type == tparquet::Type::INT96 ||
descriptor.is_timestamp;
+ if (physical_timestamp &&
projection.timestamp_is_adjusted_to_utc.has_value()) {
+ const auto target =
+ *projection.timestamp_is_adjusted_to_utc ?
TYPE_TIMESTAMPTZ : TYPE_DATETIMEV2;
+ column_schema->type = DataTypeFactory::instance().create_data_type(
Review Comment:
[P1] Apply this semantic type before TableReader fixes the file-block
contract. TableReader has already built its mapper, block template, and
DATETIMEV2-to-TIMESTAMPTZ cast from the annotated ColumnDefinition (whose type
remains DATETIMEV2) before this late mutation changes ParquetReader's private
schema to TIMESTAMPTZ. Native sync then selects DataTypeTimeStampTzSerDe, which
assert-casts the supplied column to ColumnTimeStampTz even though the block
contains ColumnDateTimeV2; nested leaves have the same mismatch, and the stale
mapper would cast an already-instant value again. Move the effective type into
the shared annotated schema before mapping/block construction and cover the
full Paimon TableReader path.
--
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]
