github-actions[bot] commented on code in PR #68532:
URL: https://github.com/apache/doris/pull/68532#discussion_r4124289934
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -750,21 +755,24 @@ private static Type
icebergPrimitiveTypeToDorisType(org.apache.iceberg.types.Typ
case STRING:
return Type.STRING;
case UUID:
- return enableMappingVarbinary ?
ScalarType.createVarbinaryType(16) : Type.STRING;
+ return ScalarType.createVarbinaryType(16);
case BINARY:
- return enableMappingVarbinary ?
ScalarType.createVarbinaryType(VarBinaryType.MAX_VARBINARY_LENGTH)
- : Type.STRING;
Review Comment:
[P1] This makes existing Iceberg `BINARY` bucket partitions unwritable. An
insert into a table partitioned by `bucket[4](binary_col)` now sends
`TYPE_VARBINARY` to `PartitionColumnTransforms::create()`, whose early guard
rejects every non-identity VARBINARY transform before the writer can open. With
the previous default mapping, the same ASCII-byte insert used the STRING bucket
transform and Arrow BINARY writer. Please add a byte-wise bucket transform (and
handle truncate(binary) similarly) or preserve the working write binding for
these partition specs; leaving binary SQL comparisons unsupported does not
prevent the writer from needing these Iceberg transforms.
##########
be/src/core/data_type_serde/data_type_string_serde.cpp:
##########
@@ -568,7 +568,12 @@ Status
DataTypeStringSerDeBase<ColumnType>::write_column_to_iceberg_arrow(
arrow::ArrayBuilder* array_builder, int64_t start, int64_t end,
const cctz::time_zone& ctz) const {
if (!is_iceberg_uuid_field(field)) {
- // Keep the existing CHAR/STRING fixed-binary binding until external
type mappings change.
+ // Iceberg fixed(N) is a byte binding, so text padding must not change
its physical value.
+ if (array_builder->type()->id() == arrow::Type::FIXED_SIZE_BINARY) {
Review Comment:
[P1] This rejects Iceberg FIXED(N) writes during the documented BE-first
rolling upgrade. With an older FE and a catalog whose
`enable.mapping.varbinary` marker is absent/false, the FE still binds FIXED(N)
as CHAR(N); the old BE wrote an exact-width CHAR value through
`write_column_to_arrow` into the fixed-size Arrow builder. After upgrading a
BE, this new guard returns `InvalidArgument` for every non-UUID fixed-size
field before that writer runs, so an unpartitioned Parquet table with FIXED(4)
fails on a four-byte insert until the FE is upgraded. Please retain the legacy
CHAR-to-FIXED binding for old FE plans, or add a compatible version gate.
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/MySQLJdbcExecutor.java:
##########
@@ -70,6 +73,14 @@ protected void abortReadConnection(Connection connection,
ResultSet resultSet)
@Override
protected void initializeStatement(Connection conn, JdbcDataSourceConfig
config, String sql) throws SQLException {
+ if (usesMySqlTimestampProtocol(config)) {
+ // MySQL sends TIMESTAMP as session-local fields. Read and bind
those fields in UTC
+ // without the driver's legacy Calendar conversion, which may
apply the JVM offset.
+ // Set this on every checkout because pooled sessions may have
been modified by a query.
+ try (Statement timezoneStatement = conn.createStatement()) {
Review Comment:
[P1] This changes old-FE MySQL TIMESTAMP reads during the documented
BE-first upgrade. With the legacy mapping marker false, the FE still plans
TIMESTAMP as DATETIMEV2, whose unchanged reader copies `LocalDateTime` fields.
This new statement setup forces every MySQL/OceanBase MySQL-mode session to
UTC; a value stored at `2024-01-01T00:00Z` that an existing `+08:00` session
returned as `08:00` now returns `00:00` after only the BE is upgraded. Please
gate the UTC reset on an instant-aware FE plan or preserve the legacy session
behavior until the FE is upgraded.
##########
fe/be-java-extensions/max-compute-connector/src/main/java/org/apache/doris/maxcompute/MaxComputeJniWriter.java:
##########
@@ -565,8 +565,24 @@ private void fillArrowVectorStreaming(VectorSchemaRoot
root, int colIdx, OdpsTyp
vec.setValueCount(numRows);
break;
}
- case DATETIME:
case TIMESTAMP: {
+ // TIMESTAMPTZ's JNI carrier is UTC and must retain
microseconds on write.
+ org.apache.arrow.vector.TimeStampVector vec =
+ (org.apache.arrow.vector.TimeStampVector)
root.getVector(colIdx);
+ vec.allocateNew(numRows);
+ for (int i = 0; i < numRows; i++) {
+ if (vc.isNullAt(rowOffset + i)) {
+ vec.setNull(i);
+ } else {
Review Comment:
[P1] Keep legacy MaxCompute TIMESTAMP writes correct during the BE-first
upgrade. The old FE maps this column to DATETIMEV2(6), and the previous writer
interpreted its civil fields in the BE JVM zone before storing an instant. This
new `getTimeStampTz()` path reads the same packed old-FE fields but interprets
them as UTC. With an Asia/Shanghai BE JVM, an old FE inserting `2024-01-01
08:00:00` previously stored `00:00Z`; after only the BE is upgraded it stores
`08:00Z` through the new microsecond Arrow vector. The base FE already supplies
the required transaction/session IDs, so this write reaches the new branch.
Please use the FE carrier type or a version signal to retain legacy zone
conversion for DATETIMEV2 while using UTC for TIMESTAMPTZ.
##########
fe/be-java-extensions/hadoop-hudi-scanner/src/main/java/org/apache/doris/hudi/HadoopHudiColumnValue.java:
##########
@@ -131,25 +145,9 @@ public LocalDate getDate() {
public LocalDateTime getDateTime() {
if (fieldData instanceof Timestamp) {
return ((Timestamp) fieldData).toLocalDateTime();
- } else if (fieldData instanceof TimestampWritableV2) {
- return
LocalDateTime.ofInstant(Instant.ofEpochSecond((((TimestampObjectInspector)
fieldInspector)
- .getPrimitiveJavaObject(fieldData)).toEpochSecond()),
zoneId);
- } else {
- long datetime = ((LongWritable) fieldData).get();
- long seconds;
- long nanoseconds;
- if (dorisType.getPrecision() == 3) {
- seconds = datetime / 1000;
- nanoseconds = (datetime % 1000) * 1000000;
- } else if (dorisType.getPrecision() == 6) {
- seconds = datetime / 1000000;
- nanoseconds = (datetime % 1000000) * 1000;
- } else {
- throw new RuntimeException("Hoodie timestamp only support
milliseconds and microseconds, "
- + "wrong precision = " + dorisType.getPrecision());
- }
- return LocalDateTime.ofInstant(Instant.ofEpochSecond(seconds,
nanoseconds), zoneId);
}
+ // DATETIMEV2 now denotes local-timestamp annotations: decode their
fields without a zone shift.
Review Comment:
[P1] Preserve old-FE Hudi DATETIMEV2 results during the BE-first upgrade. An
older FE maps Avro timestamp-millis/micros to DATETIMEV2, so MOR JNI scans call
`getDateTime()`. For LongWritable and TimestampWritableV2 values, the previous
method rendered the instant in the supplied session zone; this delegation now
renders UTC fields. With an Asia/Shanghai session, an epoch-zero value changes
from `1970-01-01 08:00:00` to `1970-01-01 00:00:00` after only the BE is
upgraded. Please retain the legacy zone conversion for old-FE plans while
keeping UTC-field decoding for new TIMESTAMPTZ plans.
--
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]
