github-actions[bot] commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4280044039
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -167,9 +271,189 @@ GenericRow tableRow(InternalRow columnValues) {
// from an omitted field and retains Paimon's writer-side
semantics.
row.setField(tableFieldIndexes[i], value);
}
+ if (insertsRow(row)) {
+ padInsertedRow(row);
+ }
return row;
}
+ /**
+ * Whether {@code row} inserts, the only kind of row that 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. A table with a
rowkind.field takes the kind of a row
+ * from that field's value when Paimon writes it ({@code TableWriteImpl}),
so only its '+I' rows insert: a '-D',
+ * '-U' or '+U' row carries a key to match, as an UPDATE or DELETE row
does.
+ */
+ private boolean insertsRow(GenericRow row) {
+ return row.getRowKind() == RowKind.INSERT
+ && (rowKindFieldIndex < 0 ||
INSERT_ROW_KIND.equals(row.getField(rowKindFieldIndex)));
Review Comment:
[P2] Resolve the effective row kind before deciding to pad
For a Parquet table `(c CHAR(3), rk STRING, v INT)` with primary key `c`,
`bucket=1` and `rowkind.field=rk`, `INSERT INTO t VALUES ('k', '+i', 1)` skips
padding here, although Paimon's
[RowKind.fromShortString](https://github.com/apache/paimon/blob/release-1.4.2/paimon-api/src/main/java/org/apache/paimon/types/RowKind.java)
uppercases its input and accepts this as INSERT. A subsequent `INSERT ...
VALUES ('k', '+I', 2)` writes `'k '` and leaves two primary-key rows instead
of replacing the first. Nullable `rk DEFAULT '+I'` with an explicit NULL has
the same failure: the SDK substitutes its default before decoding the row kind.
Determine the effective kind with the SDK's default/case rules, and cover both
paths. These are distinct from the fixed explicit `-D` case: the replacement
predicate now misses valid INSERT events instead of changing deletion keys.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonWriteBinding.java:
##########
@@ -126,10 +156,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 &&
!canonicalName.equals(rowKindField)) {
+ value = padChar(value, ((CharType) type).getLength());
Review Comment:
[P2] Keep +U overwrite rows aligned with the partition filter
For a table `(id INT, p CHAR(3), rk STRING, v INT)` with primary key
`(id,p)`, partition `p`, `bucket=1` and `rowkind.field=rk`, the supported
`INSERT OVERWRITE TABLE t PARTITION(p='x') SELECT 1, '+U', 9` pads the FE
overwrite filter to `'x '` here. The new `PaimonWriteSchema.insertsRow`
predicate skips all row padding for `+U`, leaving the materialized partition
value `'x'`; the SDK then rejects the appended file as outside the overwrite
partition. This worked with the previous unpadded path. Use a consistent static
partition policy for the filter and every accepted event kind, or reject the
combination before writing, and add a `+U` overwrite regression. Unlike the
earlier control-column partition issue, `p` is an ordinary CHAR partition and
`rk` is a separate STRING column.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -167,9 +271,189 @@ GenericRow tableRow(InternalRow columnValues) {
// from an omitted field and retains Paimon's writer-side
semantics.
row.setField(tableFieldIndexes[i], value);
}
+ if (insertsRow(row)) {
+ padInsertedRow(row);
+ }
return row;
}
+ /**
+ * Whether {@code row} inserts, the only kind of row that 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. A table with a
rowkind.field takes the kind of a row
+ * from that field's value when Paimon writes it ({@code TableWriteImpl}),
so only its '+I' rows insert: a '-D',
+ * '-U' or '+U' row carries a key to match, as an UPDATE or DELETE row
does.
+ */
+ private boolean insertsRow(GenericRow row) {
+ return row.getRowKind() == RowKind.INSERT
+ && (rowKindFieldIndex < 0 ||
INSERT_ROW_KIND.equals(row.getField(rowKindFieldIndex)));
+ }
+
+ private void padInsertedRow(GenericRow row) {
+ for (int i = 0; i < charPadders.length; i++) {
+ Object value = charPadders[i] == null ? null :
row.getField(tableFieldIndexes[i]);
+ 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]));
+ }
+ row.setField(tableFieldIndexes[i], charPadders[i].pad(value));
+ }
+ }
+
+ /**
+ * Returns the padder of a value of {@code type} in {@code column}, 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. A map whose keys padding would make equal fails the row;
see {@link #padMapKeys}.
+ */
+ private static CharPadder charPadder(DataType type, String column) {
+ 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, column);
+ 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(),
column);
+ CharPadder valuePadder = charPadder(mapType.getValueType(),
column);
+ 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() :
padMapKeys(map.keyArray(), keyGetter, keyPadder,
+ column),
+ 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(), column);
+ 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);
+ }
+ return new GenericArray(elements);
+ }
+
+ /**
+ * Pads the keys of one map, failing when padding makes two of its
distinct keys one: a map holds one entry per
+ * key, and Paimon's Avro reader, for one, keeps only one of them. Keys
equal before padding are kept as they are.
+ */
+ private static InternalArray padMapKeys(InternalArray keys,
InternalArray.ElementGetter keyGetter,
+ CharPadder keyPadder, String column) {
+ Object[] padded = new Object[keys.size()];
+ Map<Object, Object> keyOfPadded = new HashMap<>();
+ for (int i = 0; i < padded.length; i++) {
+ Object key = keyGetter.getElementOrNull(keys, i);
+ padded[i] = key == null ? null : keyPadder.pad(key);
+ if (!keyOfPadded.containsKey(padded[i])) {
+ keyOfPadded.put(padded[i], key);
Review Comment:
[P2] Avoid object hashing of Arrow-backed nested map keys
Copying an existing Parquet Paimon column `MAP<ROW<c CHAR(2), r ROW<i INT>>,
INT>` into another Paimon table (`force_jni_scanner=true; INSERT INTO dst
SELECT m FROM src`) reaches this check with an Arrow-backed key. The ROW padder
creates an outer `GenericRow` for `c` but retains the unchanged nested `r` as
`ColumnarRow`. This HashMap invokes `GenericRow.hashCode` and then
[ColumnarRow.hashCode](https://github.com/apache/paimon/blob/release-1.4.2/paimon-common/src/main/java/org/apache/paimon/data/columnar/ColumnarRow.java),
which unconditionally throws `UnsupportedOperationException`, even for a
single map entry. The external-schema mapper, JNI reader and Arrow serializer
all recursively support this scan/copy path; Doris DDL's scalar-key validation
is not involved. Use type-aware key hashing/comparison or fully materialize
descendants, and cover a real Arrow-backed nested ROW key. This differs from
the previous scalar collision issue: no duplicate keys are required.
--
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]
