github-actions[bot] commented on code in PR #68441:
URL: https://github.com/apache/doris/pull/68441#discussion_r4094548609
##########
be/src/core/data_type_serde/data_type_datetimev2_serde.cpp:
##########
@@ -73,8 +73,24 @@ Status decode_timestamp_orc_values(IColumn& nested_column,
const OrcDecodedColum
}
auto& value =
reinterpret_cast<DateV2Value<DateTimeV2ValueType>&>(data[old_data_size + row]);
- value.from_unixtime(orc_batch->data[source_row], timezone);
-
value.set_microsecond(cast_set<uint64_t>(orc_batch->nanoseconds[source_row] /
1000));
+ orc_serde_utils::RoundedOrcTimestamp timestamp;
+ auto status = orc_serde_utils::round_orc_timestamp_to_microseconds(
Review Comment:
[P1] Keep scanner V1 on the same ORC rounding contract
This helper is only used by scanner V2, while the V1 ORC decoder still
assigns `nanoseconds / 1000` in `be/src/format/orc/vorc_reader.h` without
half-up rounding or second carry. That path is still required:
`should_use_file_scanner_v2()` unconditionally rejects `transactional_hive`,
and `_init_orc_reader()` wraps the V1 `OrcReader` for those full-ACID scans.
Consequently the same external value (for example 999999999ns) reads as the
next second through V2 but as `.999999` through transactional Hive/V1, and V1
predicate pushdown can compare the raw nanoseconds against a value different
from the truncated row. Please apply the same checked rounding/carry semantics
to the V1 timestamp and timestamptz decoders, with a V1/transactional
regression.
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/FileFormatUtils.java:
##########
@@ -21,16 +21,49 @@
import org.apache.doris.catalog.PrimitiveType;
import org.apache.doris.catalog.ScalarType;
import org.apache.doris.common.AnalysisException;
+import org.apache.doris.common.DdlException;
import org.apache.doris.common.FeNameFormat;
import com.google.common.base.Strings;
+import java.time.DateTimeException;
+import java.time.ZoneId;
import java.util.List;
+import java.util.Locale;
import java.util.Optional;
import java.util.regex.Matcher;
public class FileFormatUtils {
+ public static final int PARQUET_TIMESTAMP_SEMANTICS_VERSION = 1;
+
+ public static String parseHiveParquetTimeZone(String value) throws
DdlException {
+ String timeZone = value.trim();
+ if (timeZone.isEmpty()) {
+ return "";
+ }
+
+ try {
+ String upperCaseTimeZone = timeZone.toUpperCase(Locale.ROOT);
+ boolean isUtcOrGmt = upperCaseTimeZone.equals("UTC") ||
upperCaseTimeZone.equals("GMT");
+ boolean isShortAlias =
ZoneId.SHORT_IDS.containsKey(upperCaseTimeZone)
+ || upperCaseTimeZone.equals("CST") ||
upperCaseTimeZone.equals("PRC");
+ if (!isUtcOrGmt && isShortAlias) {
+ throw new DateTimeException("Ambiguous short timezone aliases
are not supported");
+ }
+ String standardizedTimeZone =
TimeUtils.checkTimeZoneValidAndStandardize(timeZone);
Review Comment:
[P2] Accept slashless TZDB zone IDs
The error contract allows IANA zones, and the explicit alias check
intentionally leaves IDs such as `CET` alone, but this call rejects every
slashless name that is not in Doris's alias map before `ZoneId.of()` gets a
chance to validate it. `CET` (as well as `EET`/`WET`) is a valid TZDB `ZoneId`
with named-zone rules, so the new catalog/TVF property rejects a legitimate
configuration. Please send named IDs through `ZoneId.of()` after rejecting the
genuinely ambiguous short aliases, while retaining the existing Doris range
validation for numeric and UTC/GMT-prefixed offsets, and add a slashless TZDB
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]
