github-actions[bot] commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4273514664
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -163,13 +201,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) {
Review Comment:
[P2] Preserve CHAR-typed rowkind.field control tokens
Paimon permits `rowkind.field` to name a CHAR column: its
[RowKindGenerator](https://github.com/apache/paimon/blob/release-1.4.2/paimon-core/src/main/java/org/apache/paimon/table/sink/RowKindGenerator.java#L38)
accepts CHARACTER_STRING and parses the exact value with
`RowKind.fromShortString`. For a table with `(id INT, rk CHAR(3))`, primary key
`id` and `rowkind.field=rk`, an ordinary `INSERT ... VALUES (1, '+I')` has
RowKind.INSERT here, so this branch changes '+I' into '+I '. The SDK then
rejects it as an unsupported row-kind token; the same happens to '+U', '-U' and
'-D'. These writes worked before default-on padding. Pass the configured
control-field identity into the write schema and preserve its token while
padding data fields; add a CHAR control-column regression.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniWriter.java:
##########
@@ -183,8 +183,8 @@ public void open(String serializedTable, Map<String,
String> hadoopConfig,
this.bucketMode = table.bucketMode();
CoreOptions coreOptions =
CoreOptions.fromMap(table.options());
- this.writeSchema = PaimonWriteSchema.create(
- table.rowType(), columnNames, changelogWrite);
+ this.writeSchema =
PaimonWriteSchema.create(table.rowType(), columnNames, changelogWrite,
+ PaimonWriteSchema.charPadding(table.options()));
Review Comment:
[P1] Keep CHAR padding consistent with fixed-bucket routing
Enabling padding here changes routing fields after the native exchange has
already hashed their original bytes.
`PaimonWritePlanProvider.getWriteDistribution` still selects
`paimon_fixed_bucket` for CHAR partition/bucket keys before its single-writer
fallback. For an unpartitioned CHAR(3) primary key with `bucket=4` and two
writers, 'k' and 'k ' route to native buckets 2 and 3 (different writers), but
both become 'k ' and the SDK writes bucket 1. With
`changelog-producer=full-compaction` and an existing 'k ' row, both writers
restore and compact that same bucket, producing overlapping higher-level files
that [Paimon
rejects](https://github.com/apache/paimon/blob/release-1.4.2/paimon-core/src/main/java/org/apache/paimon/operation/commit/ConflictDetection.java#L299).
FE can reconcile the already-published APPEND snapshot as success while the
full-compaction changelog is missing. Normalize the exchanged route fields
consistently, or use GATHER when enabled padding can change a CHAR
routing field. The new bucket=1 regression does not expose this split.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -163,13 +201,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);
+ }
// 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),
+ valuePadder == null
+ ? map.valueArray() :
padArray(map.valueArray(), valueGetter, valuePadder));
+ };
+ }
+ case ROW: {
+ List<DataField> fields = ((RowType) type).getFields();
+ CharPadder[] fieldPadders = new CharPadder[fields.size()];
+ InternalRow.FieldGetter[] fieldGetters = new
InternalRow.FieldGetter[fields.size()];
+ boolean holdsChar = false;
+ for (int i = 0; i < fields.size(); i++) {
+ fieldPadders[i] = charPadder(fields.get(i).type());
+ fieldGetters[i] =
InternalRow.createFieldGetter(fields.get(i).type().nullable(), i);
+ holdsChar |= fieldPadders[i] != null;
+ }
+ if (!holdsChar) {
+ return null;
+ }
+ return value -> {
+ InternalRow nested = (InternalRow) value;
+ GenericRow padded = new GenericRow(fieldGetters.length);
+ for (int i = 0; i < fieldGetters.length; i++) {
+ Object field = fieldGetters[i].getFieldOrNull(nested);
+ padded.setField(i, field == null || fieldPadders[i] ==
null
+ ? field : fieldPadders[i].pad(field));
+ }
+ return padded;
+ };
+ }
+ default:
+ return null;
+ }
+ }
+
+ private static BinaryString padChar(BinaryString value, int length) {
+ int padLength = length - value.numChars();
+ return padLength > 0 ? BinaryStringUtils.concat(value,
BinaryString.blankString(padLength)) : value;
+ }
+
+ private static InternalArray padArray(InternalArray array,
InternalArray.ElementGetter elementGetter,
+ CharPadder elementPadder) {
+ Object[] elements = new Object[array.size()];
+ for (int i = 0; i < elements.length; i++) {
+ Object element = elementGetter.getElementOrNull(array, i);
+ elements[i] = element == null ? null : elementPadder.pad(element);
Review Comment:
[P2] Account for expanded nested CHAR values before materializing them
This loop retains all padded values on the Java heap before `writer.write`
can apply the managed-page budget. A supported Paimon `ARRAY<CHAR(65535)>`
containing 4096 copies of 'a' arrives as roughly 20 KiB of Arrow data/offsets
(`CHAR>255` maps to Doris STRING), but the loop retains about 256 MiB of new
byte arrays:
[blankString](https://github.com/apache/paimon/blob/release-1.4.2/paimon-common/src/main/java/org/apache/paimon/data/BinaryString.java#L85)
allocates padding and
[concat](https://github.com/apache/paimon/blob/release-1.4.2/paimon-common/src/main/java/org/apache/paimon/utils/BinaryStringUtils.java#L400)
allocates each result. These allocations bypass both ArrowMemoryPool and
PaimonJniMemoryManager, so even a writer/query with a 64 MiB budget
materializes the full 256 MiB first. Increasing the width/element count or
running concurrent writers can exhaust the shared JVM heap instead of failing
at the query budget. Reserve and charge the expanded size before allocation, or
perform padding in bounded, Doris-tracked storage.
--
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]
