github-actions[bot] commented on code in PR #69295:
URL: https://github.com/apache/doris/pull/69295#discussion_r4315766354
##########
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:
[P1] Normalize ORC keys in the global-index bootstrap too
Padding incoming ORC CHAR keys also changes the keys passed to
`GlobalIndexAssigner.processInput`, but
`PaimonJniWriter.bootstrapGlobalIndexKey` still indexes the unpadded keys
returned by Paimon's
[IndexBootstrap](https://github.com/apache/paimon/blob/release-1.4.2/paimon-core/src/main/java/org/apache/paimon/crosspartition/IndexBootstrap.java#L66-L115).
On a fresh ORC table partitioned by `pt`, with `c CHAR(3)` as the primary key
and `bucket=-1` (KEY_DYNAMIC), execute two statements inserting `(pt='p1',
c='k')` and then `(pt='p2', c='k')`. The second writer bootstraps `'k'`, looks
up the newly padded `'k '`, and takes `processNewRecord` instead of
`ExistingProcessor`, leaving the old row in `p1` and inserting another in `p2`.
This affects rows written by this version too. Normalize the bootstrap keys
consistently with incoming keys and cover a cross-partition CHAR upsert in a
second writer session.
##########
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:
[P1] Preserve the MERGE guard for keys read back with padding
`orcFiles` is true when *any* level uses ORC; it does not mean every target
key was read from ORC. For example, with `file.format=parquet` and
`file.format.per.level='5:orc'`, an initial INSERT stores `'k '` in level-0
Parquet. A subsequent `MERGE ... ON t.c = s.c WHEN NOT MATCHED THEN INSERT
...`, with source key `'k'`, does not match that padded key. This branch
nevertheless accepts `'k'`, pads it to `'k '`, and silently replaces the
existing row even though no MATCHED action selected it. CHAR partition keys
have the same problem because scans restore their padded values from partition
metadata, including on ORC tables. Retain the short-key rejection for these
cases, or make MERGE matching and writing use the same canonical key, and add
mixed-level/CHAR-partition regression coverage.
--
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]
