github-actions[bot] commented on code in PR #67784:
URL: https://github.com/apache/doris/pull/67784#discussion_r4023027428
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/MySQLJdbcExecutor.java:
##########
@@ -70,6 +77,14 @@ protected void abortReadConnection(Connection connection,
ResultSet resultSet)
@Override
protected void initializeStatement(Connection conn, JdbcDataSourceConfig
config, String sql) throws SQLException {
+ if (config.getTableType() == TOdbcTableType.MYSQL) {
Review Comment:
[P1] Apply the UTC timestamp contract to OceanBase MySQL mode
`JdbcExecutorFactory` sends both `MYSQL` and `OCEANBASE` through this
executor, and OceanBase MySQL mode delegates schema mapping to
`JdbcMySQLClient`, which now maps remote `TIMESTAMP` to `TIMESTAMPTZ`. However,
this guard and the new getter/setter guards below recognize only `MYSQL`. An
OceanBase scan therefore skips the UTC session, uses `getTimestamp(int)`
instead of the UTC-`Calendar` overload, and writes through the default-zone
setter, so the same instant can shift with the connection/JVM timezone. Please
treat `OCEANBASE` as MySQL-compatible in all three branches and add a non-UTC
read/write regression.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/client/JdbcPostgreSQLClient.java:
##########
@@ -151,8 +151,8 @@ protected Type jdbcTypeToDoris(JdbcFieldSchema fieldSchema)
{
if (scale > 6) {
scale = 6;
}
- return enableMappingTimestampTz ?
ScalarType.createTimeStampTzType(scale)
- : ScalarType.createDatetimeV2Type(scale);
+ // Never discard the instant semantics declared by PostgreSQL
timestamptz.
+ return ScalarType.createTimeStampTzType(scale);
Review Comment:
[P1] Convert PostgreSQL timestamptz array elements before JNI
`convertArrayType()` recursively reuses this mapping, so `_timestamptz` now
becomes `ARRAY<TIMESTAMPTZ>`. [The pinned pgjdbc 42.7.13
decoder](https://github.com/pgjdbc/pgjdbc/blob/REL42.7.13/pgjdbc/src/main/java/org/postgresql/jdbc/ArrayDecoding.java#L351-L392)
returns `Timestamp[]` for TIMESTAMPTZ arrays, but
`PostgreSQLJdbcExecutor.convertArray()` has no TIMESTAMPTZ case and leaves
those elements unchanged. `VectorColumn.appendArray()` then allocates a
`LocalDateTime[]` for the TIMESTAMPTZ child, so assigning the first non-null
`Timestamp` throws `ArrayStoreException`. Please convert these elements through
their instant to UTC `LocalDateTime` (including nested arrays) and add
null/multidimensional coverage.
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/SQLServerJdbcExecutor.java:
##########
@@ -68,6 +73,21 @@ protected void initializeBlock(int columnCount, String[]
replaceStringList, int
@Override
protected Object getColumnValue(int columnIndex, ColumnType type, String[]
replaceStringList) throws SQLException {
switch (type.getType()) {
+ case TIMESTAMPTZ: {
+ // JNI carries instants as UTC components, not the source
zone's wall clock.
+ if (!useLegacyTimestampRead) {
+ try {
+ OffsetDateTime value = resultSet.getObject(columnIndex
+ 1, OffsetDateTime.class);
+ return value == null ? null :
LocalDateTime.ofInstant(value.toInstant(), ZoneOffset.UTC);
+ } catch (SQLFeatureNotSupportedException e) {
Review Comment:
[P1] Catch the exception emitted by legacy SQL Server drivers
This fallback is unreachable with the legacy Microsoft drivers it is meant
to support. In [mssql-jdbc
6.4](https://github.com/microsoft/mssql-jdbc/blob/v6.4.0/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerResultSet.java#L2173-L2253),
an unsupported `getObject(index, type)` target throws the driver's ordinary
`SQLServerException`, not `SQLFeatureNotSupportedException`, and
`OffsetDateTime` is not a supported target (7.0 behaves the same). A catalog
using either driver therefore fails the first `datetimeoffset` read instead of
reaching `getTimestamp()`, while the new unit test passes only because it mocks
a different exception. Please gate the typed getter by driver capability or
recognize the driver's specific unsupported-conversion `SQLException`, and test
that real exception shape.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/parser/VarBinaryLiteralParserTest.java:
##########
@@ -132,7 +136,7 @@ public void testCreateTableVarbinaryWithLength() {
Assertions.assertTrue(plan instanceof
org.apache.doris.nereids.trees.plans.commands.CreateTableCommand);
org.apache.doris.nereids.trees.plans.commands.CreateTableCommand cmd =
(org.apache.doris.nereids.trees.plans.commands.CreateTableCommand) plan;
-
Assertions.assertThrows(org.apache.doris.nereids.exceptions.AnalysisException.class,
+ Assertions.assertDoesNotThrow(
Review Comment:
[P1] Keep these assertions aligned with the native-VARBINARY fence
Final-head `CreateTableInfo.validate()` still rejects every top-level
VARBINARY column, and the newly added
`VarBinarySqlSupportTest.testNativeVarbinaryRemainsUnsupported()` explicitly
codifies that contract. These changed success assertions therefore make the
direct, bounded, key, and partition cases fail deterministically. Please
restore rejection assertions for the top-level native cases (while retaining
literal/external coverage), or restore complete native support and update both
test classes consistently.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -716,21 +718,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;
+ // Arbitrary binary payloads are not valid UTF-8 in general,
so exposing them as
+ // STRING makes Arrow clients reject otherwise valid Iceberg
values.
+ return
ScalarType.createVarbinaryType(VarBinaryType.MAX_VARBINARY_LENGTH);
case FIXED:
Types.FixedType fixed = (Types.FixedType) primitive;
- return enableMappingVarbinary ?
ScalarType.createVarbinaryType(fixed.length())
- : ScalarType.createCharType(fixed.length());
+ // Iceberg fixed(N) is an arbitrary N-byte value, not text, so
retain both its
+ // binary semantics and declared width.
+ return ScalarType.createVarbinaryType(fixed.length());
case DECIMAL:
Types.DecimalType decimal = (Types.DecimalType) primitive;
return ScalarType.createDecimalV3Type(decimal.precision(),
decimal.scale());
case DATE:
return ScalarType.createDateV2Type();
case TIMESTAMP:
- if (enableMappingTimestampTz && ((TimestampType)
primitive).shouldAdjustToUTC()) {
+ // Preserve the logical distinction between instants and
wall-clock timestamps.
+ if (((TimestampType) primitive).shouldAdjustToUTC()) {
Review Comment:
[P1] Support TIMESTAMPTZ in Iceberg partition routing
This now maps every Iceberg timestamp-with-zone column to `TIMESTAMPTZ`,
including insert targets, but the BE partition writer still only accepts
`DATETIMEV2`: bucket/year/month/day/hour reject the new type in
`PartitionColumnTransforms::create()`. Identity partitions get slightly
farther, then fail because both `_get_iceberg_partition_value()` and
partition-value serialization also lack `TIMESTAMPTZ`. Thus inserts into any
Iceberg table partitioned by an adjusted timestamp now fail, whereas catalogs
with the old mapping flag off previously routed the column as `DATETIMEV2`.
Please add UTC-instant TIMESTAMPTZ transforms plus identity
extraction/serialization, and cover a partitioned write (including a DST fold
and NULL).
--
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]
