github-actions[bot] commented on code in PR #68381:
URL: https://github.com/apache/doris/pull/68381#discussion_r4073892912
##########
be/src/format/arrow/arrow_block_convertor.cpp:
##########
@@ -118,6 +120,98 @@ std::shared_ptr<arrow::DataType> extension_storage_type(
}
}
+// `type` is the Doris logical type; `plain_arrow_type` is its ordinary SerDe
mapping;
+// `target_type` is the Arrow representation requested by the consumer. For
example,
+// DATETIMEV2(6) may bind timestamp(us) without a timezone, but TIMESTAMPTZ(6)
must
+// retain a timezone to preserve instant semantics. Nested bindings obey the
same rule.
+bool is_declared_plain_arrow_binding(const DataTypePtr& type,
+ const std::shared_ptr<arrow::DataType>&
plain_arrow_type,
+ const std::shared_ptr<arrow::DataType>&
target_type) {
+ if (plain_arrow_type->Equals(target_type)) {
+ return true;
+ }
+ if (plain_arrow_type->id() == arrow::Type::TIMESTAMP &&
+ target_type->id() == arrow::Type::TIMESTAMP) {
+ const auto& plain_timestamp = assert_cast<const
arrow::TimestampType&>(*plain_arrow_type);
+ const auto& target_timestamp = assert_cast<const
arrow::TimestampType&>(*target_type);
+ if (plain_timestamp.unit() != target_timestamp.unit()) {
+ return false;
+ }
+ const PrimitiveType primitive =
remove_nullable(type)->get_primitive_type();
+ // A timezone-free Arrow timestamp is a wall-clock value and is
therefore only compatible
+ // with DATETIMEV2; TIMESTAMPTZ must always retain its instant
semantics.
+ if (target_timestamp.timezone().empty()) {
+ return primitive == TYPE_DATETIMEV2;
+ }
Review Comment:
[P1] Compare timezone bindings semantically instead of by cctz name
A valid `+00:00` session reaches BE as Java's canonical `Z`. The timezone
cache resolves `Z` to a cctz object named `Zulu`, while Arrow schema
construction rewrites the declared `Z` label to `UTC`.
`write_plain_arrow_column` therefore builds the source timestamp with `Zulu`,
this line resolves the target as `UTC`, and the new preflight rejects `UTC ==
Zulu` before writing even though both are the same zero-offset zone. This
breaks timezone-aware timestamp batches on the common plain converter path
(including Parquet/Hive and memory-scratch/Arrow-Flight consumers) for the
supported `+00:00` session value. Please normalize both sides to one identity
or compare actual zone equivalence, and add a full batch-conversion test using
the cctz object resolved from `Z` rather than only checking the generated
schema label.
##########
be/src/core/data_type_serde/data_type_array_serde.cpp:
##########
@@ -321,6 +321,77 @@ Status DataTypeArraySerDe::write_column_to_arrow(const
IColumn& column, const Nu
return Status::OK();
}
+namespace {
+
+template <typename WriteNested>
+Status write_array_column_to_target(const IColumn& column, const NullMap*
null_map,
+ arrow::ArrayBuilder* array_builder,
int64_t start, int64_t end,
+ WriteNested&& write_nested) {
+ const auto& array_column = assert_cast<const ColumnArray&>(column);
+ const auto& offsets = array_column.get_offsets();
+ const auto& nested_data = array_column.get_data();
+ auto& builder = assert_cast<arrow::ListBuilder&>(*array_builder);
+ auto* nested_builder = builder.value_builder();
+ for (size_t array_idx = start; array_idx < end; ++array_idx) {
+ if (null_map != nullptr && (*null_map)[array_idx]) {
+ RETURN_IF_ERROR(checkArrowStatus(builder.AppendNull(), column,
*array_builder));
+ continue;
+ }
+ RETURN_IF_ERROR(checkArrowStatus(builder.Append(), column,
*array_builder));
+ RETURN_IF_ERROR(write_nested(nested_data, nested_builder,
offsets[array_idx - 1],
+ offsets[array_idx]));
+ }
+ return Status::OK();
+}
+
+} // namespace
+
+Status DataTypeArraySerDe::write_column_to_paimon_arrow(
+ const std::shared_ptr<const IDataType>& type, const IColumn& column,
+ const NullMap* null_map, const std::shared_ptr<arrow::Field>& field,
+ arrow::ArrayBuilder* array_builder, int64_t start, int64_t end,
+ const cctz::time_zone& ctz) const {
+ // Reject an incompatible target before casting its nested schema or
builder.
+ if (field->type()->id() != arrow::Type::LIST ||
Review Comment:
[P1] Validate the nested binding before dispatching to the child SerDe
This only checks that the outer target is a LIST. The Paimon/Iceberg
converters bypass the recursive plain-binding validator, so a same-outer-kind
mismatch reaches the child writer. For example, a Doris `array<decimal(10,3)>`
value `1.234` has raw coefficient `1234`; if the pinned target is
`list<decimal(10,2)>`, the Decimal128 writer accepts the same builder and
persists that coefficient as `12.34`. `array<int>` versus `list<string>`
instead reaches the wrong builder cast, and Map/Struct have the same gap.
Please validate the full recursive binding (including decimal parameters,
timestamp units, and struct field identity/order) before appending the parent
builder.
--
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]
