github-actions[bot] commented on code in PR #65868: URL: https://github.com/apache/doris/pull/65868#discussion_r3674423769
########## fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonWriteTarget.java: ########## @@ -0,0 +1,123 @@ +// 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. + +package org.apache.doris.datasource.paimon; + +import org.apache.doris.catalog.Column; +import org.apache.doris.catalog.Type; +import org.apache.doris.common.AnalysisException; + +import com.google.common.collect.ImmutableList; +import org.apache.paimon.table.FileStoreTable; +import org.apache.paimon.table.Table; +import org.apache.paimon.types.DataField; +import org.apache.paimon.types.DataTypeRoot; + +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; + +/** + * One immutable view of the current Paimon write target. + * + * <p>A time-travel snapshot attached to the Doris table is read-side state and must not affect + * sink analysis. Capturing the latest remote table once also keeps schema binding, writer + * distribution and the serialized JNI table on the same table generation. + */ +public final class PaimonWriteTarget { + private final PaimonExternalTable dorisTable; + private final FileStoreTable table; + private final List<Column> schema; + private final Map<String, Column> columnsByName; + private final Map<String, Type> columnTypes; + private final Set<String> partitionColumnNames; + + private PaimonWriteTarget(PaimonExternalTable dorisTable, FileStoreTable table) { + this.dorisTable = dorisTable; + this.table = table; + PaimonExternalCatalog catalog = (PaimonExternalCatalog) dorisTable.getCatalog(); + + ImmutableList.Builder<Column> schemaBuilder = ImmutableList.builder(); + Map<String, Column> columns = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + Map<String, Type> types = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + for (DataField field : table.rowType().getFields()) { + Type type = PaimonUtil.paimonTypeToDorisType( Review Comment: [P1] Map Paimon VARIANT before sink coercion Every target field is converted through `paimonTypeToDorisType` here, but that switch has no `DataTypeRoot.VARIANT` case and returns `Type.UNSUPPORTED`. `BindSink` therefore tries to coerce the input against an unsupported target before the BE writer opens, so a real Paimon VARIANT insert cannot reach the `GenericVariant` conversion fixed in the earlier thread. Please map Paimon VARIANT to `Type.VARIANT` (including nested projections) and cover the production FE binding/INSERT path, not only the converter helper. ########## fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVPartitionUtil.java: ########## @@ -361,13 +362,40 @@ public static boolean isSyncWithPartitions(MTMVRefreshContext context, String mt public static String generatePartitionName(PartitionKeyDesc desc) { Matcher matcher = PARTITION_NAME_PATTERN.matcher(desc.toSql()); String partitionName = PARTITION_NAME_PREFIX + matcher.replaceAll("").replaceAll("\\,", "_"); + // The legacy readable name removes SQL quotes. Without this marker, a typed NULL and + // the string literal "NULL" both become p_NULL even though they are distinct list + // partition values. Preserve the existing NULL partition name and disambiguate only + // the literal value. + if (containsLiteralNull(desc)) { Review Comment: [P1] Keep MTMV partition names injective The literal string `NULL` is now renamed to `p_NULL_literal`, but the distinct legal string value `NULL,literal` already sanitizes to that same name because commas are preserved and then replaced with underscores. If a Paimon table contains both partitions, MTMV creation/refresh builds two `SinglePartitionDesc`s with one Doris partition name, so one partition cannot be represented correctly. Please derive the disambiguator from the complete typed partition identity (for example with collision-resistant escaping or hashing) and test typed NULL, literal `NULL`, and a normal value that sanitizes to the chosen suffix. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonUtil.java: ########## @@ -150,71 +145,129 @@ public static List<InternalRow> read( return rows; } - public static PaimonPartitionInfo generatePartitionInfo(List<Column> partitionColumns, - List<Partition> paimonPartitions, boolean legacyPartitionName) { + public static PaimonPartitionInfo generatePartitionInfo(Table table, List<Column> partitionColumns, + List<PartitionEntry> partitionEntries) { - if (CollectionUtils.isEmpty(partitionColumns) || paimonPartitions.isEmpty()) { + if (CollectionUtils.isEmpty(partitionColumns) || partitionEntries.isEmpty()) { return PaimonPartitionInfo.EMPTY; } - Map<String, PartitionItem> nameToPartitionItem = Maps.newHashMap(); - Map<String, Partition> nameToPartition = Maps.newHashMap(); - PaimonPartitionInfo partitionInfo = new PaimonPartitionInfo(nameToPartitionItem, nameToPartition); + CoreOptions options = new CoreOptions(table.options()); + RowType partitionType = table.rowType().project(table.partitionKeys()); + if (partitionType.getFields().stream().anyMatch(field -> + field.type().getTypeRoot() == DataTypeRoot.TIMESTAMP_WITH_LOCAL_TIME_ZONE)) { + // This metadata is cached by table snapshot, but LTZ values are represented as + // session-local civil times in Doris. Caching those bounds would let one session + // reuse pruning metadata produced in another time zone. Keep scan correctness by + // delegating pruning to Paimon until this cache can carry a time-zone-independent + // typed representation. + return PaimonPartitionInfo.UNPRUNABLE; + } + InternalRowPartitionComputer partitionComputer = new InternalRowPartitionComputer( + options.partitionDefaultName(), + partitionType, + table.partitionKeys().toArray(new String[0]), + options.legacyPartitionName()); List<Type> types = partitionColumns.stream() .map(Column::getType) .collect(Collectors.toList()); + List<PaimonPartitionCandidate> candidates = Lists.newArrayListWithExpectedSize(partitionEntries.size()); + Map<String, Map<String, String>> displayNameToTypedSpec = Maps.newHashMap(); + + for (PartitionEntry partitionEntry : partitionEntries) { + Map<String, String> typedSpec = getPartitionInfoMap( Review Comment: [P1] Preserve NTZ partition metadata `getPartitionInfoMap` formats a Paimon `TIMESTAMP_WITHOUT_TIME_ZONE` as ISO local datetime (for example `2026-07-01T12:34:56`), and this loop passes that string into `toListPartitionItem`. Doris maps the field to DATETIMEV2, but `DateLiteral` recognizes its time component only with a space separator, so the `T` form fails and the catch below marks the entire table `UNPRUNABLE`. Unlike LTZ, NTZ is zone-independent and should remain usable for Doris partition metadata and partition-aligned MTMV refresh. Please emit a Doris-compatible value (or avoid the string round trip) and add an NTZ partition-info test that stays `PRUNABLE`. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonWriteTarget.java: ########## @@ -0,0 +1,123 @@ +// 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. + +package org.apache.doris.datasource.paimon; + +import org.apache.doris.catalog.Column; +import org.apache.doris.catalog.Type; +import org.apache.doris.common.AnalysisException; + +import com.google.common.collect.ImmutableList; +import org.apache.paimon.table.FileStoreTable; +import org.apache.paimon.table.Table; +import org.apache.paimon.types.DataField; +import org.apache.paimon.types.DataTypeRoot; + +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; + +/** + * One immutable view of the current Paimon write target. + * + * <p>A time-travel snapshot attached to the Doris table is read-side state and must not affect + * sink analysis. Capturing the latest remote table once also keeps schema binding, writer + * distribution and the serialized JNI table on the same table generation. + */ +public final class PaimonWriteTarget { + private final PaimonExternalTable dorisTable; + private final FileStoreTable table; + private final List<Column> schema; + private final Map<String, Column> columnsByName; + private final Map<String, Type> columnTypes; + private final Set<String> partitionColumnNames; + + private PaimonWriteTarget(PaimonExternalTable dorisTable, FileStoreTable table) { + this.dorisTable = dorisTable; + this.table = table; + PaimonExternalCatalog catalog = (PaimonExternalCatalog) dorisTable.getCatalog(); + + ImmutableList.Builder<Column> schemaBuilder = ImmutableList.builder(); + Map<String, Column> columns = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + Map<String, Type> types = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + for (DataField field : table.rowType().getFields()) { + Type type = PaimonUtil.paimonTypeToDorisType( + field.type(), catalog.getEnableMappingVarbinary(), false); + // Doris exposes external-table columns as nullable. The real Paimon nullability and + // defaults remain in the pinned FileStoreTable and are enforced by the Paimon writer. + Column column = new Column(field.name(), type, true, null, true, + field.description(), true, -1); + PaimonUtil.updatePaimonColumnUniqueId(column, field); + if (field.type().getTypeRoot() == DataTypeRoot.TIMESTAMP_WITH_LOCAL_TIME_ZONE) { + column.setWithTZExtraInfo(); + } + schemaBuilder.add(column); + columns.put(field.name(), column); Review Comment: [P1] Reject case-colliding top-level fields before binding `schemaBuilder` keeps every remote field, but these case-insensitive maps silently replace the first entry for a schema containing distinct names such as `A` and `a`. The implicit INSERT path still binds both positional columns, then `getJdbcColumnToOutput` collapses their source aliases the same way; both sink outputs resolve to the second input even though the writer later resolves the exact names to two different Paimon fields. This silently duplicates data across columns. Please reject case-insensitive top-level collisions here (as `PaimonMetadataOps` already does), or preserve positional/exact-name identity throughout binding, and cover an implicit insert into `A`/`a`. -- 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]
