github-actions[bot] commented on code in PR #68786:
URL: https://github.com/apache/doris/pull/68786#discussion_r4227529236
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/MySQLTypeHandler.java:
##########
@@ -124,8 +127,16 @@ public Object getColumnValue(ResultSet rs, int
columnIndex, ColumnType type,
byte[] data = rs.getBytes(columnIndex);
return rs.wasNull() ? null : data;
}
- case TIMESTAMPTZ:
- return rs.getObject(columnIndex, LocalDateTime.class);
+ case TIMESTAMPTZ: {
+ if (usesMySqlTimestampProtocol()) {
+ // The SQL session is UTC, but older drivers retain a
cached server zone and
+ // can shift or truncate
getTimestamp()/getObject(LocalDateTime.class) results.
+ String value = rs.getString(columnIndex);
+ return value == null ? null :
LocalDateTime.parse(value.replace(' ', 'T'));
Review Comment:
[P2] Honor MySQL's zero TIMESTAMP conversion after projecting text. With
`zeroDateTimeBehavior=convertToNull`, `JdbcMySQLConnectorClient` marks
TIMESTAMP nullable, but `JdbcQueryBuilder` now selects `CAST(ts AS CHAR)`, so
the driver returns `0000-00-00 00:00:00` as text for a permitted zero value.
`LocalDateTime.parse` throws here and the scan fails instead of returning NULL.
Handle this text according to the configured conversion policy, and cover a
zero TIMESTAMP read.
##########
fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorPartitionInfo.java:
##########
@@ -125,6 +127,15 @@ public ConnectorPartitionInfo(String partitionName,
long rowCount, long sizeBytes, long lastModifiedMillis, long
fileCount,
List<String> orderedPartitionValues,
List<Boolean> partitionValueNullFlags) {
+ this(partitionName, partitionValues, properties, rowCount, sizeBytes,
lastModifiedMillis, fileCount,
+ orderedPartitionValues, partitionValueNullFlags,
Collections.emptyList());
+ }
+
+ public ConnectorPartitionInfo(String partitionName, Map<String, String>
partitionValues,
Review Comment:
[P2] Bump the connector plugin API version for this new SPI constructor.
`IcebergPartitionUtils.listPartitions` now calls this 10-argument signature,
but connector plugins still declare API 12.0. An older 12.0 FE passes the
major-only version gate and loads its own parent-first SPI class, which lacks
this constructor; partition listing then throws `NoSuchMethodError`. Bump the
major and refresh the SPI surface baseline, or keep the plugin call compatible
with the old signature.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogProperty.java:
##########
@@ -107,37 +109,43 @@ public Map<String, String> getProperties() {
}
/**
- * @return true if varbinary mapping is enabled, false otherwise
+ * @return true because external binary types always retain their binary
semantics
*/
+ @Deprecated
public boolean getEnableMappingVarbinary() {
- return Boolean.parseBoolean(getOrDefault(ENABLE_MAPPING_VARBINARY,
"false"));
+ return true;
Review Comment:
[P2] Keep upgraded followers on the journaled mapping policy until
promotion. During an FE rolling upgrade, an old master can still own a catalog
with `enable.mapping.varbinary=false`, while a new follower replays that value
and this getter nevertheless returns true. The follower is allowed to serve
reads before `migrateVarbinaryMappingProperties()` runs on a new master, so the
two FEs expose STRING and VARBINARY for the same external column (likewise
DATETIMEV2 and TIMESTAMPTZ). Gate follower-visible mapping changes on a
journaled transition, or prevent these mixed-version catalog reads; cover an
old-master/new-follower rollout.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -163,6 +167,10 @@ public class TypeCoercionUtils {
);
private static final Logger LOG =
LogManager.getLogger(TypeCoercionUtils.class);
+ private static final Set<String> UNSUPPORTED_VARBINARY_COLLECTIONS =
ImmutableSet.of(
Review Comment:
[P2] Include VARBINARY map membership in this function guard. An external
binary `payload` can form `map('k', payload)`, and `map_contains_value(map('k',
payload), payload)` passes the FE's generic signature because this set lists
`array_contains` but not its map wrapper. BE `FunctionMapContains` delegates to
array membership, whose dispatch excludes TYPE_VARBINARY, so the query fails at
execution; `map_contains_entry` has the same unsupported equality type. Reject
these map calls during analysis or add byte-aware BE equality, and cover a
nonconstant binary value.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -163,6 +167,10 @@ public class TypeCoercionUtils {
);
private static final Logger LOG =
LogManager.getLogger(TypeCoercionUtils.class);
+ private static final Set<String> UNSUPPORTED_VARBINARY_COLLECTIONS =
ImmutableSet.of(
+ "array_contains", "array_position", "countequal",
"array_distinct", "array_remove",
+ "array_enumerate_uniq", "array_contains_all", "arrays_overlap",
"array_union",
+ "array_except", "array_intersect", "collect_set");
Review Comment:
[P2] Cover VARBINARY elements in group-array set aggregates.
`group_array_union(array(payload))` and `group_array_intersect(array(payload))`
over a newly mapped external binary column pass the FE's `Array<AnyDataType>`
signatures, while this new guard lists only scalar
`array_union`/`array_intersect`. BE
`create_aggregate_function_group_array_impl` dispatches scalar or STRING
elements and returns no function for VARBINARY, so queries that worked via the
old STRING mapping now fail. Add byte-aware aggregate support or a FE legality
check for these group functions, with both binary cases tested.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java:
##########
@@ -124,28 +124,24 @@ private static ConnectorType
fromPrimitive(Type.PrimitiveType primitive,
case STRING:
return ConnectorType.of("STRING");
case UUID:
- return enableMappingVarbinary
- ? ConnectorType.of("VARBINARY", 16, 0) :
ConnectorType.of("STRING");
+ return ConnectorType.of("VARBINARY", 16, 0);
Review Comment:
[P2] Preserve canonical UUID text writes under this new VARBINARY mapping.
An ordinary `INSERT INTO iceberg_uuid VALUES
('00112233-4455-6677-8899-aabbccddeeff')` now casts that text to 36 raw bytes,
and the fixed-size 16-byte Arrow UUID writer rejects it; the former STRING
writer parsed it to 16 bytes.
`PARTITION(id='00112233-4455-6677-8899-aabbccddeeff')` separately fails the new
generic VARBINARY static guard. Normalize UUID text to the same 16 bytes for
ordinary rows and static row/partition values while preserving typed binary
input, and cover both insert forms.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java:
##########
@@ -124,28 +124,24 @@ private static ConnectorType
fromPrimitive(Type.PrimitiveType primitive,
case STRING:
return ConnectorType.of("STRING");
case UUID:
- return enableMappingVarbinary
- ? ConnectorType.of("VARBINARY", 16, 0) :
ConnectorType.of("STRING");
+ return ConnectorType.of("VARBINARY", 16, 0);
case BINARY:
// Iceberg BINARY is unbounded. Emit VARBINARY with NO
explicit length so
// ConnectorColumnConverter applies
ScalarType.MAX_VARBINARY_LENGTH — byte-identical to
// legacy IcebergUtils
createVarbinaryType(VarBinaryType.MAX_VARBINARY_LENGTH). A
// concrete length (e.g. 65535) would render a different
DESCRIBE / SHOW CREATE type.
- return enableMappingVarbinary
- ? ConnectorType.of("VARBINARY") :
ConnectorType.of("STRING");
+ // Binary payloads need not be valid UTF-8.
+ return ConnectorType.of("VARBINARY");
Review Comment:
[P2] Preserve aggregates over newly mapped binary columns. An Iceberg BINARY
`payload` now becomes VARBINARY, so `min(payload)`/`max(payload)` pass FE but
the BE single-value factory throws `NOT_IMPLEMENTED_ERROR`; `min_by(payload,
id)`/`max_by` also pass FE but their separate value factory rejects VARBINARY,
and a binary key has no dispatch. These queries worked via the former STRING
mapping. Add byte-owning support in both aggregate paths, or reject the
unsupported forms during FE analysis, and cover one- and two-argument binary
aggregates.
--
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]
