morningman commented on code in PR #69571:
URL: https://github.com/apache/doris/pull/69571#discussion_r4367353301
##########
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:
Confirmed, and fixed in `65a1682d334`. With `index-file-in-data-file-dir =
true`, a deletion vector records an absolute path only if an external data path
is configured when it is written (`DeletionFileWriter.result`). Otherwise
Paimon resolves it under the partition's directory each time a split is planned
(`IndexInDataFileDirPathFactory.toPath`). Under legacy names that directory is
the identity string of a fresh array. So once the external paths are removed, a
deletion vector written by a DELETE is never found, while its data file keeps
its absolute path. Both native readers then get NOT_FOUND when they open the
deletion vector (V1 `_init_deletion_vector`, V2
`TableReader::_parse_delete_predicates`), and `FileScanner` / `FileScannerV2`
skip the whole data range, surviving rows included. Paimon's own reader, JNI
and Rust fail instead.
For a split with a non-NULL binary partition value on a table that keeps
index files in data directories, every deletion vector the split carries must
now exist when the split is planned, whether or not its data file has an
external path. A missing one fails planning, naming the deletion vector file,
its data file, the partition and the option. The option is immutable, so the
reading table's value is the one every writer used. Deletion vectors under the
table's `index/` directory do not depend on partition names and are not
checked. The existence check runs inside the catalog authentication, once per
deletion vector file and scan.
`PaimonScanPlanProviderTest#externalBinaryPartitionFileWithLocalDeletionVectorFailsScanPlanning`
replays your sequence through the Paimon API: two rows written to one file
with external paths, the paths removed, then one row deleted by persisting a
deletion vector the way `PaimonSparkWriter.persistDeletionVectors` does it.
Without the new check that spli
t plans. `#externalBinaryPartitionDeletionVectorAtExternalPathStaysReadable`
keeps the external paths, so the deletion vector records its absolute path and
the split still plans.
master before this PR and branch-4.2 drop these rows too, since neither
checks binary partitions at all. A BE-side change that makes a missing Paimon
deletion vector an error instead of a skipped range, which would also cover
deletion vectors lost in other ways, is tracked separately.
##########
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:
Confirmed. `FileStoreTable.copy` keeps the schema ID (`TableSchema.copy`),
so a file written with a dynamic `partition.legacy-name = false` (`SET
spark.paimon.partition.legacy-name = false`, a Flink hint, or `table.copy`)
refers to a schema whose persisted option is still `true`. Nothing in the
metadata records the directory a file was written to: `DataFileMeta` has only
the file name and an optional external path, and dynamic options are not
persisted. The schema's option is therefore only a hint. Failing on it alone
rejected files that are readable once the table is altered to `false`, and
master reads them.
Fixed in `65a1682d334`. The file's schema still selects which files to look
at, but planning now fails only if such a file is also missing at its planned
path (`bucketPath/fileName`, the path the native reader is given). In a table
whose option was switched after legacy writes, the file is missing and the
error stays. In your sequence the file is there and the scan plans. This costs
one existence call per flagged file, and none for tables whose files were
written under the current naming.
`PaimonScanPlanProviderTest#binaryPartitionFilesWrittenWithDynamicNamesStayReadable`
writes through `table.copy({"partition.legacy-name": "false"})` on a table
that keeps the default: planning fails as a legacy lookup until the option is
altered to `false`, and then plans the file under the bytes of its value.
Without the existence check it fails with the error you describe.
The inverse case is not covered: a file written with a dynamic
`partition.legacy-name = true` while the schema says `false` is still looked up
under its bytes, and the BE skips it. Catching that would take an existence
call for every file of every binary partition on every scan. master and
branch-4.2 behave the same.
--
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]
