github-actions[bot] commented on code in PR #69571:
URL: https://github.com/apache/doris/pull/69571#discussion_r4366822772
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -2292,6 +2299,63 @@ static FileStoreTable legacyOrcSplitTable(FileStoreTable
table, DataSplit split)
return base;
}
+ /** Positions in the partition row of the partition columns that hold
binary values. */
+ private static int[] binaryPartitionFields(Table table) {
+ RowType partitionType = table.rowType().project(table.partitionKeys());
+ return java.util.stream.IntStream.range(0,
partitionType.getFieldCount()).filter(field -> {
+ DataTypeRoot typeRoot =
partitionType.getTypeAt(field).getTypeRoot();
+ return typeRoot == DataTypeRoot.BINARY || typeRoot ==
DataTypeRoot.VARBINARY;
+ }).toArray();
+ }
+
+ /**
+ * Paimon finds the files of a partition under the directory its values
name. With {@code partition.legacy-name}
+ * = true, the default, it names a partition after the string of each
value, which for a non-NULL binary value
+ * is the Java array object ({@code [B@1b6d3586}) rather than its bytes,
and every resolution of the name
+ * differs. A file written under such a name, or looked up under one, is
never found: Paimon's own read fails,
+ * while the BE skips a missing file of an external table and would
silently drop the partition's rows. A file
+ * with an external path is found by the path its metadata records.
Without a schema dictionary (system tables
+ * other than {@code $ro}, which the JNI reader reads with Paimon's own
reader) only the lookup is checked.
+ */
+ private void checkBinaryPartitionFiles(PaimonTableHandle handle, Table
table, DataSplit split,
+ int[] binaryPartitionFields, Supplier<Table> schemaTable,
+ Map<FileStoreTable, Map<Long, Boolean>>
legacyPartitionNameSchemas) {
+ BinaryRow partition = split.partition();
+ OptionalInt nonNullField = Arrays.stream(binaryPartitionFields)
+ .filter(field -> !partition.isNullAt(field)).findFirst();
+ if (!nonNullField.isPresent()) {
+ return;
+ }
+ int binaryField = nonNullField.getAsInt();
+ FileStoreTable splitTable = schemaTable.get() == null
+ ? null : legacyOrcSplitTable((FileStoreTable)
schemaTable.get(), split);
+ boolean legacyLookup = CoreOptions.fromMap((splitTable == null ? table
: splitTable).options())
+ .legacyPartitionName();
+ for (DataFileMeta file : split.dataFiles()) {
Review Comment:
[P2] Validate deletion-vector paths before exempting external data
An absolute `externalPath()` only makes the data file independent of
partition naming. Create a binary-partitioned append table with `bucket=-1`,
`deletion-vectors.enabled=true`, `index-file-in-data-file-dir=true`, default
legacy names and external data paths. Insert two rows in one file, unset
`data-file.external-paths` and its strategy, then delete only one row through
Spark. Paimon's
[`persistDeletionVectors`](https://github.com/apache/paimon/blob/release-2.1.0/paimon-spark/paimon-spark-common/src/main/scala/org/apache/paimon/spark/commands/PaimonSparkWriter.scala)
commits only index changes using the current local path factory: the data
keeps its recorded external path, but the new DV has a relative name under an
identity-based binary directory. This `continue` exempts the data without
checking its DV. The native reader's missing-DV `NOT_FOUND` reaches
`FileScanner`, which skips the whole data range under the default
`ignore_not_found_file_in_external_table=true`, silently los
ing the surviving row. This leaves the PR's targeted failure unfixed for that
supported layout. Include required deletion files in the naming validation and
cover this external-data/local-DV transition.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -2292,6 +2299,63 @@ static FileStoreTable legacyOrcSplitTable(FileStoreTable
table, DataSplit split)
return base;
}
+ /** Positions in the partition row of the partition columns that hold
binary values. */
+ private static int[] binaryPartitionFields(Table table) {
+ RowType partitionType = table.rowType().project(table.partitionKeys());
+ return java.util.stream.IntStream.range(0,
partitionType.getFieldCount()).filter(field -> {
+ DataTypeRoot typeRoot =
partitionType.getTypeAt(field).getTypeRoot();
+ return typeRoot == DataTypeRoot.BINARY || typeRoot ==
DataTypeRoot.VARBINARY;
+ }).toArray();
+ }
+
+ /**
+ * Paimon finds the files of a partition under the directory its values
name. With {@code partition.legacy-name}
+ * = true, the default, it names a partition after the string of each
value, which for a non-NULL binary value
+ * is the Java array object ({@code [B@1b6d3586}) rather than its bytes,
and every resolution of the name
+ * differs. A file written under such a name, or looked up under one, is
never found: Paimon's own read fails,
+ * while the BE skips a missing file of an external table and would
silently drop the partition's rows. A file
+ * with an external path is found by the path its metadata records.
Without a schema dictionary (system tables
+ * other than {@code $ro}, which the JNI reader reads with Paimon's own
reader) only the lookup is checked.
+ */
+ private void checkBinaryPartitionFiles(PaimonTableHandle handle, Table
table, DataSplit split,
+ int[] binaryPartitionFields, Supplier<Table> schemaTable,
+ Map<FileStoreTable, Map<Long, Boolean>>
legacyPartitionNameSchemas) {
+ BinaryRow partition = split.partition();
+ OptionalInt nonNullField = Arrays.stream(binaryPartitionFields)
+ .filter(field -> !partition.isNullAt(field)).findFirst();
+ if (!nonNullField.isPresent()) {
+ return;
+ }
+ int binaryField = nonNullField.getAsInt();
+ FileStoreTable splitTable = schemaTable.get() == null
+ ? null : legacyOrcSplitTable((FileStoreTable)
schemaTable.get(), split);
+ boolean legacyLookup = CoreOptions.fromMap((splitTable == null ? table
: splitTable).options())
+ .legacyPartitionName();
+ for (DataFileMeta file : split.dataFiles()) {
+ if (file.externalPath().isPresent()) {
+ continue;
+ }
+ if (legacyLookup || (splitTable != null &&
legacyPartitionNameSchemas
+ .computeIfAbsent(splitTable, ignored -> new HashMap<>())
+ .computeIfAbsent(file.schemaId(), id ->
withSchemaHistoryAuthentication(() -> CoreOptions
Review Comment:
[P2] Do not infer writer naming options from the file schema ID
Paimon's `FileStoreTable.copy(...)` can override `partition.legacy-name`
without persisting a new schema;
[`TableSchema.copy`](https://github.com/apache/paimon/blob/release-2.1.0/paimon-api/src/main/java/org/apache/paimon/schema/TableSchema.java)
retains the ID, which the writer stores in `DataFileMeta`. For example, create
a binary-partitioned append table with the default `true`, write via
`table.copy(Collections.singletonMap("partition.legacy-name", "false"))`, then
ALTER the persisted option to `false`. Those files have valid bytes-based paths
and are readable with the current options, but still reference the original
schema whose persisted option is `true`, so this branch now rejects every scan
that reads them. The inverse dynamic override also shows that this schema
boolean does not prove how a file was named. Check actual path/write provenance
before declaring historical files unreadable, and add a test that writes with a
dynamic naming option; the added ALTER-only tests do
not cover it.
--
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]
