github-actions[bot] commented on code in PR #68786:
URL: https://github.com/apache/doris/pull/68786#discussion_r4220383020
##########
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 LIST partition metadata for binary identity columns. This new
VARBINARY type reaches PluginDrivenMvccExternalTable.listPartitions for an
Iceberg identity BINARY/FIXED partition, but
PartitionKey.createListPartitionKeyWithTypes calls
LiteralExprUtils.createLiteral, which has no VARBINARY case. Every non-null
binary partition is skipped and the table is reported UNPARTITIONED, disabling
FE partition metadata and pruning. The listing currently carries
ByteBuffer.toString() text, so it needs a typed byte encoding and a VARBINARY
partition literal path. Please cover an identity binary partition; the added
suite covers bucket/truncate transforms instead.
##########
fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcQueryBuilder.java:
##########
@@ -258,6 +335,21 @@ private boolean collectFilters(ConnectorExpression expr,
List<String> clauses,
* Mirrors the old JdbcScanNode.shouldPushDownConjunct() guards.
*/
private boolean shouldPushDownExpression(ConnectorExpression expr) {
+ // Stripped CASTs lose the Doris session zone when an instant is
compared with wall-clock fields.
+ if (hasInstant(expr) && hasWallClockColumn(expr)) {
Review Comment:
[P1] Keep session-zone casts out of literal predicate pushdown. A
TIMESTAMPTZ column compared with a DATETIMEV2/DATE literal still passes this
guard because hasWallClockColumn only recognizes column refs, and the
expression converter strips CAST. In a +08 Doris session, CAST(ts AS
DATETIMEV2) > CAST('2020-01-02 04:00:00' AS DATETIMEV2) keeps ts=00:00Z
locally, but MySQL's UTC session evaluates the pushed ts > '04:00' as false;
LIMIT can then be pushed as well. Please reject this literal form or preserve
the cast in remote SQL, and cover a non-UTC session.
##########
regression-test/suites/external_table_p0/iceberg/write/test_iceberg_write_binary_partitions.groovy:
##########
@@ -0,0 +1,81 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_iceberg_write_binary_partitions",
"p0,external,iceberg,external_docker,external_docker_iceberg") {
+ if
(!context.config.otherConfigs.get("enableIcebergTest")?.toString()?.equalsIgnoreCase("true"))
{
+ return
+ }
+ String externalEnvIp = context.config.otherConfigs.get("externalEnvIp")
+ String restPort = context.config.otherConfigs.get("iceberg_rest_uri_port")
+ String minioPort = context.config.otherConfigs.get("iceberg_minio_port")
+ String endpoint = context.config.otherConfigs.get("iceberg_minio_endpoint")
+ ?: "http://${externalEnvIp}:${minioPort}";
+ String catalogName = "test_iceberg_write_binary_partitions"
+ String dbName = "binary_partition_roundtrip"
+ def hexValues = ["C3A900FF", "", "616263", "00FF80",
"000102030405060708090A0B0C0D0E0FFF80"]
+ def expected = hexValues.withIndex().collect { value, index -> [index + 1,
value] } + [[6, null]]
+ try {
+ sql "DROP CATALOG IF EXISTS ${catalogName}"
+ sql """CREATE CATALOG ${catalogName} PROPERTIES (
+ "type"="iceberg", "iceberg.catalog.type"="rest",
+ "uri"="http://${externalEnvIp}:${restPort}";,
+ "s3.endpoint"="${endpoint}", "s3.access_key"="admin",
"s3.secret_key"="password",
+ "s3.region"="us-east-1", "use_path_style"="true")"""
+ sql "SWITCH ${catalogName}"
+ sql "CREATE DATABASE IF NOT EXISTS ${dbName}"
+ sql "USE ${dbName}"
+ for (String format : ["parquet", "orc"]) {
+ for (String transform : ["bucket", "truncate"]) {
+ String table = "binary_${format}_${transform}"
+ String expression = transform == "bucket" ? "bucket(16,
binary_key)" : "truncate(1, binary_key)"
+ sql "DROP TABLE IF EXISTS ${table}"
+ try {
+ spark_iceberg """CREATE TABLE demo.${dbName}.${table} (id
INT, binary_key BINARY)
+ USING iceberg PARTITIONED BY (${expression})
+ TBLPROPERTIES ('format-version'='2',
'write.format.default'='${format}')"""
+ // Include a prefix inside UTF-8, embedded NULs, invalid
UTF-8, and arena-backed bytes.
+ String rows = hexValues.withIndex().collect { value, index
->
+ "(${index + 1}, X'${value}')"
+ }.join(", ") + ", (6, NULL)"
+ for (String operation : ["INSERT INTO", "INSERT OVERWRITE
TABLE"]) {
+ sql "${operation} ${table} VALUES ${rows}"
+ assertEquals(expected, sql("SELECT id,
from_hex(binary_key) FROM ${table} ORDER BY id"))
Review Comment:
[P3] Capture fixed regression results and retain failed tables. This suite
and test_iceberg_write_timestamp_partitions use hardcoded assertEquals for
deterministic Doris SELECT results without generated .out files, then drop
their tables/catalogs after execution. New fixed checks in
test_mariadb_jdbc_catalog, both test_mysql_jdbc_catalog suites,
test_hive_basic_type, and test_trino_mysql likewise bypass .out; test_hive_orc
adds a post-run database drop. Repository rules require ordered qt results
generated by the runner and drops before use only. Please update these new
checks and cleanup paths.
--
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]
