morningman commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4279897764
##########
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:
Fixed in `f2c45d7e38c`. This reproduces, and for most short keys rather than
one pair: Paimon stores CHAR(n) as ORC `char(n)`, and ORC pads every CHAR value
it writes (`CharTreeWriter`) and strips the trailing spaces from one it reads
(`CharTreeReader`), as Doris's ORC reader does too. Padding in the writer
therefore changed nothing that ORC stores, only the bucket a CHAR key is hashed
to, and UPDATE and DELETE, which read the key back without its spaces, hashed
another one. On a local cluster, in an ORC table `(c CHAR(3), v INT)` keyed by
`c` with `bucket = 4`, `DELETE ... WHERE v IN (1, 3, 4, 5)` of the keys `'k'`,
`'x'`, `'m'` and `'zz'` that Doris had just inserted returned COMMITTED and
removed none of them. With padding off, and in a Parquet table with padding on,
it removed all four.
The writer now pads only the partition columns of a table that writes ORC
files, by `file.format` or at some level of `file.format.per.level`
(`PaimonWriteSchema.writesOrcFiles`). A partition value is read back as it is
stored, so padding it still makes Doris and Spark write one partition, and
UPDATE and DELETE of a CHAR-partitioned ORC table were already correct. The ORC
files hold the same bytes as before, and a key Doris writes into such a table
gets the bucket it had before this PR, so UPDATE, DELETE and MERGE reach it, as
they reach the keys an earlier Doris wrote; MERGE's short-key check does not
apply to a key that is not padded. While padding is on, FE still writes a
fixed-bucket table with a CHAR key through one writer, since it does not look
at the file format; that only costs parallelism. One case stays as it is on
master: a short CHAR key that Spark or Flink wrote padded into a multi-bucket
ORC table sits in another bucket than the one Doris's UPDATE and DELETE hash fr
om the key they read back, so they still miss it. Reaching it needs the bucket
of the row Doris read, which is a change of its own.
Tests: `PaimonWriteSchemaTest.testOrcTablePadsOnlyItsPartitionValues` (an
ORC table pads its CHAR partition value but not its CHAR key or map keys, and a
MERGE that inserts a short partition value still fails) and
`testOrcFilesAreReadFromTheTableFormatOptions`; padding every column of an ORC
table, or ignoring `file.format.per.level`, turns them red.
`test_paimon_write_char_padding` now deletes four and updates two of six keys
that Doris inserted into a CHAR(3)-keyed ORC table with `bucket = 4`.
##########
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:
Fixed in `f2c45d7e38c`. This reproduces: with a key `'k'` written with
padding off and `'k '` written with it on, `INSERT INTO t SELECT c, '-D', v
FROM t WHERE c = 'k'` deleted `'k '` and left `'k'`, and the same `-D` for a
key `'j'` written unpadded, with no padded twin, deleted nothing. Doris refuses
UPDATE, DELETE and MERGE on a table with a `rowkind.field`, so such an INSERT
is how Doris deletes rows of it, with keys read back from the table.
The writer now goes by the kind Paimon will give the row: a row of such a
table is padded only when its `rowkind.field` value is `'+I'`, whether Doris
writes that value or the field's default supplies it. A `'-D'`, `'-U'` or
`'+U'` row carries a key to match, as Doris's own UPDATE and DELETE rows do,
and is written as Doris holds it, so both `-D` rows above now delete the row
they were read from. A short key written as a literal in such a row is not
padded either, and so does not reach a key that Spark or Flink wrote padded, as
on master; it has to be written full length.
Tests:
`PaimonWriteSchemaTest.testRowKindFieldValueDecidesWhetherARowIsPadded` (`'+I'`
rows are padded; `'-D'`, `'-U'`, `'+U'` and NULL rows are not; a default `'+I'`
or `'-D'` decides the same way); padding by the row's own kind alone, or
looking the value up in the input only, turns it red.
`test_paimon_write_char_padding` deletes an unpadded and a padded key of a
CHAR-keyed `rowkind.field` table with `-D` rows read back from it.
##########
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:
Fixed in `f2c45d7e38c`. This reproduces: `map('k', 1, 'k ', 2)` written into
a `MAP<CHAR(2), INT>` column read back as `{"k ":2}` from an Avro table, and as
a map holding the key `'k '` twice from a Parquet one. A map holds one entry
per key, and padding is what made these two keys one, so the writer now fails
such a row (`PaimonWriteSchema.padMapKeys`). The message names the column and
the two keys, and asks for distinct keys or for padding to be turned off. Keys
that were equal before padding are kept as they are, as before, and keys that
stay distinct are padded. Spark, with the paimon-spark extensions, rejects the
same insert with `[DUPLICATED_MAP_KEY] Duplicate map key k was found`. An ORC
table, which is no longer padded (see the ORC thread), gets both keys as
before; ORC pads them itself, as on master.
Tests: `PaimonWriteSchemaTest.testMapKeysThatPaddingMakesEqualFailTheRow`;
removing the check, or failing keys that were equal before padding too, turns
it red. `test_paimon_write_char_padding` writes such a map into an Avro table,
which fails with padding on and keeps both keys with it off, and a map of
distinct keys, which is padded. With the jars from before this commit the suite
fails at that check, its first new one.
--
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]
