morningman commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4275700626
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -163,13 +214,135 @@ 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) {
+ value = charPadders[i].pad(value);
Review Comment:
Fixed in 45e6e5d2eff. This reproduced on a local cluster. Into an empty
table keyed by `c CHAR(3)`, `MERGE INTO t USING s ON t.c = s.c WHEN NOT MATCHED
THEN INSERT (c, v) VALUES (s.c, s.v)` with the source rows `('k', 1)` and `('k
', 2)` committed and left the single row `'k '`. With a row `'k '` that a
padded INSERT had written, a NOT MATCHED-only MERGE of `('k', 9)` replaced it,
while a MATCHED-only MERGE of the same source changed nothing.
MERGE matches its source rows, and checks the keys it inserts for
duplicates, by the key as Doris holds it, unpadded, and the writer padded the
keys of the rows MERGE inserts afterwards. Normalizing the keys during planning
would mean comparing CHAR keys padded in ON, the read-side change this PR
leaves out: Doris compares CHAR values as they are, as it does for the data
Spark writes. So `PaimonWriteSchema` now fails a row that a MERGE inserts when
one of its CHAR primary-key values is shorter than the column, which is the
only case where padding changes a key, and `PaimonJniWriter` passes it the
table's primary keys. A full-length key is written as it is, so MERGE's
matching and its duplicate-insert check see the stored key. A MERGE can pad the
key itself, `ON t.c = rpad(s.c, 3, ' ') ... INSERT (c, v) VALUES (rpad(s.c, 3,
' '), s.v)`: that matches the padded row, and the duplicate-insert check then
rejects `'m'` and `'m '`. With `enable_paimon_write_char_padding = false`,
MERGE w
rites the keys unpadded, as before.
Tests:
- `PaimonWriteSchemaTest.testMergeInsertOfAShortCharKeyFails` (`'k'`, `'k '`
and `'中中'` into a CHAR(3) key), `testMergeInsertPadsTheRestOfAFullLengthKeyRow`
(a full-length key, `'中中中'` included, and a longer one are written as they are,
and the row's other CHAR column is padded), and
`testShortCharKeysThatMergeDoesNotInsertAreWrittenAsBefore` (UPDATE and DELETE
rows, padding off, and a plain INSERT). Removing the check, counting bytes, or
checking UPDATE and DELETE rows too turns them red.
- `test_paimon_write_char_padding` now runs a MERGE from a Doris table: the
short key fails and nothing is written; the `rpad` MERGE inserts `'k '` and,
run again, matches it; the `rpad` MERGE of `'m'` and `'m '` fails the
duplicate-insert check; and with padding off MERGE writes `'m'` and `'m '` as
two rows. With the jars from before this fix the suite fails at the MERGE that
must fail.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonWriteBinding.java:
##########
@@ -126,10 +153,13 @@ static Map<String, String>
resolveStaticPartition(FileStoreTable table, Connecto
}
String value = Objects.requireNonNull(castValues.get(key),
() -> "missing the cast value of static partition column "
+ key);
- if (table.rowType().getField(canonicalName).type().getTypeRoot()
- == DataTypeRoot.TIMESTAMP_WITH_LOCAL_TIME_ZONE) {
+ DataType type = table.rowType().getField(canonicalName).type();
+ if (type.getTypeRoot() ==
DataTypeRoot.TIMESTAMP_WITH_LOCAL_TIME_ZONE) {
value = toSdkLocalTime(canonicalName, value);
}
+ if (charPadding && type.getTypeRoot() == DataTypeRoot.CHAR) {
+ value = padChar(value, ((CharType) type).getLength());
Review Comment:
Fixed in 45e6e5d2eff. This reproduced too: in a table `(id INT, rk CHAR(3))`
keyed by `(id, rk)`, partitioned by `rk`, with `'rowkind.field' = 'rk'` and two
`'+I'` rows, `INSERT OVERWRITE TABLE t PARTITION (rk = '+I') SELECT 3` failed
with `Trying to overwrite partition {rk=+I }, but the changes in rk=+I/ does
not belong to this partition`, and an overwrite whose source returned no rows
at run time returned OK and left both rows. `resolveStaticPartition` now leaves
the `rowkind.field` value unpadded, as the writer does (both compare the name
case-sensitively), so the first overwrite replaces the partition with the row
of id 3 and the empty one clears it.
A source the planner already proves empty, such as `SELECT 3 LIMIT 0`,
behaves differently on this table, and not because of padding: with padding on
the table is written by one writer, the sink then sits directly on the empty
set, and fe-core skips the write altogether, as it does for any connector table
with such a plan (a dynamic-bucket primary-key table, for one). The regression
therefore uses a source that is empty at run time.
Tests:
`PaimonWriteBindingTest.staticRowKindFieldPartitionIsLeftUnpaddedAsTheRowsAre`
(the `rowkind.field` partition value stays `'+I'` while another CHAR partition
value is padded), and `test_paimon_write_char_padding`, which overwrites the
`'+I'` partition of such a table with one row and then with a source that is
empty at run time.
--
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]
