github-actions[bot] commented on code in PR #68786:
URL: https://github.com/apache/doris/pull/68786#discussion_r4237058215
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java:
##########
@@ -124,28 +124,25 @@ 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");
+ // Preserve logical UUID semantics independently of the binary
mapping option.
+ return ConnectorType.of("UUID");
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:
[P1] Keep V1 equality deletes on BINARY fields readable. With
`enable_file_scanner_v2=false`, the new BINARY-to-VARBINARY mapping reaches
both V1 delete implementations. A one-field equality delete calls
`create_set(TYPE_VARBINARY)` and throws NOT_IMPLEMENTED_ERROR; a composite
delete containing BINARY calls `ColumnVarbinary::update_hashes_with_value`
through `ColumnNullable`, which also throws because the hash hook is
unsupported. Before this change BINARY mapped to STRING by default, so these
scans worked. The existing UUID thread concerns a different type that now has
native TYPE_UUID set support. Add V1 BINARY equality-delete coverage for single
and composite keys and provide byte-aware delete handling.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java:
##########
@@ -124,28 +124,25 @@ 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");
+ // Preserve logical UUID semantics independently of the binary
mapping option.
+ return ConnectorType.of("UUID");
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] Handle VARBINARY inside array hash keys. Iceberg LIST<BINARY> now
becomes ARRAY<VARBINARY>, but GROUP BY and DISTINCT only reject a top-level
VARBINARY key. A distributed `SELECT payload, COUNT(*) FROM t GROUP BY payload`
therefore requests a hash exchange on the array; for any nonempty payload,
`ColumnArray` delegates CRC hashing to `ColumnVarbinary`, whose hook throws
NOT_IMPLEMENTED_ERROR. The former ARRAY<STRING> key worked. The same
top-level-only guard admits equality joins on arrays. Provide nested byte
hashing or recursively reject these keys during FE analysis, and cover GROUP
BY, DISTINCT, and joins on nonempty arrays.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java:
##########
@@ -124,28 +124,25 @@ 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");
+ // Preserve logical UUID semantics independently of the binary
mapping option.
+ return ConnectorType.of("UUID");
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] Keep windows partitioned by BINARY executable. A query such as
`row_number() OVER (PARTITION BY payload ORDER BY id)` on an Iceberg BINARY
column still passes FE analysis, but the window requires a hash exchange on the
newly mapped VARBINARY key. The BE partitioner calls `ColumnVarbinary`'s
unsupported CRC hash hook and fails; with a filtered row-number query, the
default PartitionTopN rewrite also reaches `get_hash_key_type(TYPE_VARBINARY)`
and fails. The former default STRING mapping supported both paths. Add
byte-aware window partition hashing or reject this key at analysis, and cover
ordinary and filtered window plans.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java:
##########
@@ -124,28 +124,25 @@ 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");
+ // Preserve logical UUID semantics independently of the binary
mapping option.
+ return ConnectorType.of("UUID");
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] Keep INTERSECT and EXCEPT on BINARY columns executable. Two Iceberg
BINARY inputs now retain VARBINARY through set-operation binding and produce
IntersectNode or ExceptNode, but `SetSharedState::hash_table_init` calls
`get_hash_key_type`, which throws NOT_IMPLEMENTED_ERROR for VARBINARY. For
example, `SELECT payload FROM t1 INTERSECT SELECT payload FROM t2` now fails at
BE open, whereas the former default STRING mapping could build the set. The FE
VARBINARY checks cover scalar grouping and joins, but not these set operations.
Add byte-aware set hashing or reject these operands during analysis, with
INTERSECT and EXCEPT coverage.
--
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]
