You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "morningman (via GitHub)" <gi...@apache.org> on 2026/10/14 16:14:29 UTC
[PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
morningman opened a new pull request, #69083:
URL: https://github.com/apache/doris/pull/69083
### What problem does this PR solve?
Issue Number: None
Related PR: #69045
Problem Summary:
**In short.** Doris wrote CHAR(n) values into Paimon tables without padding them to n characters, while Spark and Flink pad them, so a primary key that Doris wrote and the same key written by Spark or Flink were two rows. This PR pads the CHAR values of inserted rows, nested values and static partition values included, as Paimon's own cast pads them, behind a new session variable `enable_paimon_write_char_padding` that is on by default. It came up in the review of #69045; branch-4.1 and branch-4.2 write CHAR values unpadded too.
**Background.** Paimon's write path stores a value as it is given: it neither pads nor checks the length of a CHAR value, and it compares primary keys and hashes buckets by bytes. The engines pad before they write: Spark with the paimon-spark extensions (`PaimonAnalysis` → `CharVarcharUtils`, `rpad`) and Flink with an implicit cast (`CharVarCharTrimPadCastRule`), nested CHAR values included. Paimon's own cast to CHAR (`BinaryStringUtils.toCharacterString`, used for schema evolution and default values) pads the same way, counting code points. Doris writes Paimon through the JNI writer (`PaimonJniWriter`, rows built by `PaimonWriteSchema.tableRow`); the FFI writer backend is a stub that FE never selects.
**The problem, and what it cost.** On a local cluster with a file:// Paimon catalog, Parquet tables created through the Paimon API:
| Case | master | with this PR |
|---|---|---|
| Doris inserts `'k'` into a CHAR(3) primary key, then the Paimon API writes `'k '`, which is what Spark and Flink write for `'k'` | 2 rows: `[k]`, `[k ]` | 1 row: `[k ]` |
| Doris inserts `['a']`, `map('k', 'v')` and `named_struct('code', 'a', 'n', 1)` into `ARRAY<CHAR(3)>`, `MAP<CHAR(2), CHAR(3)>` and `ROW<code CHAR(3), n INT>` | `[a]`, `[k] -> [v]`, `[a]` | `[a ]`, `[k ] -> [v ]`, `[a ]` |
| `INSERT OVERWRITE ... PARTITION (p = 'x')` and `INSERT ... VALUES (2, 'y')` into a CHAR(3) partition column | partitions `[x]`, `[y]` | partitions `[x ]`, `[y ]` |
A table that Doris and Spark or Flink both wrote got duplicate primary keys, and rows of one key could land in different partitions or buckets.
**How this PR fixes it.**
- **Rows.** `PaimonWriteSchema` pads every CHAR value of an inserted row with spaces to the column length in code points, as Paimon's cast pads it, and also the CHAR values nested in ARRAY, MAP and ROW. It builds a padder once per column whose type holds a CHAR. Unlike Paimon's cast it never cuts a longer value, because Doris has already decided that: BindSink cuts it when the session truncates strings on insert and keeps it otherwise. A map keeps every entry, as before.
- **UPDATE and DELETE.** Their rows are not padded. They carry the key Doris read back from the table, which has to stay byte-equal to the stored key for Paimon to match it: padding it would leave a row that an earlier Doris wrote unpadded neither updated nor deleted. The values an UPDATE sets are written as before.
- **Static partition values.** `PaimonWriteBinding.resolveStaticPartition` pads the static partition value of a CHAR column the same way. Paimon parses that value without padding it (`TypeUtils.castFromString`), so with only the rows padded the overwrite would name another partition than the rows reach, and the commit would fail.
- **Switch.** The new session variable `enable_paimon_write_char_padding` (default true, forwarded with the statement) turns padding off, for a table whose CHAR keys an earlier Doris wrote unpadded. FE passes it to the BE writer as the option `doris.write.char-padding` on the table it serializes for the writer, the table that already carries the write options FE sets, such as `dynamic-partition-overwrite`; the table the FE committer keeps does not get the option. So the change needs no thrift field and no BE C++ change. A writer that finds no such option, from an FE without this change, does not pad, which matches that FE's unpadded static partition values. BEs are upgraded before FEs, so a new FE meets new BEs only; a BE without this change would ignore the option, and a static overwrite of a CHAR partition from a new FE would then fail its commit rather than write a wrong partition.
Reading is unchanged. Doris reads a padded value with its trailing spaces, so `WHERE c = 'a'` does not match `'a '`, as with the data that Spark and Flink write today; with ORC, orc-core strips the trailing spaces on read.
**Results.**
- **FE UT.**
- `PaimonWriteSchemaTest`: top-level CHAR values padded by code points (`'中'` → `'中 '`), a longer value kept, VARCHAR left alone; CHAR values nested in ARRAY, MAP and ROW, where a map keeps two keys that only padding makes equal; padding off; UPDATE and DELETE rows left alone; reading the option.
- `PaimonWriteBindingTest.staticCharPartitionIsPaddedAsTheRowsAre`, `PaimonBackendBoundTableTest.writeBindingTellsOnlyTheBackendWhetherToPadCharValues` and `PaimonWritePlanProviderTest.charPaddingFollowsTheSessionVariableAndDefaultsToOn`.
- Each of these mutations fails its test: no padding, padding every row kind, no nested padding, ignoring the option, counting bytes or UTF-16 units instead of code points, not setting the option, and ignoring the session variable.
- All 737 tests of fe-connector-paimon and 128 of paimon-scanner pass.
- **End to end.** The table above was produced on a local cluster. In the same run, with the switch off Doris writes `'u'`, `'w'` and the static partition `'z'` unpadded; with it on again, `DELETE ... WHERE v = 3` removes `'u'` and `UPDATE ... SET v = 40 WHERE v = 4` updates `'w'` in place, with no padded duplicate.
- **Regression.** The new `test_paimon_write_char_padding` (external_table_p0/paimon/write) writes through Spark and Doris: a key both write is one row, nested and partition values are padded, a second static overwrite of a CHAR partition replaces the padded partition, the switch off writes values as they are, and DELETE and UPDATE match the unpadded keys. Its `.out` was generated on a local cluster with the iceberg docker environment; with the FE and BE jars from before this change it fails at its first check (`[k] 1 1` instead of `[k ] 3 1`). The other suites under `external_table_p0/paimon/write` pass on the same cluster, except `test_paimon_write_thread_lifecycle`, which reads BE's `/api/be_process_thread_num` and that endpoint needs Linux `/proc`.
### Release note
Doris now pads CHAR(n) values written into Paimon tables with spaces to n characters, as Spark and Flink do, including values nested in ARRAY, MAP and ROW and static partition values; UPDATE and DELETE keep keys as stored. Doris reads a padded value back with its trailing spaces, so `WHERE c = 'a'` does not match it, as with data that Spark and Flink write. A CHAR key that an earlier Doris wrote unpadded is a different key from its padded form; set the session variable `enable_paimon_write_char_padding` to false to keep writing CHAR values unpadded.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- `test_paimon_write_char_padding`, as above.
- [x] Unit Test
- `PaimonWriteSchemaTest`, `PaimonWriteBindingTest`, `PaimonBackendBoundTableTest` and `PaimonWritePlanProviderTest`, as above; all tests of fe-connector-paimon and paimon-scanner pass.
- [x] Manual test (add detailed scripts or steps below)
- Local single-BE cluster with a file:// Paimon catalog: the cases in the table above and the switch-off, DELETE and UPDATE cases, before and after this change.
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
- CHAR values written into Paimon tables are padded to the column length unless `enable_paimon_write_char_padding` is false.
- Does this need documentation?
- [ ] No.
- [x] Yes. <!-- Add document PR link here. eg: https://github.com/apache/doris-website/pull/1214 -->
- The new session variable `enable_paimon_write_char_padding`.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6201723505
# FE Regression Coverage Report
Increment line coverage ` 100.00% (1/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69083_1740cfa4637dea43b1143058633d5ffcc3e9348f_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69083_1740cfa4637dea43b1143058633d5ffcc3e9348f_merge_fe/report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6177324636
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6186201817
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6174301595
<details>
<summary>TPC-H: <b>Total hot run time: 29605 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit f6b658b9ee7ab5479e82110fdb85b63376e8a9ab, data reload: false
------ Round 1 ----------------------------------
============================================
q1 17591 5820 5605 5605
q2 2128 319 255 255
q3 10235 1396 861 861
q4 4674 473 338 338
q5 7435 796 537 537
q6 191 186 155 155
q7 776 796 593 593
q8 9233 1380 1140 1140
q9 5669 4269 4260 4260
q10 6827 1296 1013 1013
q11 433 260 232 232
q12 637 400 288 288
q13 18095 3624 2968 2968
q14 284 270 251 251
q15 q16 759 724 681 681
q17 1209 756 586 586
q18 7079 6243 6177 6177
q19 1106 973 602 602
q20 394 340 225 225
q21 5611 2535 2630 2535
q22 399 361 303 303
Total cold run time: 100765 ms
Total hot run time: 29605 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1 6617 6613 6756 6613
q2 747 631 561 561
q3 5503 5721 5291 5291
q4 2022 2146 1549 1549
q5 5642 5642 5867 5642
q6 248 186 148 148
q7 2270 2009 1794 1794
q8 3142 2724 2801 2724
q9 7967 8037 7807 7807
q10 3651 3586 3231 3231
q11 584 399 365 365
q12 653 684 491 491
q13 3248 3627 3042 3042
q14 289 293 258 258
q15 q16 651 665 613 613
q17 7741 7009 6881 6881
q18 13097 12221 13073 12221
q19 909 786 786 786
q20 2197 2164 1930 1930
q21 5719 4687 4782 4687
q22 505 456 394 394
Total cold run time: 73402 ms
Total hot run time: 67028 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6213322866
<details>
<summary>TPC-DS: <b>Total hot run time: 152471 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit c4acf1b45e17b099fd49d257e19a243e5b46756d, data reload: false
query5 4299 617 446 446
query6 428 201 187 187
query7 4794 439 229 229
query8 318 186 158 158
query9 8775 3975 3946 3946
query10 482 305 261 261
query11 6008 3787 3527 3527
query12 140 90 86 86
query13 1251 421 318 318
query14 6127 4012 3732 3732
query14_1 3512 3504 3514 3504
query15 211 203 185 185
query16 947 432 411 411
query17 871 653 542 542
query18 2419 426 305 305
query19 192 160 133 133
query20 85 85 87 85
query21 209 134 114 114
query22 12875 13032 12915 12915
query23 13466 12864 12461 12461
query23_1 12643 12555 12686 12555
query24 7110 823 471 471
query24_1 485 476 478 476
query25 534 420 365 365
query26 1259 263 141 141
query27 2756 439 270 270
query28 4636 1976 1961 1961
query29 1574 599 467 467
query30 297 226 187 187
query31 1014 929 812 812
query32 151 97 91 91
query33 513 301 250 250
query34 909 879 497 497
query35 742 754 646 646
query36 827 812 772 772
query37 125 93 90 90
query38 1821 1765 1708 1708
query39 789 807 753 753
query39_1 730 729 735 729
query40 216 110 100 100
query41 65 62 62 62
query42 82 79 80 79
query43 362 385 328 328
query44 1321 697 700 697
query45 178 173 161 161
query46 842 951 565 565
query47 3015 3036 2928 2928
query48 302 312 213 213
query49 573 395 290 290
query50 696 282 208 208
query51 10265 10430 10444 10430
query52 118 80 69 69
query53 187 208 147 147
query54 230 220 175 175
query55 69 67 64 64
query56 229 212 210 210
query57 1600 1582 1570 1570
query58 281 268 255 255
query59 2068 2126 1969 1969
query60 294 239 227 227
query61 150 145 147 145
query62 391 332 288 288
query63 202 160 164 160
query64 2750 931 774 774
query65 3307 3246 3266 3246
query66 1771 411 323 323
query67 20234 20172 20058 20058
query68 3031 1019 611 611
query69 368 281 248 248
query70 883 785 808 785
query71 296 227 196 196
query72 2743 2817 2311 2311
query73 518 524 291 291
query74 4630 4452 4296 4296
query75 2342 2300 1939 1939
query76 2299 1016 617 617
query77 332 355 271 271
query78 9765 9741 9123 9123
query79 970 901 513 513
query80 580 469 377 377
query81 534 328 274 274
query82 673 139 113 113
query83 312 202 199 199
query84 322 129 142 129
query85 829 427 368 368
query86 326 241 230 230
query87 2051 1976 1823 1823
query88 3658 2717 2678 2678
query89 338 296 257 257
query90 1902 180 181 180
query91 172 152 134 134
query92 102 93 91 91
query93 1011 1012 574 574
query94 458 294 292 292
query95 584 407 329 329
query96 673 525 234 234
query97 2459 2414 2329 2329
query98 160 160 148 148
query99 780 773 647 647
Total cold run time: 233970 ms
Total hot run time: 152471 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6187494409
<details>
<summary>TPC-DS: <b>Total hot run time: 152477 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit f2c45d7e38c5514bc47dad67b822cdb1c6c044d6, data reload: false
query5 4321 584 445 445
query6 424 210 192 192
query7 4806 435 226 226
query8 319 187 156 156
query9 8775 4005 4007 4005
query10 476 316 266 266
query11 6064 3802 3521 3521
query12 165 91 84 84
query13 1240 463 327 327
query14 6171 4052 3757 3757
query14_1 3543 3571 3568 3568
query15 215 200 172 172
query16 940 447 446 446
query17 872 651 539 539
query18 2439 425 314 314
query19 195 162 142 142
query20 88 87 83 83
query21 214 138 117 117
query22 12933 13041 12859 12859
query23 13387 12758 12411 12411
query23_1 12498 12558 12392 12392
query24 7103 840 481 481
query24_1 479 487 479 479
query25 542 420 373 373
query26 1279 260 145 145
query27 2763 440 272 272
query28 4608 1977 1990 1977
query29 1570 598 465 465
query30 297 222 189 189
query31 1008 919 805 805
query32 146 95 92 92
query33 512 304 243 243
query34 914 864 507 507
query35 746 750 635 635
query36 837 804 725 725
query37 132 104 94 94
query38 1797 1733 1703 1703
query39 808 799 754 754
query39_1 742 735 752 735
query40 235 116 99 99
query41 72 70 73 70
query42 88 87 88 87
query43 367 385 335 335
query44 1331 702 716 702
query45 179 187 167 167
query46 850 962 579 579
query47 3013 2932 2931 2931
query48 312 311 254 254
query49 580 395 286 286
query50 668 280 227 227
query51 10511 10630 10285 10285
query52 80 83 70 70
query53 186 208 153 153
query54 229 193 188 188
query55 69 73 62 62
query56 219 221 215 215
query57 1593 1577 1547 1547
query58 292 262 254 254
query59 2134 2142 1992 1992
query60 306 242 223 223
query61 141 147 142 142
query62 393 345 289 289
query63 203 171 167 167
query64 2728 953 770 770
query65 3297 3254 3230 3230
query66 1767 415 303 303
query67 20514 20093 20158 20093
query68 3006 994 619 619
query69 394 326 255 255
query70 891 791 800 791
query71 294 227 210 210
query72 2703 2577 2318 2318
query73 508 536 297 297
query74 4580 4469 4315 4315
query75 2353 2299 1937 1937
query76 2298 1075 630 630
query77 336 380 288 288
query78 9809 9758 9209 9209
query79 968 902 518 518
query80 740 442 333 333
query81 557 321 268 268
query82 584 134 110 110
query83 276 195 182 182
query84 308 123 101 101
query85 925 432 363 363
query86 346 238 231 231
query87 2012 1939 1839 1839
query88 3675 2740 2720 2720
query89 348 299 253 253
query90 1762 185 178 178
query91 166 151 124 124
query92 98 82 95 82
query93 1009 1005 598 598
query94 473 313 267 267
query95 584 390 321 321
query96 690 523 237 237
query97 2470 2426 2295 2295
query98 166 152 149 149
query99 785 765 659 659
Total cold run time: 234496 ms
Total hot run time: 152477 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6262567502
PR approved by at least one committer and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6196226200
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6204424900
<details>
<summary>ClickBench: <b>Total hot run time: 24.62 s</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 359271837c1645906ec22bc27e06d34acaa66198, data reload: false
query1 0.00 0.00 0.01
query2 0.10 0.05 0.06
query3 0.26 0.10 0.10
query4 1.60 0.11 0.11
query5 0.26 0.26 0.23
query6 1.17 0.68 0.69
query7 0.04 0.01 0.00
query8 0.06 0.04 0.03
query9 0.40 0.34 0.35
query10 0.62 0.60 0.58
query11 0.21 0.14 0.14
query12 0.18 0.15 0.15
query13 0.50 0.51 0.50
query14 1.03 1.01 1.00
query15 0.65 0.61 0.60
query16 0.34 0.34 0.34
query17 1.12 1.14 1.14
query18 0.22 0.22 0.20
query19 2.19 2.07 2.06
query20 0.01 0.02 0.01
query21 15.52 0.28 0.16
query22 4.40 0.06 0.05
query23 16.24 0.32 0.11
query24 3.32 0.66 0.39
query25 0.11 0.04 0.04
query26 1.09 0.27 0.15
query27 0.05 0.04 0.04
query28 2.03 0.60 0.38
query29 12.51 4.22 3.32
query30 0.28 0.14 0.13
query31 2.78 0.58 0.33
query32 3.22 0.61 0.53
query33 3.22 3.23 3.19
query34 15.40 4.01 3.44
query35 3.36 3.36 3.31
query36 0.56 0.45 0.44
query37 0.09 0.06 0.06
query38 0.05 0.04 0.04
query39 0.04 0.03 0.03
query40 0.17 0.14 0.16
query41 0.09 0.03 0.03
query42 0.03 0.02 0.02
query43 0.04 0.03 0.03
Total cold run time: 95.56 s
Total hot run time: 24.62 s
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6205025822
# FE UT Coverage Report
Increment line coverage ` 100.00% (1/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69083_359271837c1645906ec22bc27e06d34acaa66198/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69083_359271837c1645906ec22bc27e06d34acaa66198/fe_report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6198775817
<details>
<summary>TPC-DS: <b>Total hot run time: 152586 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 1740cfa4637dea43b1143058633d5ffcc3e9348f, data reload: false
query5 4303 585 444 444
query6 433 224 208 208
query7 4819 464 233 233
query8 329 172 164 164
query9 8801 4024 4073 4024
query10 484 347 268 268
query11 6066 3838 3538 3538
query12 155 97 90 90
query13 1271 445 337 337
query14 6159 4074 3724 3724
query14_1 3558 3533 3539 3533
query15 210 201 187 187
query16 944 421 427 421
query17 886 662 534 534
query18 2423 433 324 324
query19 201 167 140 140
query20 87 85 92 85
query21 209 139 114 114
query22 12945 13080 12800 12800
query23 13390 12957 12589 12589
query23_1 12679 12559 12592 12559
query24 7096 835 485 485
query24_1 477 482 477 477
query25 546 433 368 368
query26 1282 261 144 144
query27 2754 438 278 278
query28 4618 1974 1997 1974
query29 1577 614 462 462
query30 307 229 194 194
query31 1007 939 810 810
query32 146 95 98 95
query33 526 353 255 255
query34 884 839 508 508
query35 764 789 651 651
query36 844 812 727 727
query37 143 98 89 89
query38 1806 1751 1691 1691
query39 799 834 747 747
query39_1 745 734 749 734
query40 226 117 100 100
query41 73 71 69 69
query42 89 96 86 86
query43 371 382 336 336
query44 1319 713 715 713
query45 197 178 168 168
query46 846 979 575 575
query47 3034 2906 2941 2906
query48 303 303 228 228
query49 588 396 303 303
query50 676 285 224 224
query51 10699 10489 10455 10455
query52 77 77 70 70
query53 187 205 156 156
query54 235 185 178 178
query55 74 73 64 64
query56 237 234 222 222
query57 1624 1596 1543 1543
query58 296 267 262 262
query59 2101 2147 1946 1946
query60 304 241 228 228
query61 151 155 139 139
query62 396 342 287 287
query63 196 169 172 169
query64 2756 927 770 770
query65 3300 3223 3237 3223
query66 1791 413 296 296
query67 20140 20067 19895 19895
query68 2944 1006 632 632
query69 385 284 252 252
query70 868 792 776 776
query71 310 223 217 217
query72 2723 2575 2272 2272
query73 513 526 308 308
query74 4603 4491 4282 4282
query75 2329 2311 1923 1923
query76 2283 1003 615 615
query77 335 378 293 293
query78 9677 9652 9147 9147
query79 995 841 537 537
query80 1253 442 346 346
query81 580 325 270 270
query82 614 140 117 117
query83 299 199 186 186
query84 310 126 97 97
query85 854 477 378 378
query86 399 243 225 225
query87 2019 1937 1835 1835
query88 3673 2731 2688 2688
query89 334 297 265 265
query90 1921 186 187 186
query91 164 157 127 127
query92 93 90 88 88
query93 1053 1022 587 587
query94 645 306 287 287
query95 590 343 342 342
query96 678 511 235 235
query97 2468 2458 2320 2320
query98 163 152 145 145
query99 787 768 651 651
Total cold run time: 235578 ms
Total hot run time: 152586 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6187591830
# FE UT Coverage Report
Increment line coverage ` 100.00% (1/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69083_f2c45d7e38c5514bc47dad67b822cdb1c6c044d6/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69083_f2c45d7e38c5514bc47dad67b822cdb1c6c044d6/fe_report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6224779165
<details>
<summary>ClickBench: <b>Total hot run time: 24.59 s</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 1012317776ab401cbf6f76ca82fd72a8431dd49b, data reload: false
query1 0.01 0.01 0.01
query2 0.10 0.05 0.05
query3 0.26 0.13 0.12
query4 1.61 0.12 0.12
query5 0.24 0.24 0.24
query6 1.17 0.68 0.68
query7 0.04 0.01 0.01
query8 0.06 0.04 0.03
query9 0.40 0.31 0.35
query10 0.57 0.60 0.61
query11 0.19 0.15 0.14
query12 0.19 0.14 0.14
query13 0.49 0.49 0.49
query14 1.02 1.02 1.01
query15 0.63 0.60 0.59
query16 0.35 0.31 0.31
query17 1.12 1.12 1.08
query18 0.23 0.21 0.20
query19 2.09 2.01 2.02
query20 0.02 0.01 0.02
query21 15.50 0.28 0.15
query22 4.44 0.06 0.05
query23 16.27 0.33 0.12
query24 3.03 0.55 0.44
query25 0.11 0.06 0.04
query26 1.30 0.26 0.15
query27 0.04 0.04 0.04
query28 1.95 0.63 0.43
query29 12.51 4.16 3.32
query30 0.28 0.14 0.14
query31 2.77 0.57 0.34
query32 3.22 0.61 0.52
query33 3.22 3.24 3.17
query34 15.38 4.03 3.38
query35 3.33 3.35 3.38
query36 0.54 0.44 0.44
query37 0.10 0.06 0.06
query38 0.05 0.04 0.04
query39 0.04 0.03 0.03
query40 0.17 0.15 0.15
query41 0.08 0.03 0.02
query42 0.04 0.03 0.03
query43 0.05 0.03 0.03
Total cold run time: 95.21 s
Total hot run time: 24.59 s
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6177803089
<details>
<summary>TPC-H: <b>Total hot run time: 29651 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit b58434116f09907b66d4f8375cd67a928aea2623, data reload: false
------ Round 1 ----------------------------------
============================================
q1 17605 5677 5555 5555
q2 2088 304 268 268
q3 10279 1393 852 852
q4 4679 470 347 347
q5 7433 788 553 553
q6 196 191 162 162
q7 775 794 609 609
q8 9256 1357 1098 1098
q9 5623 4295 4292 4292
q10 6848 1300 1015 1015
q11 431 255 237 237
q12 639 412 304 304
q13 18063 3588 2988 2988
q14 286 274 255 255
q15 q16 747 718 684 684
q17 1183 747 569 569
q18 7079 6172 6168 6168
q19 1101 957 601 601
q20 395 335 232 232
q21 5141 2607 2557 2557
q22 433 376 305 305
Total cold run time: 100280 ms
Total hot run time: 29651 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1 6550 6435 6538 6435
q2 742 589 568 568
q3 5575 5802 5322 5322
q4 2113 2198 1674 1674
q5 5866 5664 5543 5543
q6 250 206 167 167
q7 2322 2051 1822 1822
q8 3108 2799 2772 2772
q9 8068 7561 7543 7543
q10 3670 3603 3235 3235
q11 607 443 406 406
q12 668 705 520 520
q13 3288 3668 3025 3025
q14 297 295 270 270
q15 q16 678 699 617 617
q17 7844 7154 6943 6943
q18 13178 12259 13018 12259
q19 954 853 846 846
q20 2195 2192 1940 1940
q21 5933 4846 5070 4846
q22 497 486 411 411
Total cold run time: 74403 ms
Total hot run time: 67164 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4285192374
##########
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:
Fixed in `1740cfa4637`. This reproduces: in a Parquet table keyed by `c CHAR(3)` with `rowkind.field = rk`, `('k', '+i', 1)` was written unpadded, and so was `('j', NULL, 2)` when `rk` has `DEFAULT '+I'`, so a later `'+I'` of each key added a second row `'k '` or `'j '`. Paimon fills a NULL with the field's default (`DefaultValueRow`) and decodes the value ignoring case (`RowKind.fromShortString`) before it takes the kind of the row (`TableWriteImpl#writeAndReturn`), while the writer compared the value with `'+I'` byte by byte. It now decodes the kind with Paimon's own `DefaultValueRow` and `RowKindGenerator`, so `'+i'`, and a NULL or an omitted value under a `'+I'` or `'+i'` default, insert and are padded. A value Paimon cannot decode is left for Paimon's write to reject, as before.
Tests: `PaimonWriteSchemaTest.testRowKindFieldValueDecidesWhetherARowIsPadded` now covers `'+i'`, `'-d'`, an unknown value, and an explicit and an omitted NULL under `'+I'`, `'+i'` and `'-D'` defaults; not filling the default turns it red. `test_paimon_write_char_padding` writes `'+i'` and a NULL under `DEFAULT '+I'`, then `'+I'` rows of the same keys, and gets one padded row per key.
##########
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:
Fixed in `1740cfa4637`. This reproduces, and for more than this shape: the writer's rows always come from the Arrow batch, so a ROW key holding a ROW is a columnar row whose `hashCode` throws, and even a map of one such key in `MAP<ROW<c CHAR(2), r ROW<i INT>>, INT>` failed the row with `ColumnarRowData do not support hashCode`, whether the source was a Paimon table read natively or through JNI, or a Doris literal. Two ROW or ARRAY keys that padding made equal hit the unsupported `equals` instead of the intended error, and two ROW keys holding arrays were not caught at all, since `ColumnarArray` hashes by identity; an Avro table then kept only one of them. Keys are now compared by their bytes as the only field of a `BinaryRow` (`InternalRowSerializer`), and the error prints them as Paimon casts them to STRING, since `toString` of a columnar row calls the same `hashCode`: `turns the keys '{k, 1}' and '{k , 1}' into the same key '{k , 1}'`.
Tests: `PaimonWriteSchemaTest.testMapKeysOfArrowRowsAreComparedByTheirBytes` feeds rows read back from an Arrow batch: a ROW-of-ROW key is padded, and colliding ROW, ARRAY and ROW-with-ARRAY keys fail with the intended message. Hashing the key objects, or printing them with `toString`, turns it red. `test_paimon_write_char_padding` writes a map with a ROW-of-ROW key into a Spark-created table and fails a pair of such keys that padding makes equal; with the paimon-scanner jar from before this commit the suite fails at that first write.
##########
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:
Fixed in `1740cfa4637`. This reproduces, and for more than `'+U'`: in a table keyed by `(id, p)`, partitioned by `p CHAR(3)` and with `rowkind.field = rk`, a static overwrite of `p = 'x'` failed with `Trying to overwrite partition {p=x }, but the changes in p=x/ does not belong to this partition` for a `'+U'` row, and likewise for a `'-D'` row, in an ORC table, and for a `'+I'` and a `'-D'` of one key in one statement; with padding off all of them worked. An overwrite replaces the partitions it writes (`withIgnorePreviousFiles`), so none of its rows matches a stored key, and the reason the previous commit gave for leaving `'-D'`, `'-U'` and `'+U'` rows unpadded does not hold there. The writer now pads every row of an overwrite, as FE pads the static partition value of its filter: the `'+U'` row lands in `'x '`, and a `'+I'` and a `'-D'` of one key in the overwrite still cancel out. Writes other than overwrites are unchanged.
Tests: `PaimonWriteSchemaTest.testOverwritePadsEveryRowOfARowKindFieldTable` (the `'+I'`, `'+U'`, `'-D'` and `'-U'` rows of an overwrite are padded, only the partition value of an ORC table, and the `'+U'` row of a plain INSERT is not); padding by the decoded row kind in an overwrite turns it red. `test_paimon_write_char_padding` overwrites `p = 'x'` with a `'+U'` row, which reads back as `[x ]`, and `p = 'y'` with a `'+I'` and a `'-D'` of one key, which leaves the partition empty.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4286361871
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -167,9 +287,236 @@ GenericRow tableRow(InternalRow columnValues) {
// from an omitted field and retains Paimon's writer-side semantics.
row.setField(tableFieldIndexes[i], value);
}
+ if (padsRow(row)) {
+ padRow(row);
+ }
return row;
}
+ /**
+ * Whether to pad {@code row}: whether it inserts, outside an overwrite. 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#writeAndReturn}), after filling a NULL
+ * with the field's default and ignoring case: '+I', '+i', or NULL under DEFAULT '+I' inserts, while a '-D', '-U'
+ * or '+U' row carries a key to match, as an UPDATE or DELETE row does. A value Paimon cannot decode, such as NULL
+ * without a default, fails the write in Paimon with the same error, after Paimon's own NOT NULL check, so such a
+ * row is left as it is. An overwrite replaces the partitions it writes, so none of its rows matches a stored
+ * key, and all of them are padded, like the static partition value FE pads into the overwrite filter: a '+U'
+ * row of INSERT OVERWRITE ... PARTITION (p = 'x') has to land in 'x ', and a '+I' and a '-D' of one key in the
+ * statement still cancel out.
+ */
+ private boolean padsRow(GenericRow row) {
+ if (row.getRowKind() != RowKind.INSERT) {
+ return false;
+ }
+ if (overwrite || rowKindGenerator == null) {
+ return true;
+ }
+ try {
+ return rowKindGenerator.generate(defaultValueRow == null ? row : defaultValueRow.replaceRow(row))
+ == RowKind.INSERT;
+ } catch (RuntimeException e) {
+ return false;
+ }
+ }
+
+ private void padRow(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());
+ MapKeyFormat keyFormat = keyPadder == null ? null : new MapKeyFormat(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ return new PaddedMap(
+ keyPadder == null ? map.keyArray() : padMapKeys(map.keyArray(), keyGetter, keyPadder,
+ keyFormat, 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.
+ * Keys are compared and printed through {@code keyFormat}, never by their own hashCode, equals or toString: a key
+ * of a nested type holds the columnar rows and arrays of the Arrow batch, which support none of them
+ * (ColumnarArray keeps identity equality), so hashing the key of a MAP<ROW<c CHAR(2), r ROW<i INT>>, INT> itself
+ * would fail even a map of one entry.
+ */
+ private static InternalArray padMapKeys(InternalArray keys, InternalArray.ElementGetter keyGetter,
+ CharPadder keyPadder, MapKeyFormat keyFormat, String column) {
+ Object[] padded = new Object[keys.size()];
+ // Bytes of a padded key -> position of the first key padded into it.
+ Map<BinaryRow, Integer> firstKeyOfPadded = new HashMap<>();
+ for (int i = 0; i < padded.length; i++) {
+ Object key = keyGetter.getElementOrNull(keys, i);
+ padded[i] = key == null ? null : keyPadder.pad(key);
+ Integer first = firstKeyOfPadded.putIfAbsent(keyFormat.bytes(padded[i]), i);
+ if (first == null) {
+ continue;
+ }
+ Object firstKey = keyGetter.getElementOrNull(keys, first);
+ if (!keyFormat.bytes(firstKey).equals(keyFormat.bytes(key))) {
+ throw new IllegalArgumentException(String.format(
+ "Padding the CHAR keys of a map in column '%s' of a Paimon table to the CHAR length turns "
+ + "the keys '%s' and '%s' into the same key '%s', and a map holds one entry per key. "
+ + "Make the keys distinct, or set enable_paimon_write_char_padding = false",
+ column, keyFormat.text(firstKey), keyFormat.text(key), keyFormat.text(padded[i])));
+ }
+ }
+ return new GenericArray(padded);
+ }
+
+ /** Compares the keys of a map by their bytes and prints them as Paimon casts them to STRING, for any key type. */
+ private static final class MapKeyFormat {
+ private final InternalRowSerializer serializer;
+ private final CastExecutor<Object, Object> toText;
+
+ @SuppressWarnings("unchecked")
+ private MapKeyFormat(DataType keyType) {
+ this.serializer = new InternalRowSerializer(keyType);
+ this.toText = (CastExecutor<Object, Object>) CastExecutors.resolveToString(keyType);
+ }
+
+ /** The bytes of {@code key} as the only field of a row, copied since the serializer reuses its buffer. */
+ private BinaryRow bytes(Object key) {
+ return serializer.toBinaryRow(GenericRow.of(key)).copy();
Review Comment:
[P2] Compare MAP descendants independently of entry order
For an API-created Avro column `MAP<ROW<c CHAR(2), m MAP<STRING,INT>>,INT>`, insert the distinct keys `ROW('k', map('a',1,'b',2)) -> 10` and `ROW('k ', map('b',2,'a',1)) -> 20`. Both padded `c` values become `'k '`, but [InternalMapSerializer](https://github.com/apache/paimon/blob/release-1.4.2/paimon-common/src/main/java/org/apache/paimon/data/serializer/InternalMapSerializer.java#L99-L146) preserves the nested entry order, so these binary representations stay different and `padMapKeys` accepts both. On read, [GenericMap.equals](https://github.com/apache/paimon/blob/release-1.4.2/paimon-common/src/main/java/org/apache/paimon/data/GenericMap.java#L82-L115) ignores entry order; the outer [Avro ArrayMapReader](https://github.com/apache/paimon/blob/release-1.4.2/paimon-format/src/main/java/org/apache/paimon/format/avro/FieldReaderFactory.java#L487-L524) inserts the now-equal ROW keys into a HashMap and silently drops one value. This differs from the earlier unsupported Arrow object-h
ashing issue: serialization succeeds, but this new byte comparator disagrees with downstream equality. Canonicalize MAP descendants or compare keys recursively by type, and cover reversed inner-map order in an Avro round trip.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6204233743
<details>
<summary>TPC-H: <b>Total hot run time: 29615 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 359271837c1645906ec22bc27e06d34acaa66198, data reload: false
------ Round 1 ----------------------------------
============================================
q1 17568 5618 5558 5558
q2 2071 321 254 254
q3 10247 1380 843 843
q4 4676 467 342 342
q5 7432 794 530 530
q6 192 187 155 155
q7 760 793 609 609
q8 9254 1371 1126 1126
q9 5519 4286 4283 4283
q10 6824 1294 1038 1038
q11 439 262 239 239
q12 636 401 285 285
q13 18067 3504 2985 2985
q14 279 274 250 250
q15 q16 736 731 677 677
q17 1176 738 592 592
q18 7029 6211 6158 6158
q19 1126 943 634 634
q20 420 336 227 227
q21 4972 2556 2634 2556
q22 432 360 274 274
Total cold run time: 99855 ms
Total hot run time: 29615 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1 6486 6511 6473 6473
q2 702 568 557 557
q3 5580 5698 5198 5198
q4 2296 2152 1542 1542
q5 5696 5669 5684 5669
q6 247 195 163 163
q7 2360 2005 1843 1843
q8 3115 2744 2753 2744
q9 8230 7858 7587 7587
q10 3678 3632 3221 3221
q11 611 424 382 382
q12 657 699 504 504
q13 3278 3621 3058 3058
q14 288 291 267 267
q15 q16 673 696 626 626
q17 7796 6959 6847 6847
q18 13178 12301 12998 12301
q19 953 861 853 853
q20 2192 2192 1948 1948
q21 5985 4754 4981 4754
q22 504 461 416 416
Total cold run time: 74505 ms
Total hot run time: 66953 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6213402456
<details>
<summary>ClickBench: <b>Total hot run time: 24.47 s</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit c4acf1b45e17b099fd49d257e19a243e5b46756d, data reload: false
query1 0.01 0.01 0.01
query2 0.10 0.06 0.05
query3 0.26 0.12 0.11
query4 1.60 0.12 0.13
query5 0.24 0.22 0.24
query6 1.18 0.69 0.69
query7 0.04 0.02 0.00
query8 0.05 0.04 0.03
query9 0.40 0.32 0.34
query10 0.57 0.56 0.62
query11 0.20 0.15 0.15
query12 0.19 0.15 0.14
query13 0.50 0.50 0.49
query14 1.06 1.01 1.00
query15 0.65 0.63 0.61
query16 0.30 0.36 0.35
query17 1.11 1.17 1.13
query18 0.22 0.21 0.20
query19 2.06 2.02 2.05
query20 0.02 0.01 0.01
query21 15.51 0.28 0.15
query22 4.45 0.05 0.05
query23 16.24 0.32 0.12
query24 3.12 0.61 0.38
query25 0.12 0.05 0.05
query26 1.45 0.24 0.14
query27 0.06 0.03 0.03
query28 2.25 0.57 0.39
query29 12.54 4.17 3.30
query30 0.28 0.17 0.13
query31 2.77 0.57 0.34
query32 3.21 0.60 0.50
query33 3.13 3.22 3.16
query34 15.51 3.99 3.38
query35 3.35 3.38 3.36
query36 0.56 0.46 0.43
query37 0.09 0.06 0.06
query38 0.05 0.05 0.04
query39 0.04 0.03 0.03
query40 0.17 0.15 0.14
query41 0.08 0.02 0.02
query42 0.03 0.02 0.02
query43 0.04 0.04 0.04
Total cold run time: 95.81 s
Total hot run time: 24.47 s
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman merged PR #69083:
URL: https://github.com/apache/doris/pull/69083
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6210188210
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6224708029
<details>
<summary>TPC-DS: <b>Total hot run time: 151793 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 1012317776ab401cbf6f76ca82fd72a8431dd49b, data reload: false
query5 4304 578 460 460
query6 419 208 211 208
query7 4795 446 219 219
query8 317 168 162 162
query9 8778 3964 3944 3944
query10 476 303 256 256
query11 5938 3769 3525 3525
query12 138 92 86 86
query13 1250 431 322 322
query14 6118 4016 3727 3727
query14_1 3489 3501 3514 3501
query15 211 195 197 195
query16 949 386 422 386
query17 853 651 545 545
query18 2422 429 300 300
query19 208 160 144 144
query20 87 84 84 84
query21 218 136 114 114
query22 12900 12933 12859 12859
query23 13616 12863 12481 12481
query23_1 12619 12517 12469 12469
query24 7145 821 463 463
query24_1 466 479 490 479
query25 537 420 359 359
query26 1289 257 143 143
query27 2775 436 263 263
query28 4652 1964 1959 1959
query29 1540 593 444 444
query30 313 231 189 189
query31 1015 944 810 810
query32 135 131 90 90
query33 577 291 231 231
query34 1110 857 503 503
query35 734 743 648 648
query36 806 823 735 735
query37 131 94 88 88
query38 1776 1767 1682 1682
query39 791 814 756 756
query39_1 725 735 746 735
query40 210 134 94 94
query41 65 65 62 62
query42 83 84 78 78
query43 368 378 329 329
query44 1290 675 695 675
query45 191 176 161 161
query46 862 948 570 570
query47 2920 2970 2921 2921
query48 302 323 216 216
query49 578 388 307 307
query50 683 287 209 209
query51 10552 10289 10214 10214
query52 78 76 72 72
query53 195 203 151 151
query54 250 193 170 170
query55 68 69 63 63
query56 226 227 202 202
query57 1599 1581 1569 1569
query58 288 268 258 258
query59 2146 2153 1917 1917
query60 279 235 222 222
query61 142 146 176 146
query62 386 335 288 288
query63 193 167 165 165
query64 2714 925 770 770
query65 3289 3216 3225 3216
query66 1752 404 303 303
query67 20269 20120 20172 20120
query68 3092 992 617 617
query69 384 281 250 250
query70 869 774 782 774
query71 290 224 193 193
query72 2773 2566 2242 2242
query73 518 522 290 290
query74 4574 4425 4275 4275
query75 2354 2288 1923 1923
query76 2302 1076 612 612
query77 328 347 284 284
query78 9625 9692 9146 9146
query79 908 844 532 532
query80 532 419 335 335
query81 534 316 271 271
query82 394 135 112 112
query83 295 210 175 175
query84 324 119 98 98
query85 801 422 385 385
query86 322 237 229 229
query87 2032 1950 1828 1828
query88 3606 2695 2669 2669
query89 334 301 259 259
query90 1846 176 174 174
query91 167 151 122 122
query92 93 90 95 90
query93 991 1017 573 573
query94 447 308 267 267
query95 582 395 316 316
query96 672 530 234 234
query97 2457 2433 2339 2339
query98 160 152 148 148
query99 770 769 659 659
Total cold run time: 233752 ms
Total hot run time: 151793 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6172867684
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6179773291
<details>
<summary>TPC-DS: <b>Total hot run time: 152207 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 45e6e5d2eff67184390f37b61fbe0319ec1a9447, data reload: false
query5 4303 553 474 474
query6 422 205 192 192
query7 4822 438 218 218
query8 320 168 151 151
query9 8760 3941 3941 3941
query10 467 298 246 246
query11 6014 3800 3517 3517
query12 157 89 86 86
query13 1264 453 325 325
query14 6143 4043 3755 3755
query14_1 3527 3468 3481 3468
query15 210 208 177 177
query16 961 458 438 438
query17 864 661 578 578
query18 2410 415 301 301
query19 193 163 125 125
query20 83 92 78 78
query21 205 129 109 109
query22 12993 13026 12851 12851
query23 13306 12876 12389 12389
query23_1 12478 12649 12459 12459
query24 7113 829 469 469
query24_1 465 456 481 456
query25 505 392 337 337
query26 1294 251 132 132
query27 2823 432 261 261
query28 4627 1968 1953 1953
query29 1575 585 424 424
query30 294 218 183 183
query31 1022 923 799 799
query32 150 92 88 88
query33 513 304 257 257
query34 886 845 517 517
query35 735 759 660 660
query36 809 789 714 714
query37 126 102 88 88
query38 1800 1778 1707 1707
query39 787 820 756 756
query39_1 732 731 748 731
query40 215 123 93 93
query41 67 61 67 61
query42 85 85 84 84
query43 363 383 323 323
query44 1319 692 697 692
query45 174 171 161 161
query46 816 943 576 576
query47 3032 3000 2888 2888
query48 296 306 218 218
query49 571 398 297 297
query50 679 267 215 215
query51 10664 10652 10604 10604
query52 78 76 74 74
query53 184 209 149 149
query54 235 187 179 179
query55 76 80 61 61
query56 225 223 203 203
query57 1610 1572 1563 1563
query58 280 268 279 268
query59 2153 2147 1945 1945
query60 268 248 223 223
query61 145 148 144 144
query62 388 336 287 287
query63 194 165 165 165
query64 2730 948 836 836
query65 3298 3265 3244 3244
query66 1804 424 325 325
query67 20156 19997 20219 19997
query68 3036 1024 621 621
query69 392 301 276 276
query70 864 729 794 729
query71 302 229 211 211
query72 2842 2680 2412 2412
query73 505 519 290 290
query74 4603 4478 4294 4294
query75 2324 2283 1937 1937
query76 2311 999 619 619
query77 334 378 281 281
query78 9715 9724 9099 9099
query79 1014 862 532 532
query80 547 422 330 330
query81 537 316 268 268
query82 577 136 104 104
query83 288 207 187 187
query84 306 128 105 105
query85 820 433 375 375
query86 322 244 231 231
query87 2052 1959 1817 1817
query88 3614 2714 2702 2702
query89 336 290 259 259
query90 1930 178 174 174
query91 162 148 126 126
query92 129 88 81 81
query93 1031 1009 586 586
query94 459 309 294 294
query95 583 352 387 352
query96 675 523 233 233
query97 2472 2442 2338 2338
query98 166 151 150 150
query99 781 765 651 651
Total cold run time: 234212 ms
Total hot run time: 152207 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4274646965
##########
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:
[P1] Normalize CHAR keys before MERGE matching and cardinality checks
Padding here changes primary-key identity after `ConnectorChangelogPlanBuilder.addCardinalityChecks` has validated it. With an empty `(c CHAR(3), v INT)` primary-key target, MERGE source rows `(CAST('k' AS CHAR(3)), 1)` and `(CAST('k ' AS CHAR(3)), 2)` using `ON t.c = s.c WHEN NOT MATCHED THEN INSERT (c,v) VALUES (s.c,s.v)` follow this reduced plan:
```text
JNI sink: pad(c)
AssertTrue(count <= 1)
Window(count PARTITION BY c)
Project(op=INSERT, c=s.c, v=s.v)
Join(target, source, t.c=s.c)
```
Doris preserves these short CHAR values and compares their bytes, so both window counts are 1. This branch then turns both keys into `'k '`, and Paimon's deduplicate writer commits one surviving row instead of the required duplicate-insert error. A source `'k'` can likewise overwrite an existing `'k '` target through a NOT MATCHED-only MERGE. Normalize keys before matching/checking while preserving matched legacy keys, or reject this CHAR-key MERGE combination until it is supported; add both regressions.
##########
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:
[P2] Mirror the rowkind.field exemption in static partition padding
The writer now leaves the column named by `rowkind.field` unpadded, but this branch still pads it when it is a CHAR partition column. For a valid table `(id INT, rk CHAR(3))` with primary key `(id,rk)`, partition key `rk`, `bucket=1` and `rowkind.field=rk`, `INSERT OVERWRITE TABLE t PARTITION(rk='+I') SELECT 1` puts `'+I '` in the FE overwrite filter while the BE writes partition `'+I'`. Paimon rejects that file as outside the specified partition; a SELECT that returns no rows at runtime instead succeeds without clearing the existing `'+I'` partition. Apply the writer's control-field exemption here too, and cover nonempty and empty static overwrites. This is distinct from the earlier token-padding finding: its BE fix is present, but the FE overwrite filter now disagrees with it.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6179857179
<details>
<summary>ClickBench: <b>Total hot run time: 24.52 s</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 45e6e5d2eff67184390f37b61fbe0319ec1a9447, data reload: false
query1 0.01 0.00 0.01
query2 0.10 0.05 0.04
query3 0.27 0.12 0.12
query4 1.61 0.13 0.13
query5 0.24 0.24 0.25
query6 1.17 0.69 0.68
query7 0.04 0.01 0.01
query8 0.05 0.04 0.04
query9 0.40 0.35 0.36
query10 0.60 0.58 0.59
query11 0.20 0.14 0.14
query12 0.18 0.14 0.14
query13 0.48 0.50 0.49
query14 1.02 1.02 1.00
query15 0.62 0.60 0.61
query16 0.35 0.36 0.31
query17 1.15 1.11 1.13
query18 0.23 0.21 0.21
query19 2.07 1.94 2.04
query20 0.02 0.02 0.01
query21 15.50 0.27 0.14
query22 4.35 0.06 0.05
query23 16.25 0.31 0.12
query24 3.27 0.63 0.42
query25 0.10 0.08 0.06
query26 1.59 0.25 0.15
query27 0.04 0.04 0.04
query28 2.22 0.62 0.41
query29 12.50 4.18 3.35
query30 0.29 0.14 0.14
query31 2.78 0.57 0.33
query32 3.22 0.61 0.52
query33 3.20 3.26 3.18
query34 15.59 3.98 3.37
query35 3.35 3.32 3.38
query36 0.56 0.43 0.43
query37 0.10 0.06 0.07
query38 0.04 0.03 0.04
query39 0.04 0.03 0.03
query40 0.18 0.14 0.15
query41 0.08 0.03 0.03
query42 0.04 0.04 0.03
query43 0.05 0.03 0.03
Total cold run time: 96.15 s
Total hot run time: 24.52 s
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6172868216
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4291870773
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -167,9 +287,355 @@ GenericRow tableRow(InternalRow columnValues) {
// from an omitted field and retains Paimon's writer-side semantics.
row.setField(tableFieldIndexes[i], value);
}
+ if (padsRow(row)) {
+ padRow(row);
+ }
return row;
}
+ /**
+ * Whether to pad {@code row}: whether it inserts, outside an overwrite. 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#writeAndReturn}), after filling a NULL
+ * with the field's default and ignoring case: '+I', '+i', or NULL under DEFAULT '+I' inserts, while a '-D', '-U'
+ * or '+U' row carries a key to match, as an UPDATE or DELETE row does. A value Paimon cannot decode, such as NULL
+ * without a default, fails the write in Paimon with the same error, after Paimon's own NOT NULL check, so such a
+ * row is left as it is. An overwrite replaces the partitions it writes, so none of its rows matches a stored
+ * key, and all of them are padded, like the static partition value FE pads into the overwrite filter: a '+U'
+ * row of INSERT OVERWRITE ... PARTITION (p = 'x') has to land in 'x ', and a '+I' and a '-D' of one key in the
+ * statement still cancel out.
+ */
+ private boolean padsRow(GenericRow row) {
+ if (row.getRowKind() != RowKind.INSERT) {
+ return false;
+ }
+ if (overwrite || rowKindGenerator == null) {
+ return true;
+ }
+ try {
+ return rowKindGenerator.generate(defaultValueRow == null ? row : defaultValueRow.replaceRow(row))
+ == RowKind.INSERT;
+ } catch (RuntimeException e) {
+ return false;
+ }
+ }
+
+ private void padRow(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());
+ MapKeyFormat keyFormat = keyPadder == null ? null : new MapKeyFormat(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ return new PaddedMap(
+ keyPadder == null ? map.keyArray() : padMapKeys(map.keyArray(), keyGetter, keyPadder,
+ keyFormat, 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.
+ * Keys are equal as Paimon compares them: a map nested in a key is compared regardless of the order of its
+ * entries. Keys are compared and printed through {@code keyFormat}, never by their own hashCode, equals or
+ * toString: a key of a nested type holds the columnar rows and arrays of the Arrow batch, which support none of
+ * them (ColumnarArray keeps identity equality), so hashing the key of a MAP<ROW<c CHAR(2), r ROW<i INT>>, INT>
+ * itself would fail even a map of one entry.
+ */
+ private static InternalArray padMapKeys(InternalArray keys, InternalArray.ElementGetter keyGetter,
+ CharPadder keyPadder, MapKeyFormat keyFormat, String column) {
+ Object[] padded = new Object[keys.size()];
+ // Bytes of a padded key -> position of the first key padded into it.
+ Map<BinaryRow, Integer> firstKeyOfPadded = new HashMap<>();
+ for (int i = 0; i < padded.length; i++) {
+ Object key = keyGetter.getElementOrNull(keys, i);
+ padded[i] = key == null ? null : keyPadder.pad(key);
+ Integer first = firstKeyOfPadded.putIfAbsent(keyFormat.bytes(padded[i]), i);
+ if (first == null) {
+ continue;
+ }
+ Object firstKey = keyGetter.getElementOrNull(keys, first);
+ if (!keyFormat.bytes(firstKey).equals(keyFormat.bytes(key))) {
+ throw new IllegalArgumentException(String.format(
+ "Padding the CHAR keys of a map in column '%s' of a Paimon table to the CHAR length turns "
+ + "the keys '%s' and '%s' into the same key '%s', and a map holds one entry per key. "
+ + "Make the keys distinct, or set enable_paimon_write_char_padding = false",
+ column, keyFormat.text(firstKey), keyFormat.text(key), keyFormat.text(padded[i])));
+ }
+ }
+ return new GenericArray(padded);
+ }
+
+ /**
+ * Returns the sorter of a value of {@code type}, or null when the type holds no MAP. The sorter copies the value
+ * with the entries of each MAP in it, at any depth, sorted by the bytes of their keys: two values whose maps hold
+ * the same entries in another order, which Paimon takes as equal, then have the same bytes.
+ */
+ private static EntrySorter entrySorter(DataType type) {
+ switch (type.getTypeRoot()) {
+ case ARRAY: {
+ DataType elementType = ((ArrayType) type).getElementType();
+ EntrySorter elementSorter = entrySorter(elementType);
+ if (elementSorter == null) {
+ return null;
+ }
+ InternalArray.ElementGetter elementGetter = InternalArray.createElementGetter(elementType.nullable());
+ return value -> new GenericArray(sortElements((InternalArray) value, elementGetter, elementSorter));
+ }
+ case MAP: {
+ MapType mapType = (MapType) type;
+ EntrySorter keySorter = entrySorter(mapType.getKeyType());
+ EntrySorter valueSorter = entrySorter(mapType.getValueType());
+ InternalArray.ElementGetter keyGetter =
+ InternalArray.createElementGetter(mapType.getKeyType().nullable());
+ InternalArray.ElementGetter valueGetter =
+ InternalArray.createElementGetter(mapType.getValueType().nullable());
+ InternalRowSerializer keySerializer = new InternalRowSerializer(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ Object[] keys = sortElements(map.keyArray(), keyGetter, keySorter);
+ Object[] values = sortElements(map.valueArray(), valueGetter, valueSorter);
+ byte[][] keyBytes = new byte[keys.length][];
+ Integer[] order = new Integer[keys.length];
+ for (int i = 0; i < keys.length; i++) {
+ // Copied first: the serializer reuses its buffer, and toBytes returns that buffer itself when
+ // the row fills it.
+ keyBytes[i] = keySerializer.toBinaryRow(GenericRow.of(keys[i])).copy().toBytes();
+ order[i] = i;
+ }
+ Arrays.sort(order, (left, right) -> compareBytes(keyBytes[left], keyBytes[right]));
+ Object[] sortedKeys = new Object[keys.length];
+ Object[] sortedValues = new Object[keys.length];
+ for (int i = 0; i < keys.length; i++) {
+ sortedKeys[i] = keys[order[i]];
+ sortedValues[i] = values[order[i]];
+ }
+ return new PaddedMap(new GenericArray(sortedKeys), new GenericArray(sortedValues));
+ };
+ }
+ case ROW: {
+ List<DataField> fields = ((RowType) type).getFields();
+ EntrySorter[] fieldSorters = new EntrySorter[fields.size()];
+ InternalRow.FieldGetter[] fieldGetters = new InternalRow.FieldGetter[fields.size()];
+ boolean holdsMap = false;
+ for (int i = 0; i < fields.size(); i++) {
+ fieldSorters[i] = entrySorter(fields.get(i).type());
+ fieldGetters[i] = InternalRow.createFieldGetter(fields.get(i).type().nullable(), i);
+ holdsMap |= fieldSorters[i] != null;
+ }
+ if (!holdsMap) {
+ return null;
+ }
+ return value -> {
+ InternalRow nested = (InternalRow) value;
+ GenericRow sorted = new GenericRow(fieldGetters.length);
+ for (int i = 0; i < fieldGetters.length; i++) {
+ Object field = fieldGetters[i].getFieldOrNull(nested);
+ sorted.setField(i, field == null || fieldSorters[i] == null
+ ? field : fieldSorters[i].sort(field));
+ }
+ return sorted;
+ };
+ }
+ default:
+ return null;
+ }
+ }
+
+ /** The elements of {@code array}, each non-null one sorted by {@code sorter} unless the sorter is null. */
+ private static Object[] sortElements(InternalArray array, InternalArray.ElementGetter elementGetter,
+ EntrySorter sorter) {
+ Object[] elements = new Object[array.size()];
+ for (int i = 0; i < elements.length; i++) {
+ Object element = elementGetter.getElementOrNull(array, i);
+ elements[i] = element == null || sorter == null ? element : sorter.sort(element);
+ }
+ return elements;
+ }
+
+ /** Orders byte strings by their bytes, compared unsigned, then by their length. */
+ private static int compareBytes(byte[] left, byte[] right) {
+ for (int i = 0; i < Math.min(left.length, right.length); i++) {
+ int compared = Integer.compare(left[i] & 0xFF, right[i] & 0xFF);
+ if (compared != 0) {
+ return compared;
+ }
+ }
+ return Integer.compare(left.length, right.length);
+ }
+
+ /**
+ * Compares the keys of a map by their bytes and prints them as Paimon casts them to STRING, for any key type.
+ * Paimon compares maps regardless of the order of their entries ({@code GenericMap.equals}), and its Avro reader
+ * keeps one of two keys whose nested maps differ only in that order, so the entries of the maps nested in a key
+ * are sorted before the key is compared.
+ */
+ private static final class MapKeyFormat {
+ private final InternalRowSerializer serializer;
+ // Null when the key type holds no MAP.
+ private final EntrySorter entrySorter;
+ private final CastExecutor<Object, Object> toText;
+
+ @SuppressWarnings("unchecked")
+ private MapKeyFormat(DataType keyType) {
+ this.serializer = new InternalRowSerializer(keyType);
+ this.entrySorter = entrySorter(keyType);
+ this.toText = (CastExecutor<Object, Object>) CastExecutors.resolveToString(keyType);
+ }
+
+ /**
+ * The bytes of {@code key}, with the entries of its nested maps sorted, as the only field of a row, copied
+ * since the serializer reuses its buffer.
+ */
+ private BinaryRow bytes(Object key) {
+ Object sorted = key == null || entrySorter == null ? key : entrySorter.sort(key);
+ return serializer.toBinaryRow(GenericRow.of(sorted)).copy();
Review Comment:
Fixed in `c4acf1b45e1`. This reproduces as described. In an Avro table created through the Paimon API with `m MAP<ROW<c CHAR(2), d DOUBLE>, INT>`, `INSERT ... SELECT 1, map(named_struct('c', 'k', 'd', cast(pos AS DOUBLE)), 10, named_struct('c', 'k ', 'd', cast(neg AS DOUBLE)), 20)` from a Doris table holding `'NaN'` and `'-NaN'` succeeded, and Doris (native and JNI reads) read back the single entry `{"c":"k ", "d":NaN}: 20`; a FLOAT field behaves the same. A Parquet table kept both entries, two keys that Paimon takes as equal. With padding off both keys are kept, as before this PR, and the same pair with two positive NaNs already failed with the intended error.
Before the check serializes a key, it now also makes each FLOAT and DOUBLE NaN in the key, at any depth, the canonical NaN, besides sorting the entries of its nested maps. -0.0 stays apart from 0.0, as `Double.equals` keeps them apart for Paimon. All three tables now fail that write with `turns the keys '{k, NaN}' and '{k , NaN}' into the same key '{k , NaN}'`. Keys that were one key before padding, such as `('k ', NaN)` and `('k ', -NaN)`, are still written as they are; an Avro read keeps one of them with padding on or off, as on master.
Tests: `PaimonWriteSchemaTest.testNaNsInMapKeysAreComparedRegardlessOfTheirBits` feeds rows read back from an Arrow batch: NaNs of opposite signs collide in a DOUBLE field, a FLOAT field and a nested ROW, keys equal before padding are kept, and `('k', 0.0)` and `('k ', -0.0)` are padded and kept. Not normalizing the NaNs, or only the DOUBLE ones, or making -0.0 into 0.0, turns it red. `test_paimon_write_char_padding` now creates such an Avro table through Spark, fails the pair cast at run time from a Doris table and pads a single key holding a NaN; with the paimon-scanner jar from before this commit the suite fails at that write.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6199843300
# FE UT Coverage Report
Increment line coverage ` 100.00% (1/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69083_1740cfa4637dea43b1143058633d5ffcc3e9348f/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69083_1740cfa4637dea43b1143058633d5ffcc3e9348f/fe_report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6186201546
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6180689378
# FE Regression Coverage Report
Increment line coverage ` 100.00% (1/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69083_45e6e5d2eff67184390f37b61fbe0319ec1a9447_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69083_45e6e5d2eff67184390f37b61fbe0319ec1a9447_merge_fe/report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "924060929 (via GitHub)" <gi...@apache.org>.
924060929 commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4311182154
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonWritePlanProvider.java:
##########
@@ -409,16 +428,17 @@ private List<Integer> appendRouteFields(List<String> fieldNames,
return indexes;
}
- private boolean supportsNativeRouting(DataTypeRoot type) {
+ private boolean supportsNativeRouting(DataTypeRoot type, boolean charPadding) {
switch (type) {
+ case CHAR:
+ return !charPadding;
Review Comment:
Reviewed the FE changes at `1012317776ab401cbf6f76ca82fd72a8431dd49b`. The routing fix looks correct: eligibility now follows `charPaddedFields`, so ORC non-partition CHAR keys and `rowkind.field` retain native routing, while CHAR routing fields that the writer actually pads use the existing single-writer fallback where required. Expressing this through `ConnectorWriteDistribution` keeps the Paimon policy out of generic optimizer rules.
I also checked the static-partition filter alignment and the MERGE key-identity guard. Rejecting an inserted short CHAR primary key avoids changing its identity after join matching and cardinality checks. The remaining GATHER performance cost and the need for explicit `rpad` in such MERGE statements are reasonable, documented restrictions for this change. I found no additional FE correctness blockers.
This was a source review, including the relevant unit/regression test changes; I did not independently run them. The current head still has a failing `check_coverage_fe (Coverage)` check ([build 1072248](http://43.132.222.7:8111/viewLog.html?buildId=1072248&buildTypeId=Doris_Coverage_CheckCoverageFe)); please investigate that before merging. This comment covers FE and its writer-facing contracts, not approval of the separate Java-heap memory concern discussed above.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6177324998
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6210187892
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6198864894
<details>
<summary>ClickBench: <b>Total hot run time: 24.79 s</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 1740cfa4637dea43b1143058633d5ffcc3e9348f, data reload: false
query1 0.00 0.00 0.00
query2 0.10 0.05 0.05
query3 0.27 0.12 0.12
query4 1.61 0.13 0.13
query5 0.25 0.25 0.25
query6 1.17 0.70 0.70
query7 0.03 0.02 0.01
query8 0.06 0.04 0.03
query9 0.40 0.35 0.35
query10 0.59 0.58 0.61
query11 0.21 0.14 0.14
query12 0.18 0.15 0.14
query13 0.50 0.50 0.50
query14 1.02 1.00 1.03
query15 0.64 0.61 0.62
query16 0.36 0.37 0.36
query17 1.14 1.09 1.16
query18 0.23 0.23 0.22
query19 2.13 2.00 2.00
query20 0.02 0.02 0.01
query21 15.48 0.26 0.14
query22 4.39 0.06 0.05
query23 16.25 0.33 0.12
query24 3.12 0.66 0.42
query25 0.11 0.06 0.04
query26 1.23 0.27 0.15
query27 0.05 0.04 0.03
query28 1.73 0.61 0.40
query29 12.50 4.19 3.34
query30 0.28 0.14 0.14
query31 2.77 0.57 0.34
query32 3.22 0.61 0.52
query33 3.14 3.25 3.27
query34 15.44 4.00 3.40
query35 3.37 3.34 3.37
query36 0.55 0.45 0.48
query37 0.09 0.06 0.06
query38 0.06 0.04 0.04
query39 0.04 0.03 0.03
query40 0.18 0.15 0.15
query41 0.09 0.03 0.03
query42 0.03 0.03 0.03
query43 0.04 0.03 0.03
Total cold run time: 95.07 s
Total hot run time: 24.79 s
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6203627080
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6203627415
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6204361853
<details>
<summary>TPC-DS: <b>Total hot run time: 153042 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 359271837c1645906ec22bc27e06d34acaa66198, data reload: false
query5 4311 575 457 457
query6 419 210 188 188
query7 4809 444 238 238
query8 328 184 164 164
query9 8788 4100 4101 4100
query10 491 317 267 267
query11 6064 3773 3557 3557
query12 155 99 89 89
query13 1266 459 329 329
query14 6118 4107 3798 3798
query14_1 3525 3509 3501 3501
query15 213 206 198 198
query16 949 439 412 412
query17 867 683 538 538
query18 2410 413 320 320
query19 209 162 135 135
query20 84 85 85 85
query21 206 140 112 112
query22 12972 12932 12968 12932
query23 13510 12918 12618 12618
query23_1 12700 12598 12691 12598
query24 7057 834 455 455
query24_1 463 478 471 471
query25 525 388 330 330
query26 1320 264 137 137
query27 2753 440 268 268
query28 4604 1965 1966 1965
query29 1566 577 446 446
query30 299 221 184 184
query31 1016 918 809 809
query32 139 92 83 83
query33 509 298 230 230
query34 887 854 489 489
query35 736 751 632 632
query36 800 829 725 725
query37 149 100 88 88
query38 1805 1747 1684 1684
query39 792 819 743 743
query39_1 730 743 741 741
query40 215 115 97 97
query41 66 66 63 63
query42 88 91 86 86
query43 365 383 332 332
query44 1318 701 712 701
query45 184 170 162 162
query46 862 970 580 580
query47 2992 3011 2946 2946
query48 304 321 231 231
query49 576 423 301 301
query50 704 291 232 232
query51 10620 10485 10399 10399
query52 83 78 68 68
query53 189 205 154 154
query54 232 186 181 181
query55 73 67 65 65
query56 234 220 200 200
query57 1635 1581 1580 1580
query58 303 260 258 258
query59 2091 2112 1919 1919
query60 286 238 228 228
query61 151 167 159 159
query62 403 341 290 290
query63 203 165 165 165
query64 2864 1038 889 889
query65 3278 3249 3251 3249
query66 1811 424 313 313
query67 20287 20204 20062 20062
query68 3029 1022 627 627
query69 400 291 258 258
query70 875 812 791 791
query71 309 249 219 219
query72 2729 2567 2318 2318
query73 517 536 299 299
query74 4600 4488 4302 4302
query75 2337 2291 1928 1928
query76 2293 1017 618 618
query77 344 370 275 275
query78 9821 9779 9218 9218
query79 986 931 529 529
query80 1157 431 335 335
query81 574 311 269 269
query82 580 140 115 115
query83 281 205 181 181
query84 305 121 106 106
query85 1070 460 367 367
query86 358 233 232 232
query87 2028 1953 1820 1820
query88 3687 2734 2705 2705
query89 333 299 265 265
query90 1720 183 184 183
query91 159 147 124 124
query92 99 90 90 90
query93 1075 1017 587 587
query94 552 312 271 271
query95 593 346 387 346
query96 674 537 239 239
query97 2458 2467 2291 2291
query98 169 153 149 149
query99 782 769 661 661
Total cold run time: 235661 ms
Total hot run time: 153042 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6213165340
<details>
<summary>TPC-H: <b>Total hot run time: 29553 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit c4acf1b45e17b099fd49d257e19a243e5b46756d, data reload: false
------ Round 1 ----------------------------------
============================================
q1 17583 5750 5547 5547
q2 2134 296 257 257
q3 10269 1399 843 843
q4 4680 465 338 338
q5 7431 800 542 542
q6 186 189 159 159
q7 755 789 617 617
q8 9253 1354 1092 1092
q9 5531 4270 4252 4252
q10 6800 1300 1016 1016
q11 422 248 239 239
q12 640 398 283 283
q13 18088 3485 2987 2987
q14 274 262 250 250
q15 q16 726 722 677 677
q17 1196 742 576 576
q18 7030 6231 6208 6208
q19 1101 951 588 588
q20 393 337 225 225
q21 4968 2548 2699 2548
q22 403 348 309 309
Total cold run time: 99863 ms
Total hot run time: 29553 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1 6700 6701 6698 6698
q2 733 621 560 560
q3 5585 5731 5324 5324
q4 2074 2147 1572 1572
q5 5689 5474 5814 5474
q6 238 201 148 148
q7 2278 2034 1746 1746
q8 3126 2732 2795 2732
q9 8114 8016 7906 7906
q10 3668 3605 3228 3228
q11 601 407 368 368
q12 660 696 495 495
q13 3363 3604 3047 3047
q14 285 286 257 257
q15 q16 662 680 607 607
q17 7914 7028 6971 6971
q18 13211 12346 13037 12346
q19 928 795 796 795
q20 2192 2160 1962 1962
q21 5843 4713 4823 4713
q22 502 462 397 397
Total cold run time: 74366 ms
Total hot run time: 67346 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4289115960
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -167,9 +287,355 @@ GenericRow tableRow(InternalRow columnValues) {
// from an omitted field and retains Paimon's writer-side semantics.
row.setField(tableFieldIndexes[i], value);
}
+ if (padsRow(row)) {
+ padRow(row);
+ }
return row;
}
+ /**
+ * Whether to pad {@code row}: whether it inserts, outside an overwrite. 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#writeAndReturn}), after filling a NULL
+ * with the field's default and ignoring case: '+I', '+i', or NULL under DEFAULT '+I' inserts, while a '-D', '-U'
+ * or '+U' row carries a key to match, as an UPDATE or DELETE row does. A value Paimon cannot decode, such as NULL
+ * without a default, fails the write in Paimon with the same error, after Paimon's own NOT NULL check, so such a
+ * row is left as it is. An overwrite replaces the partitions it writes, so none of its rows matches a stored
+ * key, and all of them are padded, like the static partition value FE pads into the overwrite filter: a '+U'
+ * row of INSERT OVERWRITE ... PARTITION (p = 'x') has to land in 'x ', and a '+I' and a '-D' of one key in the
+ * statement still cancel out.
+ */
+ private boolean padsRow(GenericRow row) {
+ if (row.getRowKind() != RowKind.INSERT) {
+ return false;
+ }
+ if (overwrite || rowKindGenerator == null) {
+ return true;
+ }
+ try {
+ return rowKindGenerator.generate(defaultValueRow == null ? row : defaultValueRow.replaceRow(row))
+ == RowKind.INSERT;
+ } catch (RuntimeException e) {
+ return false;
+ }
+ }
+
+ private void padRow(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());
+ MapKeyFormat keyFormat = keyPadder == null ? null : new MapKeyFormat(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ return new PaddedMap(
+ keyPadder == null ? map.keyArray() : padMapKeys(map.keyArray(), keyGetter, keyPadder,
+ keyFormat, 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.
+ * Keys are equal as Paimon compares them: a map nested in a key is compared regardless of the order of its
+ * entries. Keys are compared and printed through {@code keyFormat}, never by their own hashCode, equals or
+ * toString: a key of a nested type holds the columnar rows and arrays of the Arrow batch, which support none of
+ * them (ColumnarArray keeps identity equality), so hashing the key of a MAP<ROW<c CHAR(2), r ROW<i INT>>, INT>
+ * itself would fail even a map of one entry.
+ */
+ private static InternalArray padMapKeys(InternalArray keys, InternalArray.ElementGetter keyGetter,
+ CharPadder keyPadder, MapKeyFormat keyFormat, String column) {
+ Object[] padded = new Object[keys.size()];
+ // Bytes of a padded key -> position of the first key padded into it.
+ Map<BinaryRow, Integer> firstKeyOfPadded = new HashMap<>();
+ for (int i = 0; i < padded.length; i++) {
+ Object key = keyGetter.getElementOrNull(keys, i);
+ padded[i] = key == null ? null : keyPadder.pad(key);
+ Integer first = firstKeyOfPadded.putIfAbsent(keyFormat.bytes(padded[i]), i);
+ if (first == null) {
+ continue;
+ }
+ Object firstKey = keyGetter.getElementOrNull(keys, first);
+ if (!keyFormat.bytes(firstKey).equals(keyFormat.bytes(key))) {
+ throw new IllegalArgumentException(String.format(
+ "Padding the CHAR keys of a map in column '%s' of a Paimon table to the CHAR length turns "
+ + "the keys '%s' and '%s' into the same key '%s', and a map holds one entry per key. "
+ + "Make the keys distinct, or set enable_paimon_write_char_padding = false",
+ column, keyFormat.text(firstKey), keyFormat.text(key), keyFormat.text(padded[i])));
+ }
+ }
+ return new GenericArray(padded);
+ }
+
+ /**
+ * Returns the sorter of a value of {@code type}, or null when the type holds no MAP. The sorter copies the value
+ * with the entries of each MAP in it, at any depth, sorted by the bytes of their keys: two values whose maps hold
+ * the same entries in another order, which Paimon takes as equal, then have the same bytes.
+ */
+ private static EntrySorter entrySorter(DataType type) {
+ switch (type.getTypeRoot()) {
+ case ARRAY: {
+ DataType elementType = ((ArrayType) type).getElementType();
+ EntrySorter elementSorter = entrySorter(elementType);
+ if (elementSorter == null) {
+ return null;
+ }
+ InternalArray.ElementGetter elementGetter = InternalArray.createElementGetter(elementType.nullable());
+ return value -> new GenericArray(sortElements((InternalArray) value, elementGetter, elementSorter));
+ }
+ case MAP: {
+ MapType mapType = (MapType) type;
+ EntrySorter keySorter = entrySorter(mapType.getKeyType());
+ EntrySorter valueSorter = entrySorter(mapType.getValueType());
+ InternalArray.ElementGetter keyGetter =
+ InternalArray.createElementGetter(mapType.getKeyType().nullable());
+ InternalArray.ElementGetter valueGetter =
+ InternalArray.createElementGetter(mapType.getValueType().nullable());
+ InternalRowSerializer keySerializer = new InternalRowSerializer(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ Object[] keys = sortElements(map.keyArray(), keyGetter, keySorter);
+ Object[] values = sortElements(map.valueArray(), valueGetter, valueSorter);
+ byte[][] keyBytes = new byte[keys.length][];
+ Integer[] order = new Integer[keys.length];
+ for (int i = 0; i < keys.length; i++) {
+ // Copied first: the serializer reuses its buffer, and toBytes returns that buffer itself when
+ // the row fills it.
+ keyBytes[i] = keySerializer.toBinaryRow(GenericRow.of(keys[i])).copy().toBytes();
+ order[i] = i;
+ }
+ Arrays.sort(order, (left, right) -> compareBytes(keyBytes[left], keyBytes[right]));
+ Object[] sortedKeys = new Object[keys.length];
+ Object[] sortedValues = new Object[keys.length];
+ for (int i = 0; i < keys.length; i++) {
+ sortedKeys[i] = keys[order[i]];
+ sortedValues[i] = values[order[i]];
+ }
+ return new PaddedMap(new GenericArray(sortedKeys), new GenericArray(sortedValues));
+ };
+ }
+ case ROW: {
+ List<DataField> fields = ((RowType) type).getFields();
+ EntrySorter[] fieldSorters = new EntrySorter[fields.size()];
+ InternalRow.FieldGetter[] fieldGetters = new InternalRow.FieldGetter[fields.size()];
+ boolean holdsMap = false;
+ for (int i = 0; i < fields.size(); i++) {
+ fieldSorters[i] = entrySorter(fields.get(i).type());
+ fieldGetters[i] = InternalRow.createFieldGetter(fields.get(i).type().nullable(), i);
+ holdsMap |= fieldSorters[i] != null;
+ }
+ if (!holdsMap) {
+ return null;
+ }
+ return value -> {
+ InternalRow nested = (InternalRow) value;
+ GenericRow sorted = new GenericRow(fieldGetters.length);
+ for (int i = 0; i < fieldGetters.length; i++) {
+ Object field = fieldGetters[i].getFieldOrNull(nested);
+ sorted.setField(i, field == null || fieldSorters[i] == null
+ ? field : fieldSorters[i].sort(field));
+ }
+ return sorted;
+ };
+ }
+ default:
+ return null;
+ }
+ }
+
+ /** The elements of {@code array}, each non-null one sorted by {@code sorter} unless the sorter is null. */
+ private static Object[] sortElements(InternalArray array, InternalArray.ElementGetter elementGetter,
+ EntrySorter sorter) {
+ Object[] elements = new Object[array.size()];
+ for (int i = 0; i < elements.length; i++) {
+ Object element = elementGetter.getElementOrNull(array, i);
+ elements[i] = element == null || sorter == null ? element : sorter.sort(element);
+ }
+ return elements;
+ }
+
+ /** Orders byte strings by their bytes, compared unsigned, then by their length. */
+ private static int compareBytes(byte[] left, byte[] right) {
+ for (int i = 0; i < Math.min(left.length, right.length); i++) {
+ int compared = Integer.compare(left[i] & 0xFF, right[i] & 0xFF);
+ if (compared != 0) {
+ return compared;
+ }
+ }
+ return Integer.compare(left.length, right.length);
+ }
+
+ /**
+ * Compares the keys of a map by their bytes and prints them as Paimon casts them to STRING, for any key type.
+ * Paimon compares maps regardless of the order of their entries ({@code GenericMap.equals}), and its Avro reader
+ * keeps one of two keys whose nested maps differ only in that order, so the entries of the maps nested in a key
+ * are sorted before the key is compared.
+ */
+ private static final class MapKeyFormat {
+ private final InternalRowSerializer serializer;
+ // Null when the key type holds no MAP.
+ private final EntrySorter entrySorter;
+ private final CastExecutor<Object, Object> toText;
+
+ @SuppressWarnings("unchecked")
+ private MapKeyFormat(DataType keyType) {
+ this.serializer = new InternalRowSerializer(keyType);
+ this.entrySorter = entrySorter(keyType);
+ this.toText = (CastExecutor<Object, Object>) CastExecutors.resolveToString(keyType);
+ }
+
+ /**
+ * The bytes of {@code key}, with the entries of its nested maps sorted, as the only field of a row, copied
+ * since the serializer reuses its buffer.
+ */
+ private BinaryRow bytes(Object key) {
+ Object sorted = key == null || entrySorter == null ? key : entrySorter.sort(key);
+ return serializer.toBinaryRow(GenericRow.of(sorted)).copy();
Review Comment:
[P2] Canonicalize NaNs before comparing padded ROW map keys
For an Avro `MAP<ROW<c CHAR(2), d DOUBLE>, INT>` column, read VARCHAR source fields containing `'NaN'` and `'-NaN'`, and construct keys `named_struct('c','k','d',CAST(pos AS DOUBLE)) -> 10` and `named_struct('c','k ','d',CAST(neg AS DOUBLE)) -> 20` in `INSERT ... SELECT`. These runtime casts preserve different NaN signs through Arrow. Padding makes both CHAR fields `'k '`, but [Paimon's serializer preserves raw NaN bits](https://github.com/apache/paimon/blob/release-1.4.2/paimon-common/src/main/java/org/apache/paimon/memory/MemorySegment.java#L366-L367), so this byte comparison accepts both keys. On read, [GenericRow equality/hash](https://github.com/apache/paimon/blob/release-1.4.2/paimon-common/src/main/java/org/apache/paimon/data/GenericRow.java#L221-L235) treats these NaNs as equal, and Avro's [ArrayMapReader](https://github.com/apache/paimon/blob/release-1.4.2/paimon-format/src/main/java/org/apache/paimon/format/avro/FieldReaderFactory.java#L517-L523) inserts the now-equal ke
ys into a HashMap, silently dropping one value. Before padding the CHAR fields differ and both entries survive. Canonicalize NaNs in comparison copies or use matching semantic equality, and cover this Arrow-to-Avro case. Unlike the existing MAP-order thread, this key has no MAP descendant for `entrySorter` to normalize.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6206292542
# FE Regression Coverage Report
Increment line coverage ` 100.00% (1/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69083_359271837c1645906ec22bc27e06d34acaa66198_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69083_359271837c1645906ec22bc27e06d34acaa66198_merge_fe/report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6174579721
<details>
<summary>ClickBench: <b>Total hot run time: 24.56 s</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit f6b658b9ee7ab5479e82110fdb85b63376e8a9ab, data reload: false
query1 0.01 0.01 0.01
query2 0.10 0.05 0.06
query3 0.26 0.11 0.12
query4 1.61 0.12 0.12
query5 0.24 0.24 0.24
query6 1.18 0.69 0.70
query7 0.03 0.01 0.00
query8 0.06 0.03 0.03
query9 0.40 0.34 0.36
query10 0.56 0.58 0.59
query11 0.21 0.14 0.14
query12 0.18 0.15 0.15
query13 0.48 0.48 0.49
query14 1.03 1.01 1.01
query15 0.63 0.60 0.59
query16 0.36 0.35 0.33
query17 1.14 1.14 1.14
query18 0.22 0.20 0.20
query19 2.15 2.02 2.02
query20 0.02 0.01 0.01
query21 15.49 0.25 0.15
query22 4.50 0.05 0.05
query23 16.29 0.33 0.12
query24 3.26 0.56 0.36
query25 0.11 0.05 0.05
query26 1.07 0.27 0.15
query27 0.05 0.03 0.05
query28 1.45 0.60 0.41
query29 12.53 4.24 3.38
query30 0.28 0.14 0.15
query31 2.78 0.58 0.34
query32 3.22 0.61 0.51
query33 3.08 3.26 3.20
query34 15.60 3.92 3.35
query35 3.32 3.33 3.30
query36 0.56 0.46 0.42
query37 0.09 0.06 0.06
query38 0.05 0.03 0.04
query39 0.04 0.03 0.03
query40 0.18 0.15 0.15
query41 0.08 0.04 0.03
query42 0.04 0.03 0.03
query43 0.04 0.03 0.04
Total cold run time: 94.98 s
Total hot run time: 24.56 s
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6179592624
<details>
<summary>TPC-H: <b>Total hot run time: 29584 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 45e6e5d2eff67184390f37b61fbe0319ec1a9447, data reload: false
------ Round 1 ----------------------------------
============================================
q1 17574 5656 5548 5548
q2 2090 301 247 247
q3 10280 1392 830 830
q4 4678 471 361 361
q5 7430 802 533 533
q6 196 199 155 155
q7 772 821 607 607
q8 9274 1342 1078 1078
q9 5477 4279 4281 4279
q10 6824 1294 1036 1036
q11 438 253 243 243
q12 633 398 283 283
q13 18117 3518 3008 3008
q14 273 279 249 249
q15 q16 738 727 667 667
q17 1187 740 570 570
q18 7016 6221 6182 6182
q19 1104 949 622 622
q20 383 331 230 230
q21 5459 2564 2609 2564
q22 421 341 292 292
Total cold run time: 100364 ms
Total hot run time: 29584 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1 6540 6499 6365 6365
q2 726 592 557 557
q3 5637 5688 5166 5166
q4 2293 2177 1586 1586
q5 5850 5668 5617 5617
q6 286 193 153 153
q7 2326 2024 1844 1844
q8 3127 2786 2818 2786
q9 8223 7796 7543 7543
q10 3638 3611 3217 3217
q11 620 424 371 371
q12 664 696 508 508
q13 3370 3632 3068 3068
q14 291 284 267 267
q15 q16 682 680 613 613
q17 7918 6986 6975 6975
q18 13163 12296 13046 12296
q19 897 811 807 807
q20 2190 2190 1935 1935
q21 5822 4706 4825 4706
q22 521 453 432 432
Total cold run time: 74784 ms
Total hot run time: 66812 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4274463612
##########
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:
Fixed in b58434116f0. You are right, and it reproduces. On a local cluster with a file:// catalog I wrote a CHAR(3) primary-key table with `bucket = 4` and `changelog-producer = full-compaction` through `INSERT ... SELECT ... FROM numbers("number" = "100")` with four writers. Every bucket received files from all four writers. Paimon committed the APPEND snapshot and then gave up the compaction with `LSM conflicts detected` (two level-5 files of bucket 2 from different writers); the following INSERTs gave it up with `File deletion conflicts detected`. Each INSERT still returned COMMITTED, and the table had only APPEND snapshots, so no changelog was written.
The fix takes your second route. With padding on, `PaimonWritePlanProvider` no longer routes a CHAR partition or bucket field natively (`supportsNativeRouting`), so `fixedBucketDistribution` returns null and the table gets the distribution of a routing field that BE cannot hash: GATHER to one writer for a primary-key table or one that compacts. With padding off the route is unchanged. Normalizing the exchanged values would mean padding in BE's route function, and only for the rows the writer pads, which are the INSERT rows and not the UPDATE and DELETE rows of a MERGE; that is a larger change than this PR should carry.
Tests:
- `PaimonWritePlanProviderTest.charRouteFieldsFallBackToSingleWriterWhenCharPaddingIsOn`: a CHAR(3) primary key and a CHAR(3) partition key give GATHER with padding on and `paimon_fixed_bucket` with it off.
- `test_paimon_write_char_padding` now writes a CHAR(3) primary-key table with `bucket = 4` and `changelog-producer = full-compaction` from `numbers()` with `parallel_pipeline_task_num = 4`, and checks `$snapshots`: an APPEND, then a COMPACT with 100 changelog records. With the jars from before this fix it fails there, with no COMPACT snapshot.
- On the local cluster, EXPLAIN of that INSERT now shows `DistributionSpecGather`, and every INSERT commits an APPEND and a COMPACT snapshot.
##########
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:
Fixed in b58434116f0. `PaimonWriteSchema` now leaves the column that the table names as `rowkind.field` unpadded (`PaimonJniWriter` passes `CoreOptions.rowkindField()`), and pads the other CHAR columns as before. Before the fix, `INSERT INTO t VALUES (3, '+I', 'c', 30)` into a table with `rk CHAR(3)` and `'rowkind.field' = 'rk'` failed with `Unsupported short string '+I ' for row kind`.
Tests: `PaimonWriteSchemaTest.testCharPaddingLeavesTheRowKindFieldAlone`; and `test_paimon_write_char_padding` writes two `'+I'` rows and then a `'-D'` row into such a table, and checks that the `'-D'` row deletes key 1 while the other CHAR column is padded.
##########
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:
Not changing this. The padded value is the value Paimon stores: an `ARRAY<CHAR(65535)>` with 4096 `'a'` elements is a 256 MiB row once padded, Paimon serializes that row when it writes it, and Spark and Flink build the same row in their JVMs. The writer pads one row at a time, and the padded values are garbage once `writer.write` has taken the row. The amplification comes from the CHAR length the table was declared with; Doris itself does not create a CHAR longer than 255 and reads a longer Paimon CHAR as STRING. Charging the padded bytes to the query would need a new bridge from these Java heap allocations to Doris's memory tracker, which is out of proportion to this case.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6175020102
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6175020559
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6187573827
<details>
<summary>ClickBench: <b>Total hot run time: 24.53 s</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit f2c45d7e38c5514bc47dad67b822cdb1c6c044d6, data reload: false
query1 0.00 0.00 0.00
query2 0.10 0.05 0.06
query3 0.26 0.12 0.11
query4 1.61 0.13 0.12
query5 0.25 0.22 0.25
query6 1.17 0.68 0.68
query7 0.04 0.01 0.01
query8 0.05 0.04 0.04
query9 0.41 0.34 0.35
query10 0.58 0.61 0.58
query11 0.21 0.15 0.14
query12 0.19 0.14 0.15
query13 0.48 0.49 0.49
query14 1.02 1.01 1.02
query15 0.63 0.61 0.62
query16 0.36 0.37 0.36
query17 1.15 1.11 1.11
query18 0.23 0.22 0.21
query19 2.09 1.94 2.03
query20 0.02 0.01 0.02
query21 15.48 0.25 0.14
query22 4.43 0.06 0.05
query23 16.27 0.31 0.12
query24 3.28 0.60 0.42
query25 0.11 0.07 0.04
query26 1.12 0.27 0.15
query27 0.05 0.03 0.03
query28 1.98 0.64 0.41
query29 12.51 4.14 3.31
query30 0.28 0.13 0.14
query31 2.77 0.60 0.34
query32 3.23 0.61 0.52
query33 3.27 3.25 3.23
query34 15.54 4.00 3.40
query35 3.36 3.32 3.35
query36 0.55 0.48 0.42
query37 0.09 0.06 0.06
query38 0.05 0.04 0.03
query39 0.03 0.03 0.03
query40 0.17 0.14 0.13
query41 0.08 0.03 0.03
query42 0.03 0.02 0.02
query43 0.04 0.04 0.03
Total cold run time: 95.57 s
Total hot run time: 24.53 s
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6225390331
# FE UT Coverage Report
Increment line coverage ` 100.00% (1/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69083_1012317776ab401cbf6f76ca82fd72a8431dd49b/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69083_1012317776ab401cbf6f76ca82fd72a8431dd49b/fe_report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6172865188
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://doris.apache.org/community/how-to-contribute/review-and-ci).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6177981333
<details>
<summary>TPC-DS: <b>Total hot run time: 152669 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit b58434116f09907b66d4f8375cd67a928aea2623, data reload: false
query5 4302 604 449 449
query6 424 205 195 195
query7 4824 446 246 246
query8 336 167 159 159
query9 8759 4014 4082 4014
query10 461 317 261 261
query11 6078 3782 3536 3536
query12 149 98 88 88
query13 1251 444 324 324
query14 6106 4002 3752 3752
query14_1 3524 3496 3480 3480
query15 209 195 176 176
query16 943 436 417 417
query17 869 668 534 534
query18 2421 420 316 316
query19 199 161 135 135
query20 88 86 83 83
query21 211 137 125 125
query22 12991 12999 12952 12952
query23 13394 12858 12314 12314
query23_1 12564 12486 12475 12475
query24 7103 838 456 456
query24_1 480 476 460 460
query25 507 385 329 329
query26 1271 253 132 132
query27 2781 439 269 269
query28 4614 1932 1941 1932
query29 1528 573 430 430
query30 305 221 181 181
query31 1008 933 800 800
query32 139 95 87 87
query33 514 290 239 239
query34 882 851 504 504
query35 757 749 648 648
query36 853 788 720 720
query37 123 100 87 87
query38 1824 1767 1703 1703
query39 805 796 749 749
query39_1 730 733 743 733
query40 212 116 93 93
query41 67 65 67 65
query42 83 84 79 79
query43 365 388 325 325
query44 1335 707 706 706
query45 182 175 158 158
query46 872 959 573 573
query47 3007 3010 2960 2960
query48 301 304 223 223
query49 581 400 294 294
query50 712 276 214 214
query51 10350 10503 10586 10503
query52 76 80 80 80
query53 191 202 165 165
query54 254 191 177 177
query55 73 71 64 64
query56 233 215 214 214
query57 1605 1607 1554 1554
query58 287 262 253 253
query59 2143 2139 1987 1987
query60 295 241 216 216
query61 145 142 145 142
query62 391 340 290 290
query63 190 165 172 165
query64 2758 921 784 784
query65 3285 3253 3209 3209
query66 1771 413 301 301
query67 20077 20155 20496 20155
query68 2934 1053 642 642
query69 386 280 248 248
query70 873 785 793 785
query71 289 232 212 212
query72 2679 2802 2456 2456
query73 517 533 298 298
query74 4614 4459 4333 4333
query75 2345 2309 1958 1958
query76 2303 1018 630 630
query77 354 374 285 285
query78 9641 9687 9122 9122
query79 862 906 537 537
query80 554 444 367 367
query81 540 327 274 274
query82 599 146 128 128
query83 318 222 183 183
query84 313 133 114 114
query85 962 430 374 374
query86 336 234 228 228
query87 1985 1964 1798 1798
query88 3648 2737 2715 2715
query89 360 293 264 264
query90 1830 182 182 182
query91 162 153 129 129
query92 100 90 92 90
query93 1024 1004 583 583
query94 441 285 280 280
query95 584 348 384 348
query96 664 529 232 232
query97 2473 2429 2340 2340
query98 161 154 152 152
query99 783 762 650 650
Total cold run time: 233630 ms
Total hot run time: 152669 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4291993215
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -167,9 +287,369 @@ GenericRow tableRow(InternalRow columnValues) {
// from an omitted field and retains Paimon's writer-side semantics.
row.setField(tableFieldIndexes[i], value);
}
+ if (padsRow(row)) {
+ padRow(row);
+ }
return row;
}
+ /**
+ * Whether to pad {@code row}: whether it inserts, outside an overwrite. 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#writeAndReturn}), after filling a NULL
+ * with the field's default and ignoring case: '+I', '+i', or NULL under DEFAULT '+I' inserts, while a '-D', '-U'
+ * or '+U' row carries a key to match, as an UPDATE or DELETE row does. A value Paimon cannot decode, such as NULL
+ * without a default, fails the write in Paimon with the same error, after Paimon's own NOT NULL check, so such a
+ * row is left as it is. An overwrite replaces the partitions it writes, so none of its rows matches a stored
+ * key, and all of them are padded, like the static partition value FE pads into the overwrite filter: a '+U'
+ * row of INSERT OVERWRITE ... PARTITION (p = 'x') has to land in 'x ', and a '+I' and a '-D' of one key in the
+ * statement still cancel out.
+ */
+ private boolean padsRow(GenericRow row) {
+ if (row.getRowKind() != RowKind.INSERT) {
+ return false;
+ }
+ if (overwrite || rowKindGenerator == null) {
+ return true;
+ }
+ try {
+ return rowKindGenerator.generate(defaultValueRow == null ? row : defaultValueRow.replaceRow(row))
+ == RowKind.INSERT;
+ } catch (RuntimeException e) {
+ return false;
+ }
+ }
+
+ private void padRow(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());
+ MapKeyFormat keyFormat = keyPadder == null ? null : new MapKeyFormat(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ return new PaddedMap(
+ keyPadder == null ? map.keyArray() : padMapKeys(map.keyArray(), keyGetter, keyPadder,
+ keyFormat, 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.
+ * Keys are equal as Paimon compares them: a map nested in a key is compared regardless of the order of its
+ * entries, and a NaN regardless of its bits. Keys are compared and printed through {@code keyFormat}, never by
+ * their own hashCode, equals or toString: a key of a nested type holds the columnar rows and arrays of the Arrow
+ * batch, which support none of them (ColumnarArray keeps identity equality), so hashing the key of a
+ * MAP<ROW<c CHAR(2), r ROW<i INT>>, INT> itself would fail even a map of one entry.
+ */
+ private static InternalArray padMapKeys(InternalArray keys, InternalArray.ElementGetter keyGetter,
+ CharPadder keyPadder, MapKeyFormat keyFormat, String column) {
+ Object[] padded = new Object[keys.size()];
+ // Bytes of a padded key -> position of the first key padded into it.
+ Map<BinaryRow, Integer> firstKeyOfPadded = new HashMap<>();
+ for (int i = 0; i < padded.length; i++) {
+ Object key = keyGetter.getElementOrNull(keys, i);
+ padded[i] = key == null ? null : keyPadder.pad(key);
+ Integer first = firstKeyOfPadded.putIfAbsent(keyFormat.bytes(padded[i]), i);
+ if (first == null) {
+ continue;
+ }
+ Object firstKey = keyGetter.getElementOrNull(keys, first);
+ if (!keyFormat.bytes(firstKey).equals(keyFormat.bytes(key))) {
+ throw new IllegalArgumentException(String.format(
+ "Padding the CHAR keys of a map in column '%s' of a Paimon table to the CHAR length turns "
+ + "the keys '%s' and '%s' into the same key '%s', and a map holds one entry per key. "
+ + "Make the keys distinct, or set enable_paimon_write_char_padding = false",
+ column, keyFormat.text(firstKey), keyFormat.text(key), keyFormat.text(padded[i])));
+ }
+ }
+ return new GenericArray(padded);
+ }
+
+ /**
+ * Returns the normalizer of a value of {@code type}, or null when the type holds no MAP, FLOAT or DOUBLE. The
+ * normalizer copies the value with each NaN in it, at any depth, made the canonical NaN, and the entries of each
+ * MAP in it sorted by the bytes of their keys: two values that Paimon takes as equal, though their NaNs differ in
+ * their bits or their maps hold the same entries in another order, then have the same bytes. A NaN in a ROW is
+ * serialized with its bits, while Paimon compares a FLOAT or DOUBLE as {@code Double.equals} does, taking every
+ * NaN as one value; -0.0 and 0.0 stay apart, as they are for Paimon.
+ */
+ private static Normalizer normalizer(DataType type) {
+ switch (type.getTypeRoot()) {
+ case FLOAT:
+ return value -> Float.isNaN((Float) value) ? Float.NaN : value;
+ case DOUBLE:
+ return value -> Double.isNaN((Double) value) ? Double.NaN : value;
+ case ARRAY: {
+ DataType elementType = ((ArrayType) type).getElementType();
+ Normalizer elementNormalizer = normalizer(elementType);
+ if (elementNormalizer == null) {
+ return null;
+ }
+ InternalArray.ElementGetter elementGetter = InternalArray.createElementGetter(elementType.nullable());
+ return value -> new GenericArray(
+ normalizeElements((InternalArray) value, elementGetter, elementNormalizer));
+ }
+ case MAP: {
+ MapType mapType = (MapType) type;
+ Normalizer keyNormalizer = normalizer(mapType.getKeyType());
+ Normalizer valueNormalizer = normalizer(mapType.getValueType());
+ InternalArray.ElementGetter keyGetter =
+ InternalArray.createElementGetter(mapType.getKeyType().nullable());
+ InternalArray.ElementGetter valueGetter =
+ InternalArray.createElementGetter(mapType.getValueType().nullable());
+ InternalRowSerializer keySerializer = new InternalRowSerializer(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ Object[] keys = normalizeElements(map.keyArray(), keyGetter, keyNormalizer);
+ Object[] values = normalizeElements(map.valueArray(), valueGetter, valueNormalizer);
+ byte[][] keyBytes = new byte[keys.length][];
+ Integer[] order = new Integer[keys.length];
+ for (int i = 0; i < keys.length; i++) {
+ // Copied first: the serializer reuses its buffer, and toBytes returns that buffer itself when
+ // the row fills it.
+ keyBytes[i] = keySerializer.toBinaryRow(GenericRow.of(keys[i])).copy().toBytes();
+ order[i] = i;
+ }
+ Arrays.sort(order, (left, right) -> compareBytes(keyBytes[left], keyBytes[right]));
+ Object[] sortedKeys = new Object[keys.length];
Review Comment:
[P2] Reduce duplicate descendant keys before comparing padded map keys
For an Avro `MAP<ROW<c CHAR(2), m MAP<DOUBLE,INT>>,INT>`, insert outer keys `ROW('k',map(CAST(pos AS DOUBLE),1,CAST(neg AS DOUBLE),2)) -> 10` and `ROW('k ',map(CAST(pos AS DOUBLE),2)) -> 20`, reading VARCHAR `pos='NaN', neg='-NaN'` at run time. Doris's `map()` deduplicates serialized bytes, so the signed NaNs remain separate through Arrow. This normalizer canonicalizes their bits but keeps both entries, so the padded keys still compare differently (two inner entries versus one). [Paimon's Avro reader](https://github.com/apache/paimon/blob/release-1.4.2/paimon-format/src/main/java/org/apache/paimon/format/avro/FieldReaderFactory.java#L452-L483) reduces the first inner map to `{NaN:2}`, then considers the outer keys equal and drops value 10. With padding off their CHAR fields differ and both outer values survive. This differs from the existing scalar-NaN and entry-order threads: equal descendant keys need last-wins reduction in the comparison copy before sorting/serialization, or eq
uivalent collision rejection. Cover this two-entry versus one-entry case through Arrow and Avro.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonWritePlanProvider.java:
##########
@@ -409,16 +428,17 @@ private List<Integer> appendRouteFields(List<String> fieldNames,
return indexes;
}
- private boolean supportsNativeRouting(DataTypeRoot type) {
+ private boolean supportsNativeRouting(DataTypeRoot type, boolean charPadding) {
switch (type) {
+ case CHAR:
+ return !charPadding;
Review Comment:
[P2] Preserve native routing for unpadded ORC CHAR bucket keys
For an unpartitioned ORC table `(c CHAR(3), v INT)` with primary key `c`, `bucket=4` and no key default, `PaimonWriteSchema.create` installs no padders: ORC's nonpartition fields are exempt. The new default-on flag nevertheless rejects its CHAR bucket key here, making `fixedBucketDistribution` return null and `requiresSingleWriter` select GATHER. A distributed `INSERT ... SELECT` therefore goes through one sink instance instead of the existing parallel bucket writers, even though none of its routing bytes changes. This differs from the earlier routing thread, whose padded fields actually changed bucket identity. Make routing eligibility follow the writer's per-field padding eligibility, preserving native routing for this unchanged ORC case, and add an ORC padding-on distribution test.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4288929154
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -167,9 +287,236 @@ GenericRow tableRow(InternalRow columnValues) {
// from an omitted field and retains Paimon's writer-side semantics.
row.setField(tableFieldIndexes[i], value);
}
+ if (padsRow(row)) {
+ padRow(row);
+ }
return row;
}
+ /**
+ * Whether to pad {@code row}: whether it inserts, outside an overwrite. 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#writeAndReturn}), after filling a NULL
+ * with the field's default and ignoring case: '+I', '+i', or NULL under DEFAULT '+I' inserts, while a '-D', '-U'
+ * or '+U' row carries a key to match, as an UPDATE or DELETE row does. A value Paimon cannot decode, such as NULL
+ * without a default, fails the write in Paimon with the same error, after Paimon's own NOT NULL check, so such a
+ * row is left as it is. An overwrite replaces the partitions it writes, so none of its rows matches a stored
+ * key, and all of them are padded, like the static partition value FE pads into the overwrite filter: a '+U'
+ * row of INSERT OVERWRITE ... PARTITION (p = 'x') has to land in 'x ', and a '+I' and a '-D' of one key in the
+ * statement still cancel out.
+ */
+ private boolean padsRow(GenericRow row) {
+ if (row.getRowKind() != RowKind.INSERT) {
+ return false;
+ }
+ if (overwrite || rowKindGenerator == null) {
+ return true;
+ }
+ try {
+ return rowKindGenerator.generate(defaultValueRow == null ? row : defaultValueRow.replaceRow(row))
+ == RowKind.INSERT;
+ } catch (RuntimeException e) {
+ return false;
+ }
+ }
+
+ private void padRow(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());
+ MapKeyFormat keyFormat = keyPadder == null ? null : new MapKeyFormat(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ return new PaddedMap(
+ keyPadder == null ? map.keyArray() : padMapKeys(map.keyArray(), keyGetter, keyPadder,
+ keyFormat, 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.
+ * Keys are compared and printed through {@code keyFormat}, never by their own hashCode, equals or toString: a key
+ * of a nested type holds the columnar rows and arrays of the Arrow batch, which support none of them
+ * (ColumnarArray keeps identity equality), so hashing the key of a MAP<ROW<c CHAR(2), r ROW<i INT>>, INT> itself
+ * would fail even a map of one entry.
+ */
+ private static InternalArray padMapKeys(InternalArray keys, InternalArray.ElementGetter keyGetter,
+ CharPadder keyPadder, MapKeyFormat keyFormat, String column) {
+ Object[] padded = new Object[keys.size()];
+ // Bytes of a padded key -> position of the first key padded into it.
+ Map<BinaryRow, Integer> firstKeyOfPadded = new HashMap<>();
+ for (int i = 0; i < padded.length; i++) {
+ Object key = keyGetter.getElementOrNull(keys, i);
+ padded[i] = key == null ? null : keyPadder.pad(key);
+ Integer first = firstKeyOfPadded.putIfAbsent(keyFormat.bytes(padded[i]), i);
+ if (first == null) {
+ continue;
+ }
+ Object firstKey = keyGetter.getElementOrNull(keys, first);
+ if (!keyFormat.bytes(firstKey).equals(keyFormat.bytes(key))) {
+ throw new IllegalArgumentException(String.format(
+ "Padding the CHAR keys of a map in column '%s' of a Paimon table to the CHAR length turns "
+ + "the keys '%s' and '%s' into the same key '%s', and a map holds one entry per key. "
+ + "Make the keys distinct, or set enable_paimon_write_char_padding = false",
+ column, keyFormat.text(firstKey), keyFormat.text(key), keyFormat.text(padded[i])));
+ }
+ }
+ return new GenericArray(padded);
+ }
+
+ /** Compares the keys of a map by their bytes and prints them as Paimon casts them to STRING, for any key type. */
+ private static final class MapKeyFormat {
+ private final InternalRowSerializer serializer;
+ private final CastExecutor<Object, Object> toText;
+
+ @SuppressWarnings("unchecked")
+ private MapKeyFormat(DataType keyType) {
+ this.serializer = new InternalRowSerializer(keyType);
+ this.toText = (CastExecutor<Object, Object>) CastExecutors.resolveToString(keyType);
+ }
+
+ /** The bytes of {@code key} as the only field of a row, copied since the serializer reuses its buffer. */
+ private BinaryRow bytes(Object key) {
+ return serializer.toBinaryRow(GenericRow.of(key)).copy();
Review Comment:
Fixed in `359271837c1`. This reproduces as described. In an Avro table created through the Paimon API with `m MAP<ROW<c CHAR(2), m MAP<STRING, INT>>, INT>`, inserting `map(named_struct('c', 'k', 'm', map('a', 1, 'b', 2)), 10, named_struct('c', 'k ', 'm', map('b', 2, 'a', 1)), 20)` succeeded, and Doris (native and JNI reads) and the Paimon API all read back the single entry `{"c":"k ", "m":{"a":1, "b":2}}: 20`. A Parquet table kept both entries, that is, two keys that Paimon takes as equal. With padding off both keys are kept, as before this PR, and the same pair with the inner maps in the same order already failed with the intended error.
The check now sorts the entries of every map nested in a key, at any depth (in a ROW, an ARRAY or a MAP), by their key bytes before it serializes the key, so two keys have the same bytes when Paimon's `GenericMap.equals` takes their maps as equal. Both tables now fail that write with `turns the keys '{k, {a -> 1, b -> 2}}' and '{k , {b -> 2, a -> 1}}' into the same key '{k , {b -> 2, a -> 1}}'`. Keys that were one key before padding, such as `('k ', {a: 1, b: 2})` and `('k ', {b: 2, a: 1})`, are still written as they are; an Avro read keeps one of them with padding on or off, as on master.
Tests: `PaimonWriteSchemaTest.testMapsNestedInMapKeysAreComparedRegardlessOfTheirEntryOrder` feeds rows read back from an Arrow batch: the reversed inner map collides, directly and inside an array, keys equal before padding are kept, and keys whose inner maps differ are padded and kept. Not sorting the entries, or sorting them by bytes left in the serializer's reused buffer, turns it red. `test_paimon_write_char_padding` now creates such an Avro table through Spark, fails the reversed pair and pads a single key holding a map; with the paimon-scanner jar from before this commit the suite fails at that write.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on code in PR #69083:
URL: https://github.com/apache/doris/pull/69083#discussion_r4299008914
##########
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:
A correction to the second paragraph of my reply above. With `1012317776a` this table is no longer written by one writer: its CHAR partition column is the `rowkind.field`, which the writer never pads, so it is routed natively. And since #69010, fe-core runs the transaction of an external `INSERT OVERWRITE` even when planning proves its source empty. On a cluster with both, `INSERT OVERWRITE TABLE t PARTITION (rk = '+I') SELECT 3 LIMIT 0` now commits an OVERWRITE snapshot and clears the partition, as a source that is empty at run time does; the regression still uses the latter.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonWriteSchema.java:
##########
@@ -167,9 +287,369 @@ GenericRow tableRow(InternalRow columnValues) {
// from an omitted field and retains Paimon's writer-side semantics.
row.setField(tableFieldIndexes[i], value);
}
+ if (padsRow(row)) {
+ padRow(row);
+ }
return row;
}
+ /**
+ * Whether to pad {@code row}: whether it inserts, outside an overwrite. 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#writeAndReturn}), after filling a NULL
+ * with the field's default and ignoring case: '+I', '+i', or NULL under DEFAULT '+I' inserts, while a '-D', '-U'
+ * or '+U' row carries a key to match, as an UPDATE or DELETE row does. A value Paimon cannot decode, such as NULL
+ * without a default, fails the write in Paimon with the same error, after Paimon's own NOT NULL check, so such a
+ * row is left as it is. An overwrite replaces the partitions it writes, so none of its rows matches a stored
+ * key, and all of them are padded, like the static partition value FE pads into the overwrite filter: a '+U'
+ * row of INSERT OVERWRITE ... PARTITION (p = 'x') has to land in 'x ', and a '+I' and a '-D' of one key in the
+ * statement still cancel out.
+ */
+ private boolean padsRow(GenericRow row) {
+ if (row.getRowKind() != RowKind.INSERT) {
+ return false;
+ }
+ if (overwrite || rowKindGenerator == null) {
+ return true;
+ }
+ try {
+ return rowKindGenerator.generate(defaultValueRow == null ? row : defaultValueRow.replaceRow(row))
+ == RowKind.INSERT;
+ } catch (RuntimeException e) {
+ return false;
+ }
+ }
+
+ private void padRow(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());
+ MapKeyFormat keyFormat = keyPadder == null ? null : new MapKeyFormat(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ return new PaddedMap(
+ keyPadder == null ? map.keyArray() : padMapKeys(map.keyArray(), keyGetter, keyPadder,
+ keyFormat, 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.
+ * Keys are equal as Paimon compares them: a map nested in a key is compared regardless of the order of its
+ * entries, and a NaN regardless of its bits. Keys are compared and printed through {@code keyFormat}, never by
+ * their own hashCode, equals or toString: a key of a nested type holds the columnar rows and arrays of the Arrow
+ * batch, which support none of them (ColumnarArray keeps identity equality), so hashing the key of a
+ * MAP<ROW<c CHAR(2), r ROW<i INT>>, INT> itself would fail even a map of one entry.
+ */
+ private static InternalArray padMapKeys(InternalArray keys, InternalArray.ElementGetter keyGetter,
+ CharPadder keyPadder, MapKeyFormat keyFormat, String column) {
+ Object[] padded = new Object[keys.size()];
+ // Bytes of a padded key -> position of the first key padded into it.
+ Map<BinaryRow, Integer> firstKeyOfPadded = new HashMap<>();
+ for (int i = 0; i < padded.length; i++) {
+ Object key = keyGetter.getElementOrNull(keys, i);
+ padded[i] = key == null ? null : keyPadder.pad(key);
+ Integer first = firstKeyOfPadded.putIfAbsent(keyFormat.bytes(padded[i]), i);
+ if (first == null) {
+ continue;
+ }
+ Object firstKey = keyGetter.getElementOrNull(keys, first);
+ if (!keyFormat.bytes(firstKey).equals(keyFormat.bytes(key))) {
+ throw new IllegalArgumentException(String.format(
+ "Padding the CHAR keys of a map in column '%s' of a Paimon table to the CHAR length turns "
+ + "the keys '%s' and '%s' into the same key '%s', and a map holds one entry per key. "
+ + "Make the keys distinct, or set enable_paimon_write_char_padding = false",
+ column, keyFormat.text(firstKey), keyFormat.text(key), keyFormat.text(padded[i])));
+ }
+ }
+ return new GenericArray(padded);
+ }
+
+ /**
+ * Returns the normalizer of a value of {@code type}, or null when the type holds no MAP, FLOAT or DOUBLE. The
+ * normalizer copies the value with each NaN in it, at any depth, made the canonical NaN, and the entries of each
+ * MAP in it sorted by the bytes of their keys: two values that Paimon takes as equal, though their NaNs differ in
+ * their bits or their maps hold the same entries in another order, then have the same bytes. A NaN in a ROW is
+ * serialized with its bits, while Paimon compares a FLOAT or DOUBLE as {@code Double.equals} does, taking every
+ * NaN as one value; -0.0 and 0.0 stay apart, as they are for Paimon.
+ */
+ private static Normalizer normalizer(DataType type) {
+ switch (type.getTypeRoot()) {
+ case FLOAT:
+ return value -> Float.isNaN((Float) value) ? Float.NaN : value;
+ case DOUBLE:
+ return value -> Double.isNaN((Double) value) ? Double.NaN : value;
+ case ARRAY: {
+ DataType elementType = ((ArrayType) type).getElementType();
+ Normalizer elementNormalizer = normalizer(elementType);
+ if (elementNormalizer == null) {
+ return null;
+ }
+ InternalArray.ElementGetter elementGetter = InternalArray.createElementGetter(elementType.nullable());
+ return value -> new GenericArray(
+ normalizeElements((InternalArray) value, elementGetter, elementNormalizer));
+ }
+ case MAP: {
+ MapType mapType = (MapType) type;
+ Normalizer keyNormalizer = normalizer(mapType.getKeyType());
+ Normalizer valueNormalizer = normalizer(mapType.getValueType());
+ InternalArray.ElementGetter keyGetter =
+ InternalArray.createElementGetter(mapType.getKeyType().nullable());
+ InternalArray.ElementGetter valueGetter =
+ InternalArray.createElementGetter(mapType.getValueType().nullable());
+ InternalRowSerializer keySerializer = new InternalRowSerializer(mapType.getKeyType());
+ return value -> {
+ InternalMap map = (InternalMap) value;
+ Object[] keys = normalizeElements(map.keyArray(), keyGetter, keyNormalizer);
+ Object[] values = normalizeElements(map.valueArray(), valueGetter, valueNormalizer);
+ byte[][] keyBytes = new byte[keys.length][];
+ Integer[] order = new Integer[keys.length];
+ for (int i = 0; i < keys.length; i++) {
+ // Copied first: the serializer reuses its buffer, and toBytes returns that buffer itself when
+ // the row fills it.
+ keyBytes[i] = keySerializer.toBinaryRow(GenericRow.of(keys[i])).copy().toBytes();
+ order[i] = i;
+ }
+ Arrays.sort(order, (left, right) -> compareBytes(keyBytes[left], keyBytes[right]));
+ Object[] sortedKeys = new Object[keys.length];
Review Comment:
Fixed in `41d351b3b90`. This reproduces as described. In an Avro table created through the Paimon API with `m MAP<ROW<c CHAR(2), mm MAP<DOUBLE, INT>>, INT>`, `INSERT ... SELECT 1, map(named_struct('c', 'k', 'mm', map(cast(pos AS DOUBLE), 1, cast(neg AS DOUBLE), 2)), 10, named_struct('c', 'k ', 'mm', map(cast(pos AS DOUBLE), 2)), 20)` from a Doris table holding `'NaN'` and `'-NaN'` succeeded, and Doris read back the single entry `{"c":"k ", "mm":{NaN:2}}: 20`; a Parquet table kept both entries, two keys that Paimon takes as equal. With padding off both entries are kept, as before this PR.
The copy of a nested map that the check compares now keeps, once its NaNs are canonical and its entries sorted by their key bytes, only the last of the entries whose keys are equal, the one whose value Paimon's Avro reader keeps (`ArrayMapReader` puts the entries into a `HashMap` in order). Both tables now fail that write with `turns the keys '{k, {NaN -> 1, NaN -> 2}}' and '{k , {NaN -> 2}}' into the same key '{k , {NaN -> 2}}'`. Keys whose nested maps keep other last values, such as `(k, {NaN: 1, -NaN: 2})` and `(k , {NaN: 1})`, are padded and written, and read back as two entries; the nested maps are written whole, as before.
Tests: `PaimonWriteSchemaTest.testMapsNestedInMapKeysAreComparedByTheLastEntryOfEachKey` feeds rows read back from an Arrow batch: the pair above collides, keys that were one key before padding are kept, and keys whose nested maps keep other last values are padded, with their nested maps whole. Not reducing the nested entries, keeping the first of them instead of the last, or comparing the nested maps by their keys alone turns it red. `test_paimon_write_char_padding` creates such an Avro table through Spark, fails the pair cast at run time from a Doris table and pads the other pair; with the paimon-scanner jar from before this commit the suite fails at that write.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonWritePlanProvider.java:
##########
@@ -409,16 +428,17 @@ private List<Integer> appendRouteFields(List<String> fieldNames,
return indexes;
}
- private boolean supportsNativeRouting(DataTypeRoot type) {
+ private boolean supportsNativeRouting(DataTypeRoot type, boolean charPadding) {
switch (type) {
+ case CHAR:
+ return !charPadding;
Review Comment:
Fixed in `1012317776a`. Confirmed: for a primary-key ORC table `(c CHAR(3), v INT)` with `bucket = 4`, `EXPLAIN SHAPE PLAN INSERT ... SELECT ... FROM numbers("number" = "100")` planned `DistributionSpecGather` with padding on and `DistributionSpecExternalTableSinkHashPartitioned` with it off, though the writer leaves that key unpadded.
FE now decides it per routing field by the writer's rule, in `PaimonWriteBinding.charPaddedFields`: with padding on, the writer pads every CHAR field but the table's `rowkind.field`, and of a table that writes ORC files, by `file.format` or at some level of `file.format.per.level`, only the partition fields. A CHAR routing field it pads still falls back, as before, and the others are routed natively. The static partition values follow the same rule, which leaves them as they were. The remark in r4279897764 that FE does not look at the file format no longer holds. With padding on, the ORC table above and one with `file.format.per.level = '5:orc'` now plan `DistributionSpecExternalTableSinkHashPartitioned`, while a Parquet table and an ORC table partitioned by a CHAR field still plan `DistributionSpecGather`. Writing the ORC table with `changelog-producer = full-compaction` from `numbers()` with four writers commits an APPEND and a COMPACT snapshot with 100 changelog records, with n
o LSM conflict, and a DELETE and an UPDATE then reach its keys.
Tests: `PaimonWritePlanProviderTest.charRouteFieldsThatTheWriterLeavesUnpaddedAreRoutedNatively` routes, with padding on, the CHAR key of an ORC table, by `file.format` and by `file.format.per.level`, and a CHAR partition that is the `rowkind.field` natively, and the CHAR partition of an ORC table through one writer; `PaimonWriteBindingTest.staticCharPartitionOfAnOrcTableIsPaddedAsTheRowsAre` keeps the static value of that partition padded. Falling back for every CHAR routing field, ignoring `file.format.per.level` or the `rowkind.field`, or routing the partition of an ORC table natively and leaving it unpadded turns them red. `test_paimon_write_char_padding` checks the plan of such a full-compaction ORC table, next to the Parquet one's, and its `$snapshots` after a four-writer write; with the fe-connector-paimon jar from before this commit the suite fails at that plan.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6224054317
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6224053818
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6224555363
<details>
<summary>TPC-H: <b>Total hot run time: 29130 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 1012317776ab401cbf6f76ca82fd72a8431dd49b, data reload: false
------ Round 1 ----------------------------------
============================================
q1 17578 5609 5480 5480
q2 2086 327 246 246
q3 10258 1380 830 830
q4 4678 467 348 348
q5 7426 778 527 527
q6 187 187 156 156
q7 744 791 614 614
q8 9240 1263 1157 1157
q9 6223 4246 4252 4246
q10 6819 1288 1036 1036
q11 435 252 235 235
q12 644 400 281 281
q13 18088 3564 2983 2983
q14 273 273 253 253
q15 q16 726 726 672 672
q17 1192 739 584 584
q18 7060 6207 6164 6164
q19 1113 969 608 608
q20 402 325 223 223
q21 4818 2224 2354 2224
q22 355 306 263 263
Total cold run time: 100345 ms
Total hot run time: 29130 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1 6131 6110 6107 6107
q2 659 555 540 540
q3 5238 5383 4874 4874
q4 1992 2105 1514 1514
q5 5270 5131 5941 5131
q6 258 203 152 152
q7 2255 2054 1870 1870
q8 3146 2732 2665 2665
q9 8197 8058 8161 8058
q10 3804 3677 3287 3287
q11 614 439 433 433
q12 669 713 512 512
q13 3585 3886 3206 3206
q14 315 329 298 298
q15 q16 744 737 660 660
q17 8428 7553 7060 7060
q18 13149 12207 13083 12207
q19 917 801 793 793
q20 2172 2157 1935 1935
q21 5727 4666 4760 4666
q22 502 466 428 428
Total cold run time: 73772 ms
Total hot run time: 66396 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6174486904
<details>
<summary>TPC-DS: <b>Total hot run time: 152089 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit f6b658b9ee7ab5479e82110fdb85b63376e8a9ab, data reload: false
query5 4289 585 426 426
query6 454 204 194 194
query7 4806 445 230 230
query8 314 205 149 149
query9 8749 3973 4005 3973
query10 448 315 257 257
query11 5985 3787 3511 3511
query12 142 90 86 86
query13 1260 428 314 314
query14 6029 4042 3757 3757
query14_1 3525 3498 3467 3467
query15 206 201 182 182
query16 943 448 404 404
query17 845 671 523 523
query18 2399 411 293 293
query19 186 160 129 129
query20 85 81 78 78
query21 204 133 111 111
query22 12964 12939 12805 12805
query23 13577 13017 12600 12600
query23_1 12535 12644 12529 12529
query24 7087 841 468 468
query24_1 468 465 466 465
query25 499 393 338 338
query26 1268 253 137 137
query27 2772 440 264 264
query28 4644 1966 1962 1962
query29 1508 581 434 434
query30 298 222 182 182
query31 1019 920 813 813
query32 132 84 86 84
query33 504 289 233 233
query34 989 855 498 498
query35 752 757 637 637
query36 817 807 736 736
query37 126 103 93 93
query38 1808 1727 1660 1660
query39 825 804 742 742
query39_1 733 740 755 740
query40 214 111 94 94
query41 64 66 61 61
query42 85 84 78 78
query43 361 381 336 336
query44 1295 690 697 690
query45 179 174 172 172
query46 839 939 567 567
query47 3054 2997 2912 2912
query48 302 302 213 213
query49 561 386 298 298
query50 685 281 222 222
query51 10685 10628 10524 10524
query52 79 79 67 67
query53 183 207 154 154
query54 246 189 183 183
query55 69 65 70 65
query56 234 220 211 211
query57 1604 1591 1548 1548
query58 281 258 256 256
query59 2105 2151 1935 1935
query60 295 235 226 226
query61 143 145 142 142
query62 391 341 282 282
query63 198 162 167 162
query64 2709 941 866 866
query65 3276 3243 3230 3230
query66 1746 420 299 299
query67 20600 20372 20052 20052
query68 3361 988 611 611
query69 402 293 247 247
query70 848 788 785 785
query71 288 224 199 199
query72 2835 2129 2401 2129
query73 526 521 297 297
query74 4613 4472 4289 4289
query75 2341 2288 1944 1944
query76 2387 1009 592 592
query77 344 398 274 274
query78 9598 9573 9057 9057
query79 896 850 547 547
query80 491 402 338 338
query81 506 313 263 263
query82 213 138 107 107
query83 190 196 184 184
query84 290 122 102 102
query85 766 412 379 379
query86 300 258 235 235
query87 1972 1978 1827 1827
query88 3580 2711 2687 2687
query89 325 291 266 266
query90 2089 185 178 178
query91 168 154 125 125
query92 101 85 93 85
query93 1059 998 579 579
query94 460 316 282 282
query95 607 418 325 325
query96 670 526 233 233
query97 2453 2440 2343 2343
query98 156 149 145 145
query99 769 763 663 663
Total cold run time: 234040 ms
Total hot run time: 152089 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6187335529
<details>
<summary>TPC-H: <b>Total hot run time: 29696 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit f2c45d7e38c5514bc47dad67b822cdb1c6c044d6, data reload: false
------ Round 1 ----------------------------------
============================================
q1 17627 5680 5573 5573
q2 2083 319 295 295
q3 10212 1436 850 850
q4 4683 466 355 355
q5 7460 825 555 555
q6 185 185 160 160
q7 775 802 614 614
q8 9255 1335 1136 1136
q9 5560 4296 4279 4279
q10 6800 1287 994 994
q11 430 275 238 238
q12 635 401 308 308
q13 18072 3537 3032 3032
q14 269 279 245 245
q15 q16 731 723 666 666
q17 1186 762 581 581
q18 6996 6247 6176 6176
q19 1116 979 619 619
q20 383 340 225 225
q21 5354 2493 2662 2493
q22 389 365 302 302
Total cold run time: 100201 ms
Total hot run time: 29696 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1 6552 6488 6515 6488
q2 750 602 571 571
q3 5602 5797 5185 5185
q4 2324 2197 1626 1626
q5 5861 5600 5638 5600
q6 264 188 149 149
q7 2420 1997 1788 1788
q8 3125 2736 2733 2733
q9 8175 7601 7543 7543
q10 3658 3616 3225 3225
q11 624 425 404 404
q12 669 683 514 514
q13 3316 3651 3014 3014
q14 284 296 271 271
q15 q16 674 696 625 625
q17 8106 6998 6987 6987
q18 13232 12250 13116 12250
q19 929 807 792 792
q20 2195 2144 1934 1934
q21 5901 4773 5023 4773
q22 517 448 412 412
Total cold run time: 75178 ms
Total hot run time: 66884 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6178071393
<details>
<summary>ClickBench: <b>Total hot run time: 24.53 s</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit b58434116f09907b66d4f8375cd67a928aea2623, data reload: false
query1 0.01 0.01 0.00
query2 0.10 0.05 0.04
query3 0.27 0.12 0.12
query4 1.61 0.13 0.13
query5 0.25 0.22 0.24
query6 1.19 0.68 0.68
query7 0.04 0.01 0.00
query8 0.05 0.04 0.04
query9 0.41 0.35 0.34
query10 0.60 0.57 0.57
query11 0.21 0.14 0.14
query12 0.17 0.14 0.15
query13 0.49 0.51 0.50
query14 1.02 1.02 1.01
query15 0.63 0.61 0.61
query16 0.34 0.36 0.35
query17 1.16 1.15 1.10
query18 0.23 0.21 0.21
query19 2.09 1.99 1.97
query20 0.02 0.01 0.02
query21 15.51 0.26 0.14
query22 4.48 0.06 0.05
query23 16.27 0.32 0.12
query24 3.19 0.62 0.39
query25 0.11 0.06 0.05
query26 1.28 0.26 0.16
query27 0.05 0.04 0.03
query28 2.06 0.65 0.42
query29 12.55 4.19 3.36
query30 0.30 0.14 0.12
query31 2.78 0.56 0.34
query32 3.22 0.60 0.50
query33 3.16 3.22 3.30
query34 15.57 4.02 3.36
query35 3.34 3.34 3.32
query36 0.55 0.42 0.45
query37 0.10 0.06 0.06
query38 0.05 0.04 0.03
query39 0.04 0.03 0.03
query40 0.17 0.14 0.15
query41 0.09 0.03 0.03
query42 0.04 0.02 0.02
query43 0.04 0.04 0.04
Total cold run time: 95.84 s
Total hot run time: 24.53 s
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6180814425
# FE Regression Coverage Report
Increment line coverage ` 100.00% (1/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69083_45e6e5d2eff67184390f37b61fbe0319ec1a9447_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69083_45e6e5d2eff67184390f37b61fbe0319ec1a9447_merge_fe/report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "morningman (via GitHub)" <gi...@apache.org>.
morningman commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6195857284
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69083:
URL: https://github.com/apache/doris/pull/69083#issuecomment-6198588175
<details>
<summary>TPC-H: <b>Total hot run time: 29234 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 1740cfa4637dea43b1143058633d5ffcc3e9348f, data reload: false
------ Round 1 ----------------------------------
============================================
q1 17600 5656 5548 5548
q2 2099 302 274 274
q3 10239 1369 832 832
q4 4677 471 342 342
q5 7421 791 530 530
q6 190 185 161 161
q7 746 799 621 621
q8 9258 1341 1091 1091
q9 5497 4299 4250 4250
q10 6813 1294 1020 1020
q11 438 263 245 245
q12 638 397 300 300
q13 18058 3480 3012 3012
q14 284 263 242 242
q15 q16 736 735 661 661
q17 1182 746 573 573
q18 7022 6226 6180 6180
q19 1118 966 600 600
q20 370 335 235 235
q21 4924 2238 2368 2238
q22 369 343 279 279
Total cold run time: 99679 ms
Total hot run time: 29234 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1 6130 6324 6143 6143
q2 708 586 540 540
q3 5204 5420 4951 4951
q4 2013 2128 1558 1558
q5 5307 5182 6065 5182
q6 254 210 169 169
q7 2219 2126 1785 1785
q8 3160 2794 2747 2747
q9 8214 8082 7998 7998
q10 3952 3702 3396 3396
q11 660 497 409 409
q12 716 725 530 530
q13 3547 3995 3212 3212
q14 344 343 283 283
q15 q16 706 759 687 687
q17 8397 7526 6963 6963
q18 13177 12269 13121 12269
q19 919 789 787 787
q20 2185 2183 1941 1941
q21 5833 4812 4837 4812
q22 507 489 411 411
Total cold run time: 74152 ms
Total hot run time: 66773 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org