morningman commented on code in PR #69295:
URL: https://github.com/apache/doris/pull/69295#discussion_r4316839684
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -210,8 +208,7 @@ static PaimonWriteSchema create(RowType tableType, String[]
columnNames, boolean
// Paimon writer return its normal constraint diagnostic instead
of reading a default
// primitive value from a non-null getter.
inputFields.add(field.newType(field.type().nullable()));
- if (charPadding && !field.name().equals(rowKindField)
- && (!orcFiles || partitionKeys.contains(field.name()))) {
+ if (charPadding && !field.name().equals(rowKindField)) {
Review Comment:
Fixed in 0037d011cdb. This was a regression of this PR, and it reproduced on
a local cluster: on an ORC table partitioned by `p`, with `c CHAR(3)` as its
primary key and `bucket = -1`, `INSERT ('k', 1, 'p1')` and then, in a second
statement, `INSERT ('k', 2, 'p2')` left two rows of `k`, while master's writer
(#69083), which wrote `'k'` unpadded on ORC tables, kept one.
The index here is Doris's own `GlobalIndexAssigner`, filled through Paimon's
`IndexBootstrap`, whose rows hold the primary keys that are not partition keys
as the data files give them back, then the partition and the bucket.
`PaimonWriteSchema.bootstrapKeyPadder` now pads those keys on a table that
writes ORC files, at some levels or at all, with the same padders as the rows
the writer writes, so the index holds the keys as they are written. The
partition values come from the partitions as they are stored and are kept, as
is the bucket; a table without ORC files, or a write with padding off, gets its
bootstrap rows unchanged.
Tests:
-
`PaimonWriteSchemaTest.testBootstrappedIndexOfAnOrcTableFindsTheKeyThatTheWriterPads`
writes `'k '` into `p1` of a real ORC table through the Paimon API,
bootstraps the assigner from it, and writes `'k'` into `p2`: the stored row is
deleted from `p1` (`-D` into its bucket) and the new one goes to `p2`. Without
the padding the delete is missing.
- `testBootstrapKeysOfAnOrcTableArePaddedAsTheWrittenKeys` checks that only
the keys are padded, not the partition.
- Regression `orc_cross_partition_upsert`: `'k'` written by Doris into `p1`
and then `p2`, and `'m'` written by Spark into `p1` and then by Doris into
`p2`, each keep one row in `p2`. With the previous commit's jar both keys kept
two rows. With master's jar the key Spark wrote did too, because master wrote
back `'m'` unpadded.
Paimon's own writer for Spark and Flink bootstraps the same trimmed keys:
Spark writing `'k'` into `p1` and then `p2` of such an ORC table keeps two rows
(Parquet keeps one). That one is a Paimon issue.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -327,24 +334,49 @@ private void padRow(GenericRow row) {
if (value == null) {
continue;
}
- // In a changelog write only MERGE inserts rows, and MERGE has
matched them to the table and checked them
- // for duplicate keys by the key as it is: Doris compares CHAR
values unpadded. Padding a shorter key
- // here would make 'k' and 'k ' one key, or let a NOT MATCHED row
replace the stored 'k ' it did not
- // match. A key as long as the column is not changed by padding.
- if (insertedKeyCharLengths[i] > 0 && ((BinaryString)
value).numChars() < insertedKeyCharLengths[i]) {
- throw new IllegalArgumentException(String.format(
- "MERGE cannot insert '%s' into the CHAR(%d) primary
key column '%s' of a Paimon table: "
- + "the value is shorter than the column, and
padding it to the column "
- + "length would change the key that MERGE
matched. Pad the key in the "
- + "MERGE, with rpad(<key>, %d, ' ') in both
the ON condition and the "
- + "inserted values, or set
enable_paimon_write_char_padding = false",
- value, insertedKeyCharLengths[i],
inputType.getFields().get(i).name(),
- insertedKeyCharLengths[i]));
+ if (insertedKeyCharLengths[i] > 0 && row.getRowKind() ==
RowKind.INSERT) {
+ checkInsertedKey((BinaryString) value, i);
}
row.setField(tableFieldIndexes[i], charPadders[i].pad(value));
}
}
+ /**
+ * In a changelog write only MERGE inserts rows, and MERGE has matched
them to the table and checked them for
+ * duplicate keys by the key as it is: Doris compares CHAR values
unpadded. A table that writes no ORC files gives
+ * Doris its keys back padded, so padding a shorter key here would make
'k' and 'k ' one key, or let a NOT MATCHED
+ * row replace the stored 'k ' it did not match; a key as long as the
column is not changed by padding. ORC gives
+ * the keys back without their trailing spaces, so there MERGE matched 'k'
to the stored 'k ', and only a key
+ * that ends with a space, such as 'k ', matches no stored key yet pads to
one.
+ */
+ private void checkInsertedKey(BinaryString key, int column) {
+ int length = insertedKeyCharLengths[column];
+ String name = inputType.getFields().get(column).name();
+ if (orcFiles) {
Review Comment:
Fixed in 0037d011cdb. Both cases reproduced on a local cluster:
- `file.format = 'parquet'` with `file.format.per.level = '5:orc'`: a MERGE
with source key `'k'` missed the stored `'k '` and replaced it (`v` became the
inserted value).
- ORC table with a CHAR partition column `p` in the primary key: a MERGE
whose source `p` was `'x'` missed the stored `'x '` and replaced that row.
Master (#69083) handled both, because it did not pad the non-partition keys
of such tables and kept the short-key check for partition keys. So both were
regressions of this PR.
The check now depends on how Doris reads the stored values of each CHAR
primary-key column back (`KeyReadBack`):
| Column | Read back | A MERGE insert fails when it |
|---|---|---|
| partition column, any format; or any column of a table without ORC files |
padded, as stored | is shorter than the column (the rule of #69083) |
| non-partition column of a table where every level writes ORC | without
trailing spaces | ends with a space |
| non-partition column of a table that writes ORC at some levels only |
either way, depending on the file | is shorter than the column, or ends with a
space |
On a mixed table only keys as long as the column that do not end with a
space are safe; the message says so and names
`enable_paimon_write_char_padding`. `PaimonWriteSchema.orcFiles` now reads the
format of each level from 0 to `num-levels - 1` as Paimon's writer picks it
(`file.format.per.level`, or else `file.format`), and returns NONE / SOME / ALL.
Tests:
- `testMergeIntoAnOrcTableChecksItsPartitionKeysAsTheyAreStored`,
`testMergeIntoATableWithOrcFilesAtSomeLevelsFailsForShortKeysAndKeysEndingWithASpace`
and `testOrcFilesAreReadFromTheTableFormatOptions`. Each turns red under one
mutation: checking a partition key as an ORC key, treating a mixed table as
all-ORC or as non-ORC, or ignoring the per-level formats or the number of
levels.
- Regression `orc_partition_key_merge`: the MERGE with an unpadded partition
value fails, and one with `rpad` updates the row.
- Regression `avro_level0_*`: an `'orc'` + `'0:avro'` table where a
compacted `'q'` reads back as `'q'` and a level-0 `'k'` as `'k '`. MERGE
inserting `'k'` or `'q '` fails, and `'abc'` is inserted. On master, the MERGE
inserting `'k'` added a second `k` row next to the one Spark wrote.
A related limit in such mixed tables belongs to Paimon itself: once a CHAR
key has been compacted into an ORC file, a newer version of it in a non-ORC
level does not merge with it. Paimon's merge reader compares the keys as each
format returns them, `'q '` from Avro and `'q'` from ORC. Spark alone shows
it: upserting `'q'` after the compaction reads back two rows of `q`. So the
regression test does not update a key in an ORC level of that table.
--
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]
