github-actions[bot] commented on code in PR #67784:
URL: https://github.com/apache/doris/pull/67784#discussion_r3986073440
##########
be/src/core/data_type_serde/data_type_serde.cpp:
##########
@@ -496,8 +524,28 @@ Status decode_timestamp_orc_values(IColumn& nested_column,
const OrcDecodedColum
}
auto& value =
reinterpret_cast<DateV2Value<DateTimeV2ValueType>&>(data[old_data_size + row]);
+ RoundedOrcTimestamp timestamp;
+ auto status = round_orc_timestamp_to_microseconds(
+ orc_batch->data[source_row],
orc_batch->nanoseconds[source_row], ×tamp);
+ if (!status.ok()) {
+ data.resize(old_data_size);
+ return status;
+ }
value.from_unixtime(orc_batch->data[source_row], timezone);
-
value.set_microsecond(cast_set<uint64_t>(orc_batch->nanoseconds[source_row] /
1000));
+ if (!value.is_valid_date()) {
Review Comment:
[P2] Please validate the effective rounded value rather than rejecting the
pre-carry second. In UTC, an ORC value at second `-62167219201` with
`999999500ns` rounds exactly to `-62167219200`, i.e. Doris's supported
`0000-01-01 00:00:00` boundary. Here `from_unixtime` checks the
one-second-earlier year-minus-one value and returns before the carry can make
it valid. The new boundary test uses zero nanos and misses this case; add a
rounding-into-range lower-bound case.
##########
be/src/exec/operator/file_scan_operator.cpp:
##########
@@ -147,9 +148,18 @@ bool FileScanLocalState::_should_use_file_scanner_v2(const
TQueryOptions& query_
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 has_versioned_parquet_semantics =
+ scan_params.format_type == TFileFormatType::FORMAT_PARQUET &&
Review Comment:
[P1] Please include Paimon native-Parquet scans in this forced-V2 contract.
Paimon's scan-level `params.format_type` is always `FORMAT_JNI`; only its
native `TFileRangeDesc` entries are tagged `FORMAT_PARQUET`. Therefore
`enable_file_scanner_v2=false` still makes `_init_scanners` instantiate
FileScannerV1, which cannot consume the new history-schema per-column
TIMESTAMP/TIMESTAMP_LTZ semantics. The Paimon regression already documents that
the V1 Parquet timestamp(7/8/9) path returns values eight hours too large, so
the same versioned plan remains incorrect solely because of the scanner toggle.
Please route versioned Paimon native-Parquet ranges through V2 as well and
cover this setting in a regression.
##########
be/src/format/transformer/vparquet_transformer.h:
##########
@@ -88,18 +89,20 @@ struct ParquetFileOptions {
// a wrapper of parquet output stream
class VParquetTransformer final : public VFileFormatTransformer {
public:
- VParquetTransformer(RuntimeState* state, doris::io::FileWriter*
file_writer,
- const VExprContextSPtrs& output_vexpr_ctxs,
- std::vector<std::string> column_names, bool
output_object_data,
- const ParquetFileOptions& parquet_options,
- const std::string* iceberg_schema_json = nullptr,
- const iceberg::Schema* iceberg_schema = nullptr);
-
- VParquetTransformer(RuntimeState* state, doris::io::FileWriter*
file_writer,
- const VExprContextSPtrs& output_vexpr_ctxs,
- std::vector<TParquetSchema> parquet_schemas, bool
output_object_data,
- const ParquetFileOptions& parquet_options,
- const std::string* iceberg_schema_json = nullptr);
+ VParquetTransformer(
+ RuntimeState* state, doris::io::FileWriter* file_writer,
+ const VExprContextSPtrs& output_vexpr_ctxs,
std::vector<std::string> column_names,
+ bool output_object_data, const ParquetFileOptions& parquet_options,
+ const std::string* iceberg_schema_json = nullptr,
+ const iceberg::Schema* iceberg_schema = nullptr,
+ const ArrowWriteConverter& arrow_write_converter =
plain_arrow_write_converter());
Review Comment:
[P1] The plain default breaks both existing Iceberg Variant transformer
tests, whose constructors pass a non-null `iceberg_schema` but omit this new
argument. That schema creates Variant's `struct<metadata,value>` storage, while
`plain_arrow_write_converter()` only binds Doris VARIANT to
STRING/LARGE_STRING, so both `transformer.write()` assertions now
deterministically return the plain-binding InvalidArgument. Production Iceberg
writers pass `iceberg_arrow_write_converter()`; please update the two tests as
well, or infer/require the target converter whenever `iceberg_schema` is
non-null.
##########
be/src/core/data_type_serde/data_type_serde.cpp:
##########
@@ -477,6 +477,34 @@ int64_t find_struct_child_index(const ::orc::Type& type,
const std::string& fiel
return -1;
}
+struct RoundedOrcTimestamp {
+ int64_t seconds;
+ uint64_t microseconds;
+ bool carry;
+};
+
+Status round_orc_timestamp_to_microseconds(int64_t seconds, int64_t
nanoseconds,
+ RoundedOrcTimestamp* result) {
+ constexpr int64_t NANOS_PER_SECOND = 1000000000;
+ constexpr int64_t NANOS_PER_MICROSECOND = 1000;
+ constexpr int64_t MICROS_PER_SECOND = 1000000;
+ DORIS_CHECK(result != nullptr);
+ DORIS_CHECK(nanoseconds >= 0 && nanoseconds < NANOS_PER_SECOND);
Review Comment:
[P2] Please return a checked file-data error for an out-of-range nanoseconds
value instead of using `DORIS_CHECK` here. Both ORC timestamp decoders pass
`TimestampVectorBatch::nanoseconds` from the external file directly into this
helper. `DORIS_CHECK` turns malformed file input into a FATAL_ERROR (caught as
a scan failure in the default release path, but process-fatal in debug or with
`exit_on_exception=true`) rather than using the helper's Status channel. This
should be `DataQualityError`/`Corruption`, like the adjacent overflow and
calendar checks.
##########
be/src/core/data_type_serde/data_type_serde.cpp:
##########
@@ -496,8 +524,28 @@ Status decode_timestamp_orc_values(IColumn& nested_column,
const OrcDecodedColum
}
auto& value =
reinterpret_cast<DateV2Value<DateTimeV2ValueType>&>(data[old_data_size + row]);
+ RoundedOrcTimestamp timestamp;
+ auto status = round_orc_timestamp_to_microseconds(
+ orc_batch->data[source_row],
orc_batch->nanoseconds[source_row], ×tamp);
+ if (!status.ok()) {
+ data.resize(old_data_size);
+ return status;
+ }
value.from_unixtime(orc_batch->data[source_row], timezone);
-
value.set_microsecond(cast_set<uint64_t>(orc_batch->nanoseconds[source_row] /
1000));
+ if (!value.is_valid_date()) {
+ data.resize(old_data_size);
+ return Status::DataQualityError(
+ "Decoded ORC timestamp is outside the target timezone
range");
+ }
+ value.set_microsecond(timestamp.microseconds);
+ // Plain ORC TIMESTAMP is a civil value. Carry after timezone
conversion so a fractional
+ // round does not jump backward or skip an hour at a daylight-saving
transition.
+ if (timestamp.carry &&
Review Comment:
[P1] This civil-time carry also handles `TIMESTAMP_INSTANT` when
`enable_mapping_timestamp_tz` is false (the default). `DataTypeDateTimeV2SerDe`
routes both ORC timestamp kinds here. For `1636275599.999999500Z` in
`America/Los_Angeles`, instant rounding must first reach `1636275600Z` and then
decode as `01:00:00 PST`; this code converts the prior second to `01:59:59 PDT`
and returns civil `02:00:00` (and returns nonexistent `02:00` at the spring gap
instead of `03:00`). Please branch on `file_type`: retain civil carry for plain
TIMESTAMP, but apply epoch carry before conversion for TIMESTAMP_INSTANT,
including the unmapped DATETIMEV2 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]
