github-actions[bot] commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4276034934
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -163,13 +237,149 @@ GenericRow tableRow(InternalRow columnValues) {
row.setRowKind(toRowKind(value));
continue;
}
+ // Only an inserted row is padded; its row kind, if any, is the
first input column. An UPDATE or
+ // DELETE row carries the key Doris read back from the table,
which has to stay byte-equal to the
+ // stored key for Paimon to match it, padded or not.
+ if (charPadders[i] != null && value != null && row.getRowKind() ==
RowKind.INSERT) {
+ // 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]));
+ }
+ value = charPadders[i].pad(value);
+ }
// Actual Doris input is applied last so an explicit NULL remains
distinct
// from an omitted field and retains Paimon's writer-side
semantics.
row.setField(tableFieldIndexes[i], value);
}
return row;
}
+ /**
+ * Returns the padder of a value of {@code type}, or null when the type
holds no CHAR. A CHAR value is padded
+ * with spaces to the type length in code points, as Paimon's cast to CHAR
pads it
+ * ({@code BinaryStringUtils.toCharacterString}), and so are the CHAR
values nested in an ARRAY, a MAP or a
+ * ROW. Unlike that cast, a longer value is kept: Doris has already cut it
to the length or kept it, as the
+ * session asks.
+ */
+ private static CharPadder charPadder(DataType type) {
+ switch (type.getTypeRoot()) {
+ case CHAR: {
+ int length = ((CharType) type).getLength();
+ return value -> padChar((BinaryString) value, length);
+ }
+ case ARRAY: {
+ DataType elementType = ((ArrayType) type).getElementType();
+ CharPadder elementPadder = charPadder(elementType);
+ if (elementPadder == null) {
+ return null;
+ }
+ InternalArray.ElementGetter elementGetter =
InternalArray.createElementGetter(elementType.nullable());
+ return value -> padArray((InternalArray) value, elementGetter,
elementPadder);
+ }
+ case MAP: {
+ MapType mapType = (MapType) type;
+ CharPadder keyPadder = charPadder(mapType.getKeyType());
+ CharPadder valuePadder = charPadder(mapType.getValueType());
+ if (keyPadder == null && valuePadder == null) {
+ return null;
+ }
+ InternalArray.ElementGetter keyGetter =
+
InternalArray.createElementGetter(mapType.getKeyType().nullable());
+ InternalArray.ElementGetter valueGetter =
+
InternalArray.createElementGetter(mapType.getValueType().nullable());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ return new PaddedMap(
+ keyPadder == null ? map.keyArray() :
padArray(map.keyArray(), keyGetter, keyPadder),
Review Comment:
[P2] Reject padded map-key collisions that Avro cannot preserve
Retaining both padded key-array entries does not preserve them through the
supported Avro format. For a table with `m MAP<CHAR(2), INT>` and
`file.format=avro`, inserting `map('k',1,'k ',2)` turns the two distinct keys
into two identical `'k '` keys here. Paimon's Avro writer emits both entries,
but its [generic
MapReader](https://github.com/apache/paimon/blob/release-1.4.2/paimon-format/src/main/java/org/apache/paimon/format/avro/FieldReaderFactory.java)
reconstructs a `HashMap` with `put`; INT values select that reader, so
subsequent reads return one entry and silently discard an original value.
Before this padding change both distinct keys survive. The new unit test checks
duplicate preservation only in `tableRow`, before format
serialization/readback. Reject normalized key collisions when the format cannot
preserve them, or use an entry-preserving representation, and add an Avro
CHAR-map round-trip regression.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -163,13 +237,149 @@ GenericRow tableRow(InternalRow columnValues) {
row.setRowKind(toRowKind(value));
continue;
}
+ // Only an inserted row is padded; its row kind, if any, is the
first input column. An UPDATE or
+ // DELETE row carries the key Doris read back from the table,
which has to stay byte-equal to the
+ // stored key for Paimon to match it, padded or not.
+ if (charPadders[i] != null && value != null && row.getRowKind() ==
RowKind.INSERT) {
+ // 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]));
+ }
+ value = charPadders[i].pad(value);
Review Comment:
[P1] Preserve ORC CHAR key identity for UPDATE and DELETE
Padding inserted keys breaks subsequent row-level DML on ORC tables because
both native and SDK ORC readers strip CHAR trailing spaces. For `(c CHAR(3), v
INT)` with `primary-key=c`, `bucket=4` and `file.format=orc`, `INSERT INTO t
VALUES ('k',1)` now supplies `'k '` to [Paimon's bucket
extractor](https://github.com/apache/paimon/blob/release-1.4.2/paimon-core/src/main/java/org/apache/paimon/table/sink/FixedBucketRowKeyExtractor.java),
putting the row in bucket 1. `DELETE FROM t WHERE v=1` reads back `'k'`; this
method leaves DELETE keys unpadded, so the SDK writes its tombstone to bucket 2
and the selected row survives although DELETE commits. UPDATE similarly writes
another bucket. Previously Doris's short INSERT and its mutations used the same
unpadded key/bucket. GATHER consolidates writers but cannot reconcile these SDK
buckets. Preserve stored key/bucket identity through the ORC mutation read
path, or gate this combination until supported; cover ORC INSERT followed by
UPDAT
E/DELETE with multiple buckets.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -163,13 +237,149 @@ GenericRow tableRow(InternalRow columnValues) {
row.setRowKind(toRowKind(value));
continue;
}
+ // Only an inserted row is padded; its row kind, if any, is the
first input column. An UPDATE or
+ // DELETE row carries the key Doris read back from the table,
which has to stay byte-equal to the
+ // stored key for Paimon to match it, padded or not.
+ if (charPadders[i] != null && value != null && row.getRowKind() ==
RowKind.INSERT) {
Review Comment:
[P1] Resolve rowkind.field before deciding whether to pad keys
An ordinary INSERT starts with `GenericRow`'s INSERT kind, so this branch
pads CHAR data keys even when `rowkind.field` carries `-D`, `-U` or `+U`. The
SDK resolves that operation later in
[TableWriteImpl.writeAndReturn](https://github.com/apache/paimon/blob/release-1.4.2/paimon-core/src/main/java/org/apache/paimon/table/sink/TableWriteImpl.java),
using the already-padded key. For a Parquet table `(c CHAR(3), rk VARCHAR(2),
v INT)` keyed by `c`, with `rowkind.field=rk` and `bucket=1`, seed
`('k','+I',1)` with padding off, then `('k','+I',2)` with padding on. Still
padding on, `INSERT INTO t SELECT c, '-D', v FROM t WHERE c = 'k'` deletes the
unrelated stored `'k '` row and leaves the selected legacy `'k'` row. Resolve
the effective control-field operation before padding and preserve non-insert
keys as for changelog rows; add a CHAR-key control-event regression. This is
distinct from the existing token-padding thread: the token is preserved here,
but its delete key is changed.
--
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]
