github-actions[bot] commented on code in PR #68786:
URL: https://github.com/apache/doris/pull/68786#discussion_r4225722491
##########
fe/be-java-extensions/trino-connector-scanner/src/main/java/org/apache/doris/trinoconnector/TrinoConnectorColumnValue.java:
##########
@@ -171,7 +172,8 @@ public LocalDateTime getDateTime() {
@Override
public LocalDateTime getTimeStampTz() {
Object o = trinoType.getObjectValue(connectorSession, block, position);
- return ((SqlTimestampWithTimeZone)
o).toZonedDateTime().toLocalDateTime();
+ // JNI encodes UTC components; dropping the source zone would shift
the instant.
+ return LocalDateTime.ofInstant(((SqlTimestampWithTimeZone)
o).toZonedDateTime().toInstant(), ZoneOffset.UTC);
Review Comment:
[P2] Bound native Trino zoned values before JNI packing. Trino can return a
`TIMESTAMP WITH TIME ZONE` at UTC year 10000. This new conversion passes that
year to `VectorColumn.appendTimeStampTz`, which bit-packs it without validation
even though Doris TIMESTAMPTZ ends at year 9999. A scan can publish an invalid
value (including in arrays). Reject the out-of-range instant before appending,
or return NULL only with nullable output, and cover this boundary. The existing
JDBC Trino range thread does not guard this native scanner.
##########
fe/be-java-extensions/hadoop-hudi-scanner/src/main/java/org/apache/doris/hudi/HadoopHudiColumnValue.java:
##########
@@ -113,7 +113,21 @@ public byte getByte() {
@Override
public LocalDateTime getTimeStampTz() {
- return ((Timestamp) fieldData).toLocalDateTime();
+ // Different Hudi readers expose the same instant through different
physical carriers.
+ Instant instant;
+ if (fieldData instanceof Timestamp) {
+ instant = ((Timestamp) fieldData).toInstant();
+ } else if (fieldData instanceof TimestampWritableV2) {
+ org.apache.hadoop.hive.common.type.Timestamp timestamp =
+ ((TimestampObjectInspector)
fieldInspector).getPrimitiveJavaObject(fieldData);
+ instant = Instant.ofEpochSecond(timestamp.toEpochSecond(),
timestamp.getNanos());
+ } else {
+ long value = ((LongWritable) fieldData).get();
+ long units = dorisType.getPrecision() == 3 ? 1000L : 1_000_000L;
+ instant = Instant.ofEpochSecond(Math.floorDiv(value, units),
+ Math.floorMod(value, units) * (1_000_000_000L / units));
+ }
+ return LocalDateTime.ofInstant(instant, java.time.ZoneOffset.UTC);
Review Comment:
[P2] Check Hudi UTC years before JNI timestamp packing. Avro timestamp
millis/micros can encode an instant in year 10000, and this new getter returns
a UTC `LocalDateTime` for it. `HadoopHudiJniScanner` then calls
`VectorColumn.appendTimeStampTz`, which packs the year without validation
although Doris TIMESTAMPTZ ends at 9999. Reject an out-of-range value (or
produce NULL only with nullable output) before append, and test the boundary.
Existing JDBC range comments do not cover the Hudi scanner.
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/TrinoTypeHandler.java:
##########
@@ -107,7 +118,71 @@ public ColumnValueConverter getOutputConverter(ColumnType
columnType, String rep
}
}
- private Object convertArray(List<?> input, ColumnType childType) {
- return input;
+ private List<?> convertArray(List<?> array, ColumnType type) {
+ if (array == null) {
+ return null;
+ }
+ if (array.isEmpty()) {
+ return Collections.emptyList();
+ }
+ switch (type.getType()) {
+ case DATE:
+ case DATEV2: {
+ List<LocalDate> result = Lists.newArrayList();
+ for (Object element : array) {
+ result.add(element != null ? ((Date)
element).toLocalDate() : null);
+ }
+ return result;
+ }
+ case TIMESTAMPTZ: {
+ List<LocalDateTime> result = Lists.newArrayList();
+ // Trino JDBC exposes timestamp-with-zone array elements as
java.sql.Timestamp.
+ for (Object element : array) {
+ result.add(element == null ? null
+ : checkedUtcTimestamp(((Timestamp)
element).toInstant()));
Review Comment:
[P1] Decode PrestoDB zoned array elements from the driver's actual carrier.
`PRESTO` now uses `PrestoTypeHandler`, which inherits this array converter, but
the official PrestoDB JDBC driver returns `String` elements for
`array(timestamp with time zone)` (its JSON fixup preserves them as strings and
`getArray()` does no timestamp conversion). The new `((Timestamp) element)`
therefore throws `ClassCastException` for any non-null element, so these tables
cannot be scanned. Handle PrestoDB's zoned text while preserving its offset,
and cover an array read with the official driver.
##########
be/src/exec/sink/writer/iceberg/viceberg_table_writer.cpp:
##########
@@ -626,6 +707,28 @@ void VIcebergTableWriter::_cleanup_closed_files() {
_closed_files.clear();
}
+std::string VIcebergTableWriter::_partition_value_to_human_string(size_t index,
+ const
std::any& value) {
+ auto& partition = _iceberg_partition_columns[index];
+ auto& transform = partition.partition_column_transform();
+ auto type = transform.get_result_type();
+ if (type->get_primitive_type() != TYPE_VARBINARY || !value.has_value()) {
+ return transform.to_human_string(type, value);
+ }
+ const auto& bytes = std::any_cast<const std::string&>(value);
+ if (_schema->find_type(partition.field().source_id())->type_id() ==
iceberg::TypeID::UUID) {
+ if (bytes.size() != 16) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT, "UUID partition
requires 16 bytes");
+ }
+ boost::uuids::uuid uuid;
+ std::copy(bytes.begin(), bytes.end(), uuid.begin());
+ return boost::uuids::to_string(uuid);
+ }
+ std::string encoded;
+ base64_encode(bytes, &encoded);
Review Comment:
[P1] Keep binary partition writer keys distinct from NULL. The new Base64
path rendering maps bytes `0x9ee965` to the literal `null`, the same path
component used for a NULL partition. If one INSERT contains both values,
`_partitions_to_writers` reuses one writer for `key=null`, but
`_create_partition_writer` records partition metadata from only its first row.
The resulting Iceberg file contains rows from two partitions and can be skipped
by selective scans. Key writers by typed partition values, or encode binary
paths so they cannot collide with NULL; add a mixed-value write/read test.
##########
fe/fe-connector/fe-connector-maxcompute/src/main/java/org/apache/doris/connector/maxcompute/MCTypeMapping.java:
##########
@@ -197,6 +199,8 @@ private static TypeInfo toMcScalarType(String name,
ConnectorType type) {
case "DATETIME":
case "DATETIMEV2":
return TypeInfoFactory.DATETIME;
+ case "TIMESTAMPTZ":
Review Comment:
[P2] Support nested zoned timestamps in the MaxCompute writer. This new
mapping also accepts `ARRAY`/`MAP`/`STRUCT` children recursively, so a nested
`TIMESTAMPTZ` becomes a MaxCompute `TIMESTAMP`. Complex writes call
`MaxComputeJniWriter.writeListElement`, which has no `TimeStampVector` case and
eventually casts it to `VarCharVector`; any non-null nested value fails with
`ClassCastException`. Write timestamp children in the configured microsecond
UTC unit, and cover a nested write as well as the scalar case.
##########
fe/fe-connector/fe-connector-fluss/src/main/java/org/apache/doris/connector/fluss/FlussTypeMapping.java:
##########
@@ -185,10 +173,8 @@ public ConnectorType visit(TimestampType timestampType) {
@Override
public ConnectorType visit(LocalZonedTimestampType
localZonedTimestampType) {
int scale = clampScale(localZonedTimestampType.getPrecision());
- if (options.isMapTimestampTz()) {
- return ConnectorType.of("TIMESTAMPTZ", scale, 0);
- }
- return ConnectorType.of("DATETIMEV2", scale, 0);
+ // LTZ stores an instant; a legacy marker must not reinterpret it as
local wall-clock fields.
+ return ConnectorType.of("TIMESTAMPTZ", scale, 0);
Review Comment:
[P2] Bound Fluss zoned instants before JNI packing. Fluss 1.0.0 permits a
local `9999-12-31 23:59:59 -14:59` value, whose stored instant is in UTC year
10000. This new unconditional `TIMESTAMPTZ` mapping sends it through
`FlussColumnValue.getTimeStampTz` to `VectorColumn.appendTimeStampTz`, which
packs that year without validation although Doris ends at 9999. Reject an
out-of-range instant before append (or emit NULL only with nullable output),
and cover this boundary. The other scanner range findings do not cover Fluss.
##########
fe/fe-connector/fe-connector-hms/src/main/java/org/apache/doris/connector/hms/HmsTypeMapping.java:
##########
@@ -332,8 +335,9 @@ public static final class Options {
public Options(int timeScale, boolean mapBinaryToVarbinary,
boolean mapTimestampTz) {
this.timeScale = timeScale;
- this.mapBinaryToVarbinary = mapBinaryToVarbinary;
- this.mapTimestampTz = mapTimestampTz;
+ // External payload types retain bytes and instant semantics
regardless of legacy options.
+ this.mapBinaryToVarbinary = true;
+ this.mapTimestampTz = true;
Review Comment:
[P2] Validate Hive ORC V1 zoned timestamps before packing. This new
unconditional LTZ mapping exposes Hive `TIMESTAMP WITH LOCAL TIME ZONE` as
Doris TIMESTAMPTZ. With `enable_file_scanner_v2=false`, ORC `TIMESTAMP_INSTANT`
reaches V1 `_decode_timestamp_tz_column`, which calls unchecked
`from_unixtime`; Hive can store a UTC year-10000 epoch, so this path publishes
an invalid Doris value. The V2 ORC SerDe rejects that year correctly. Check the
UTC year in V1 or route these scans to V2, and test an out-of-range Hive LTZ
row with V1 enabled (including a nested value).
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonTypeMapping.java:
##########
@@ -319,8 +320,9 @@ public static final class Options {
private final boolean mapTimestampTz;
public Options(boolean mapBinaryToVarbinary, boolean mapTimestampTz) {
- this.mapBinaryToVarbinary = mapBinaryToVarbinary;
- this.mapTimestampTz = mapTimestampTz;
+ // The flags are retained for callers, but external logical
mappings are mandatory.
+ this.mapBinaryToVarbinary = true;
+ this.mapTimestampTz = true;
Review Comment:
[P2] Bound Paimon zoned instants before JNI packing. Paimon 1.4.2 permits a
local `9999-12-31 23:59:59 -14:59` value, whose UTC instant is in year 10000.
This newly unconditional `TIMESTAMPTZ` mapping lets a `force_jni_scanner` scan
pass that value through `PaimonColumnValue.getTimeStampTz` to
`VectorColumn.appendTimeStampTz`, which packs the year without validation
although Doris ends at 9999. Reject the out-of-range instant before append (or
emit NULL only with nullable output), and cover this boundary. The other
scanner range findings do not cover Paimon.
--
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]
