Skip to content

[fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do - #69083

Open
morningman wants to merge 9 commits into
apache:masterfrom
morningman:b2m-t28a-paimon-char-pad
Open

morningman wants to merge 9 commits into
apache:masterfrom
morningman:b2m-t28a-paimon-char-pad

Conversation

@morningman

@morningman morningman commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

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 (in a table that writes ORC files only the partition values, since ORC pads the rest itself), 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 whose distinct keys padding would make one key fails the row: a map holds one entry per key, Paimon's Avro reader keeps only one of them, and Spark rejects such a map too (DUPLICATED_MAP_KEY). Keys are compared by their serialized bytes, and printed as Paimon casts them to STRING, since a key of a nested type arrives in the Arrow batch as a columnar row or array that supports neither hashCode, equals nor toString. Before a key is compared, each NaN in it is made the canonical NaN, and the entries of each map nested in it are sorted by their key bytes, keeping only the last of the entries whose keys are then equal: Paimon takes NaNs of any bits as one value and compares maps regardless of the order of their entries, and its Avro reader keeps the value of the last of the entries with equal keys, while the bytes of a ROW keep the bits of a NaN and Doris's map() keeps NaN and -NaN apart.
  • 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. Nor are the '-D', '-U' and '+U' rows of a table with a rowkind.field padded, outside an overwrite: Paimon takes the kind of a row from that field, and those rows carry a key to match as well. Doris refuses UPDATE and DELETE on such a table, so inserting '-D' rows read back from it is how Doris deletes its rows.
  • MERGE. MERGE matches its source rows to the table, and checks the keys it inserts for duplicates, by the key as Doris holds it, unpadded: 'k' and 'k ' are two keys there, and neither matches a stored 'k '. Padding a shorter key in a row that MERGE inserts would change the key after those checks, so the writer fails such a row when a CHAR primary-key value is shorter than the column, the only case in which padding changes a key. The message says to pad the key in the MERGE with rpad, in the ON condition and in the inserted values, or to turn padding off. A full-length key is written as it is, so MERGE's matching and its duplicate check see the stored key.
  • ORC. Paimon stores CHAR(n) as ORC char(n): ORC pads every CHAR value it writes and strips the trailing spaces from one it reads, and so does Doris's ORC reader. Padding in the writer would change nothing that ORC stores, only the bucket of a CHAR key, and UPDATE and DELETE, which read the key back without its spaces, would hash another bucket and miss the row. So a table that writes ORC files, by file.format or at some level of file.format.per.level, gets only its partition values padded, which are read back as stored, and its keys keep the bucket they had before. A short CHAR key that Spark or Flink wrote padded into a multi-bucket ORC table stays out of reach of Doris's UPDATE and DELETE, as on master.
  • 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. An overwrite replaces the partitions it writes, so none of its rows matches a stored key: all of them are padded, whatever their rowkind.field value, and a '+U' row of INSERT OVERWRITE ... PARTITION (p = 'x') lands in 'x '.
  • Routing. A fixed-bucket table routes each row to the writer that owns its partition and bucket, which BE computes from the row's partition and bucket fields (paimon_fixed_bucket). BE hashes the unpadded value while Paimon buckets the padded row, so with padding on a CHAR partition or bucket field that the writer pads is not routed natively, and the table gets the distribution of a routing field BE cannot hash: one writer for a primary-key table or one that compacts. Otherwise several writers would write and compact one bucket, and Paimon would give up committing the compaction, and with it the changelog of a full-compaction table, after committing the rows. FE decides it per field by the writer's rule (PaimonWriteBinding.charPaddedFields, which the static partition values use too), so a CHAR field that the writer leaves unpadded, the rowkind.field or a field other than a partition field of a table that writes ORC files, is routed natively as before. With padding off the route is unchanged.
  • rowkind.field. The column a table names as rowkind.field is not padded, in the rows or as a static partition value: Paimon decodes the row kind from its value, and a CHAR(3) '+I' padded to '+I ' fails the write. Outside an overwrite, a row of such a table is padded only when Paimon decodes that value as an insert, which the writer decides with Paimon's own DefaultValueRow and RowKindGenerator: '+I' or '+i', or a NULL or omitted value under such a default. A '-D', '-U' or '+U' row keeps its key as written, so with padding on a short key written as a literal does not reach a key that Doris itself wrote padded: after INSERT INTO t VALUES ('m', '+I', 5) stores 'm ', INSERT INTO t VALUES ('m', '-D', 5) deletes nothing, where master deletes the unpadded 'm', and a '+I' and a '-D' of one short key in one INSERT do not cancel out. Such a key has to be written at full length, or read back from the table, as in INSERT INTO t SELECT c, '-D', v FROM t WHERE ....
  • 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; a map whose distinct keys padding makes equal fails, while keys equal before padding are kept; padding off; UPDATE and DELETE rows left alone; the rowkind.field column left alone, and of the rows of such a table only those that Paimon decodes as inserts padded ('+I', '+i', a NULL or omitted value under a '+I' or '+i' default), while every row of an overwrite is padded; an ORC table, by file.format or file.format.per.level, padded in its partition values only; reading the option; a CHAR primary-key value shorter than the column in a row that MERGE inserts fails ('k', 'k ', '中中'), while a full-length or longer key, the rows of UPDATE and DELETE, padding off and a plain INSERT do not; map keys read from an Arrow batch, a ROW holding a ROW padded and colliding ROW, ARRAY and ROW-with-ARRAY keys failing with the keys printed; keys whose nested maps differ only in the order of their entries colliding, directly and inside an array, while keys equal before padding are kept; keys holding NaNs of opposite signs colliding, in a DOUBLE field, a FLOAT field and a nested ROW, while such keys equal before padding are kept and -0.0 stays apart from 0.0; keys whose nested maps are one map once each keeps the last of its entries with equal keys colliding ((k, {NaN: 1, -NaN: 2}) and (k , {NaN: 2})), while keys whose nested maps keep other last values are padded, with their nested maps written whole.
    • PaimonWriteBindingTest.staticCharPartitionIsPaddedAsTheRowsAre, PaimonWriteBindingTest.staticRowKindFieldPartitionIsLeftUnpaddedAsTheRowsAre, PaimonBackendBoundTableTest.writeBindingTellsOnlyTheBackendWhetherToPadCharValues, PaimonWriteBindingTest.staticCharPartitionOfAnOrcTableIsPaddedAsTheRowsAre, PaimonWritePlanProviderTest.charPaddingFollowsTheSessionVariableAndDefaultsToOn, PaimonWritePlanProviderTest.charRouteFieldsFallBackToSingleWriterWhenCharPaddingIsOn (a CHAR primary key or partition key: one writer with padding on, paimon_fixed_bucket with it off) and PaimonWritePlanProviderTest.charRouteFieldsThatTheWriterLeavesUnpaddedAreRoutedNatively (with padding on, the CHAR primary key of an ORC table, by file.format or file.format.per.level, and a CHAR partition key that is the rowkind.field: paimon_fixed_bucket; the CHAR partition key of an ORC table: one writer).
    • 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, ignoring the session variable, routing CHAR fields natively with padding on, padding the rowkind.field column or its static partition value, not failing a short MERGE key, counting its bytes, failing the short keys of UPDATE and DELETE rows too, padding every column of an ORC table, ignoring file.format.per.level, padding by a row's own kind regardless of its rowkind.field, looking that value up in the input only, not filling it with the field's default, padding by the decoded row kind in an overwrite, not failing map keys that padding makes equal, failing keys that were equal before padding, hashing the keys themselves, printing them with toString, comparing keys without sorting the entries of their nested maps, comparing them without making their NaNs the canonical NaN, or only their DOUBLE ones, making -0.0 into 0.0, comparing keys without reducing the entries of their nested maps to one per key, keeping the first of those entries instead of the last, comparing nested maps by their keys alone, falling back for every CHAR routing field while padding is on, ignoring file.format.per.level or the rowkind.field when routing, routing the CHAR partition field of an ORC table natively, and leaving the static partition value of an ORC table unpadded.
    • All 741 tests of fe-connector-paimon and 141 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. More cases, with padding on:

    Case before the fix with this PR
    INSERT ... SELECT ... FROM numbers("number" = "100") with four writers into a CHAR(3) primary-key table with bucket = 4 and changelog-producer = full-compaction every bucket gets files from all four writers; each INSERT returns COMMITTED, but Paimon gives up the compaction (LSM conflicts detected, then File deletion conflicts detected) and the table has only APPEND snapshots, without changelog one writer (DistributionSpecGather); each INSERT commits an APPEND and a COMPACT snapshot with its changelog
    the same INSERT into such a table that writes ORC files, whose keys the writer leaves unpadded one writer (DistributionSpecGather) four writers (DistributionSpecExternalTableSinkHashPartitioned); an APPEND and a COMPACT snapshot with 100 changelog records, and a DELETE and an UPDATE then reach the keys
    INSERT ... VALUES (3, '+I', 'c', 30) into a table whose rowkind.field is rk CHAR(3) fails: Unsupported short string '+I ' for row kind written; a later '-D' row deletes its key
    MERGE INTO t USING s ON t.c = s.c WHEN NOT MATCHED THEN INSERT (c, v) VALUES (s.c, s.v), source ('k', 1) and ('k ', 2), into an empty table keyed by c CHAR(3) commits one row 'k ': the duplicate-insert check is bypassed fails, asking to pad the key with rpad or to turn padding off; with rpad in ON and in the inserted values, the duplicate-insert check rejects the two rows
    the same NOT MATCHED-only MERGE of ('k', 9) into a table holding 'k ' replaces the row 'k ' that it did not match fails as above; with rpad, a MATCHED clause updates 'k '
    INSERT OVERWRITE TABLE t PARTITION (rk = '+I') ... into a table partitioned by its rowkind.field rk CHAR(3), holding two '+I' rows SELECT 3 fails (Trying to overwrite partition {rk=+I }, but the changes in rk=+I/ does not belong to this partition); a source that returns no rows at run time returns OK and leaves both rows SELECT 3 replaces the partition with one row; the empty source clears it
    DELETE ... WHERE v IN (1, 3, 4, 5) of four of six short keys that Doris inserted into an ORC table keyed by c CHAR(3) with bucket = 4 returns COMMITTED and deletes none: the keys went to the buckets of 'k ' and the like, DELETE reads 'k' back and hashes another bucket deletes all four; an UPDATE of the other two updates them in place
    INSERT INTO t SELECT c, '-D', v FROM t WHERE c = 'k' into a table keyed by c CHAR(3) with rowkind.field, holding 'k' (written with padding off) and 'k ' deletes 'k ' and leaves 'k'; for a key 'j' with no padded twin, deletes nothing deletes 'k', and a '-D' read back from 'k ' deletes 'k '
    INSERT ... VALUES (1, map('k', 1, 'k ', 2)) into a MAP<CHAR(2), INT> column of an Avro table written as two 'k ' keys, read back as {"k ":2} fails, naming the column and the two keys; map('k', 1, 'kk', 2) is written padded
    INSERT ... VALUES (1, map(named_struct('c', 'k', 'r', named_struct('i', 1)), 7)) into a MAP<STRUCT<c: CHAR(2), r: STRUCT<i: INT>>, INT> column fails, even for one key: ColumnarRowData do not support hashCode written with the key {k , {1}}; a pair of keys that padding makes equal fails, printing '{k, {1}}' and '{k , {1}}'
    INSERT ... VALUES ('k', '+i', 1), ('j', NULL, 2), then ('k', '+I', 3), ('j', '+I', 4), into a table keyed by c CHAR(3) whose rowkind.field is rk STRING DEFAULT '+I' two rows per key: 'k' and 'j' unpadded, 'k ' and 'j ' padded one row per key, 'k ' and 'j '
    INSERT OVERWRITE TABLE t PARTITION (p = 'x') SELECT 1, '+U', 9 into a table partitioned by p CHAR(3) with a separate rowkind.field fails: Trying to overwrite partition {p=x }, but the changes in p=x/ does not belong to this partition written into 'x '; a '+I' and a '-D' of one key in an overwrite of p = 'y' cancel out
  • 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. It also writes the full-compaction table above from numbers() with parallel_pipeline_task_num = 4 and checks its $snapshots (an APPEND, then a COMPACT with 100 changelog records), and the rowkind.field table. It runs the MERGE cases above against a Doris source table (the failing MERGE writes nothing, the rpad MERGE inserts and then matches 'k ', and with padding off MERGE writes 'm' and 'm ' as two rows), and overwrites the '+I' partition of a table partitioned by its CHAR rowkind.field, once with a row and once with a source that is empty at run time. It deletes and updates Doris-inserted keys of a CHAR-keyed ORC table with bucket = 4, writes a CHAR-keyed full-compaction ORC table with bucket = 4 from numbers() with four writers, checking that it is routed natively (DistributionSpecExternalTableSinkHashPartitioned, where the Parquet table above gets DistributionSpecGather) and that both snapshots are committed, deletes an unpadded and a padded key of a CHAR-keyed rowkind.field table with '-D' rows read back from it, and writes a map whose keys padding makes equal into an Avro table, which fails with padding on and keeps both keys with it off. It also writes a map with a ROW-of-ROW key and fails a pair of such keys that padding makes equal, writes a map with a key holding a map into an Avro table and fails a pair of keys that differ only in the order of that map's entries once padded, does the same with a key holding a DOUBLE and a pair of keys holding NaNs of opposite signs, cast at run time from a Doris table, and with a key holding a map with DOUBLE keys and a pair of keys whose nested maps are one map once each keeps the last of its entries with equal keys, writes '+i' and a NULL under DEFAULT '+I' into a rowkind.field table and then '+I' rows of the same keys (one padded row per key), and overwrites a CHAR partition of a table with a separate rowkind.field with a '+U' row and with a '+I' and a '-D' of one key. 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), with those from before the routing fix it fails at the $snapshots check, with no COMPACT snapshot, with those from before the MERGE fix at the MERGE that must fail, with those from before the ORC, rowkind.field and map fix at the map that must fail, with the paimon-scanner jar from before the commit that compares map keys by bytes at the ROW-of-ROW map key (ColumnarRowData do not support hashCode), with the one from before the commit that sorts nested map entries at the pair of keys whose nested maps differ only in their entry order, with the one from before the commit that compares NaNs regardless of their bits at the pair of keys holding NaNs of opposite signs, and with the one from before the commit that keeps the last entry of each key at the pair of keys whose nested maps keep the last entry; each of those jars writes its pair. With the fe-connector-paimon jar from before the last commit it fails at the route of the ORC table (DistributionSpecGather). 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. A table that writes ORC files gets only its partition values padded, since ORC pads CHAR values itself; a row of a table with rowkind.field is padded only when Paimon decodes that field as an insert ('+I' in any case, or its default), except in an overwrite, which pads every row, so a '-D' row written with a short literal key no longer deletes a key that Doris wrote padded: write the key at full length, or read it back from the table; and a map whose CHAR keys padding would make equal fails the write. While padding is on, a fixed-bucket table is written by one writer when a partition or bucket key is a CHAR column that the writer pads, which leaves out the rowkind.field and, in a table that writes ORC files, the columns other than partition columns, and a MERGE that inserts a CHAR primary-key value shorter than the column fails: pad the key in the MERGE with rpad, or turn padding off.

Check List (For Author)

  • Test

    • Regression test
      • test_paimon_write_char_padding, as above.
    • Unit Test
      • PaimonWriteSchemaTest, PaimonWriteBindingTest, PaimonBackendBoundTableTest and PaimonWritePlanProviderTest, as above; all tests of fe-connector-paimon and paimon-scanner pass.
    • Manual test (add detailed scripts or steps below)
      • Local single-BE cluster with a file:// Paimon catalog: the cases in the tables 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
  • Behavior changed:

    • No.
    • Yes.
      • CHAR values written into Paimon tables are padded to the column length unless enable_paimon_write_char_padding is false; in an ORC table only the partition values.
  • Does this need documentation?

    • No.
    • Yes.
      • The new session variable enable_paimon_write_char_padding.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases

…link do

### What problem does this PR solve?

Issue Number: None

Related PR: apache#69045

Problem Summary:

Doris wrote a CHAR(n) value into a Paimon table as it was, without the
trailing spaces that pad it to n characters. Spark (with the
paimon-spark extensions) and Flink pad CHAR values when they write a
Paimon table, nested values too, and Paimon compares primary keys and
hashes buckets by bytes, so a key that Doris wrote as 'a' and Spark as
'a  ' made two rows of a primary-key table. On a local cluster with a
file:// Paimon catalog, a CHAR(3) primary-key table into which Doris
inserted 'k' and the Paimon API then wrote 'k  ', as Spark and Flink
write 'k', had two rows; it now has one.

The Paimon JNI writer (PaimonWriteSchema) now pads the CHAR values of
the rows it inserts, top-level and nested in ARRAY, MAP and ROW, with
spaces to the column length in code points, as Paimon's own cast to
CHAR does. A longer value is kept: Doris has already cut it or kept it,
as the session asks. The static partition value of a CHAR partition
column is padded the same way, because Paimon parses it without padding
it, and an unpadded value would name another partition than the rows
reach. UPDATE and DELETE rows are not padded: they carry the key Doris
read back from the table, which has to stay byte-equal to the stored
key.

The new session variable enable_paimon_write_char_padding (default
true) turns this 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
FE committer's table goes without it. A writer that finds no such
option, as from an older FE, does not pad.

### 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 key that an earlier Doris wrote unpadded is another key than
its padded form; set enable_paimon_write_char_padding to false to keep
writing CHAR values unpadded.

### Check List (For Author)

- Test: Regression test, Unit Test, Manual test
    - test_paimon_write_char_padding (external_table_p0/paimon/write): a
      key that Doris and Spark both write is one row, nested and partition
      values are padded, a second static overwrite replaces the padded
      partition, the switch off, and DELETE and UPDATE of unpadded keys;
      it fails without this change at its first check
    - PaimonWriteSchemaTest: top-level and nested CHAR padding, padding
      off, UPDATE and DELETE rows left alone, reading the table option
    - PaimonWriteBindingTest: static CHAR partition value padding
    - PaimonBackendBoundTableTest: only the BE table carries the option
    - PaimonWritePlanProviderTest: the session variable, on by default
    - Manual test on a local cluster with a file:// Paimon catalog: the
      primary-key table above; CHAR values nested in ARRAY, MAP and ROW
      and the partition values of a static overwrite and of an INSERT
      are padded; with the switch off values are written unpadded, and
      with it on DELETE and UPDATE still match those unpadded keys
- Behavior changed: Yes. CHAR values written into Paimon tables are
  padded to the column length unless enable_paimon_write_char_padding
  is false.
- Does this need documentation: Yes (the new session variable)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

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?

@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@morningman

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of all 12 changed files at f6b658b, plus the relevant FE planning/forwarding, BE routing/memory, JNI Arrow/write/close, SDK 1.4.2 and transaction commit paths. Three independent reviewers covered schema/BE, FE/tests, and the initial risk mechanisms. Three verified new findings are inline: one P1 routing/compaction issue and two P2 control-token/memory issues. No existing inline threads or additional user focus were supplied.

Critical checkpoint conclusions:

  • Goal and proof: ordinary INSERT padding, nested values and static partitions implement the intended behavior, but the inline findings prevent correctness across all supported cases. Added tests provide relevant assertions; none were executed during this review.
  • Scope and reuse: the change is focused on the Paimon write schema, statement binding and session option; existing SDK/internal-value interfaces are reused.
  • Concurrency and locks: new transformation state is per writer and FE binding state is immutable. No new locks/deadlock path was found; P1 breaks the existing exclusive ownership of a fixed partition/bucket across parallel compaction writers.
  • Lifecycle/static initialization: backend-only table copying leaves the catalog-aware FE committer intact. Existing Arrow release, writer/compaction close and abort boundaries remain; no new handle cycle/static initialization dependency was found.
  • Configuration: default-on session Boolean is dynamic per statement and needForward=true; reflective session extraction and master forwarding carry it to planning.
  • Compatibility: absent backend option means false, matching an old FE's unpadded static values. Supported shared-nothing and cloud upgrade procedures put BE before FE. Padding changes stored CHAR/key bytes as disclosed in the release note; the opt-out preserves legacy INSERT behavior.
  • Thrift/protobuf: no definitions, field ids, enums, native JNI signatures or shared SPI surface changed.
  • Parallel paths: INSERT/OVERWRITE/MERGE inserts share the JNI transform; SQL UPDATE/DELETE preserve read-back keys. Recursive ARRAY/MAP/ROW and non-CHAR/null paths were traced. FFI remains an unselected unsupported stub. Fixed-bucket routing and rowkind.field control handling need the inline fixes.
  • Conditional checks: operation-first ordering, padding-off, non-CHAR passthrough and static SQL NULL handling are justified. Static reserved-name collisions are rejected after normalization; control-column handling is incomplete.
  • Tests: unit and regression changes cover codepoint length, overlength retention, nulls, nested shapes, opt-out, Spark key replacement and repeated static overwrite. Missing cases include multiple fixed buckets/writers, CHAR rowkind control fields and bounded large nested expansion; no executed build/test evidence is claimed.
  • Golden results: all changed tuples agree with the SQL sequence and deterministic order_qt output under static inspection. Author-reported generation/test runs were not independently reproduced.
  • Observability: existing JNI/transaction failures retain useful error boundaries; no new general metric requirement was identified. Native memory metrics do not account for the newly expanded heap data described inline.
  • Transactions/persistence: the execution-only option does not mutate persisted table metadata; FE retains existing commit identity, abort and reconciliation logic. P1 exposes a partial APPEND/COMPACT outcome with missing changelog; no separate new EditLog/master-failover defect was found.
  • Writes/atomicity/crashes: SDK prepare/commit/abort and error propagation otherwise remain intact. The P1 ownership problem and P2 budget bypass are the material correctness/resource risks; no other new crash leak or deadlock was substantiated.
  • FE-to-BE propagation: every production binding creation passes the session decision, the serialized backend table retains write/branch options and JNI reads the same key. Constant-fold/point-query paths do not invoke this writer.
  • Performance/memory: padders/getters are built once and types without CHAR retain existing values. The new eager nested expansion bypasses managed memory limits as reported inline.
  • Additional concerns and user focus: initial risks about defaults, static partition/null/unicode agreement, table-copy/branch identity and supported mixed-version forwarding were resolved with code evidence. No extra user focus was supplied. No unresolved static candidate remains; runtime and golden-generation provenance are outside this static-only execution.

Convergence: converged after 2 review rounds. All three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS in round 2 against the unchanged inline set. All 12 changed files and applicable checkpoints were covered; every candidate and initial risk was accepted, deduplicated or dismissed with evidence. No unresolved static question remains. This convergence result does not claim runtime validation: builds/tests and OOM reproduction were prohibited, and golden-generation provenance was not independently established. The P1 finding requires changes.

Review converged after 2 round(s).

this.writeSchema = PaimonWriteSchema.create(
table.rowType(), columnNames, changelogWrite);
this.writeSchema = PaimonWriteSchema.create(table.rowType(), columnNames, changelogWrite,
PaimonWriteSchema.charPadding(table.options()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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. 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b584341. 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.

// 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve CHAR-typed rowkind.field control tokens

Paimon permits rowkind.field to name a CHAR column: its RowKindGenerator 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b584341. 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.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 allocates padding and concat 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29605 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152089 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.56 s
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

…ile padding

With padding on, the writer pads a CHAR value before Paimon computes the
partition and the bucket of the row, but the fixed-bucket route
(paimon_fixed_bucket) hashes the routing values that BE sends, unpadded.
Rows of one bucket then reached several writers. In a full-compaction
table each of them compacted the bucket, and Paimon gave up committing
the compaction after the rows were committed (LSM conflicts, then file
deletion conflicts), so the INSERT reported success without its
changelog. A CHAR partition or bucket field is now routed only while
padding is off; with padding on the table gets the distribution of a
routing field BE cannot hash, a single writer for a primary-key table.

The writer also leaves the value of the table's rowkind.field alone:
Paimon takes the row kind from it as it is, and a CHAR(3) '+I' padded to
'+I ' failed the write.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@morningman

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes for the new P1 MERGE key-identity bug. Two new inline findings are declared: late CHAR normalization can bypass MERGE matching/cardinality checks (P1), and static overwrites disagree with the writer when a CHAR partition column is also rowkind.field (P2).

Reviewed all 12 changed files at b584341, including surrounding FE planning, Arrow/JNI conversion, SDK routing and commit paths. The runtime chain reviewed was session -> distribution/binding -> serialized backend table -> Arrow row -> CHAR conversion -> SDK bucket writer -> FE commit.

  • Goal and proof: inserted CHAR values are padded recursively by code points and the switch preserves legacy writes; the two findings prevent full correctness. Targeted unit and regression test sources exist; none were executed in this static-only review.
  • Scope and reuse: changes are focused on Paimon conversion, routing and statement options; existing SDK row/array/map and string helpers are reused.
  • Concurrency and locking: no new shared mutable state, threads or lock order. Binding remains immutable, transaction binding/data snapshots retain synchronization, and GATHER restores per-statement bucket ownership for changed CHAR routes.
  • Lifecycle and initialization: padders are writer-local, backend options statement-local, and Arrow views remain live through synchronous SDK consumption. No new global initializer, reference cycle or resource-close path.
  • Configuration: enable_paimon_write_char_padding defaults true, forwards with statements, and is observed on each planned write without restart.
  • Compatibility: no function ABI, JNI signature or storage-format change. Absent backend option defaults false for old-FE/new-BE operation. This relies on the documented BE-before-FE upgrade sequence; reverse ordering can mismatch static partitions.
  • Thrift/protobuf: no changes; field-ID and enum compatibility checks are not applicable.
  • Parallel paths: top-level/nested/null/default values, static/dynamic partitions, SQL UPDATE/DELETE/MERGE, INSERT control rows, fixed/dynamic/key-dynamic/unaware buckets and write-only/compacting variants were traced. Matched UPDATE/DELETE retain scanned key bytes. Findings identify the MERGE INSERT and static control-partition intersections.
  • Conditional checks: INSERT-only padding, control-field exclusion, route rejection, absent-option fallback and static NULL/default-name handling have concrete reasons; FE misses the control-field exclusion in the new P2.
  • Test coverage: new JUnit 5 and regression sources cover CHAR/VARCHAR, Unicode, nesting/nulls, flag-off, legacy-key DML, static overwrites, full compaction and control tokens. They omit the two reported intersections. Direct Arrow-backed nested/default CHAR coverage could be stronger.
  • Changed results: all 11 golden blocks agree with the query projections and operation sequence under the intended behavior; order_qt ensures ordering. The generated-file header and author report do not independently prove generation or successful execution.
  • Observability: existing writer/transaction errors and identifiers remain; no new background lifecycle requires additional metrics. The unchanged heap-accounting concern below remains relevant.
  • Transactions and persistence: FE retains its catalog-aware pinned table; only the serialized backend copy receives the option. Edit-log/replay and commit/reconciliation protocols are unchanged; no new failover state is introduced.
  • Writes and atomicity: existing prepare/commit/abort boundaries remain. The P1 silently bypasses a pre-write semantic check, and the P2 fails partition validation or selects the wrong empty overwrite target. Internal Doris visible-version and MoW delete-bitmap mechanisms are untouched.
  • FE/BE propagation: the session reaches both distribution and binding; the identical table-option key reaches JNI open, including overwrite and changelog sinks. No additional thrift sending point is introduced.
  • Performance and memory: padding work is linear in visited values and expanded bytes; padders are prepared once. GATHER reduces parallelism for correctness. Existing P2 heap amplification still applies and is not reposted.
  • Other issues and duplicate control: independently confirmed the existing P1 routing and P2 token-padding scenarios are fixed. No previously reported P0/P1 remains applicable, so existing_blocking_comment_ids is empty. The new FE overwrite mismatch is distinct from the fixed token parser failure.

No additional user focus was supplied. All changed paths and risk focuses were covered and every candidate received a disposition. Evidence is static source analysis, including the pinned Paimon 1.4.2 implementation; no builds, tests or runtime reproductions were run, and source files were not modified.

Review converged after 2 round(s).

// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 45e6e5d. 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 writes 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.

value = toSdkLocalTime(canonicalName, value);
}
if (charPadding && type.getTypeRoot() == DataTypeRoot.CHAR) {
value = padChar(value, ((CharType) type).getLength());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 45e6e5d. 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

MERGE matches its source rows to a Paimon table, and checks the keys it
inserts for duplicates, by the key as Doris holds it: Doris compares
CHAR values unpadded, so 'k' and 'k ' are two keys and neither matches a
stored 'k  '. The writer padded the keys of the rows MERGE inserts
afterwards. On a local cluster, a NOT MATCHED MERGE of 'k' and 'k ' into
an empty CHAR(3)-keyed table committed one row, and a NOT MATCHED-only
MERGE of 'k' replaced the stored row 'k  ' that it had not matched. The
writer now fails such a row when a CHAR primary-key value is shorter
than the column, the only case in which padding changes a key, and the
message says to pad the key in the MERGE with rpad or to turn padding
off. A full-length key is written as it is.

The static partition value of the table's rowkind.field is no longer
padded either, as the writer already leaves the rows' value alone: with
a CHAR rowkind.field partition column, a nonempty static overwrite of
'+I' failed with "does not belong to this partition", and an empty one
returned OK without clearing the partition.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@morningman

Copy link
Copy Markdown
Contributor Author

/review

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29651 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152669 ms
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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes for two verified P1 data-correctness regressions: rowkind.field control events can delete a different CHAR key, and ORC read-back can send UPDATE/DELETE to a different Paimon bucket. A third verified P2 concerns Avro silently losing an entry when padding aliases CHAR map keys. All three are inline below and distinct from the existing review threads.

Reviewed all 12 changed files at 45e6e5d2eff67184390f37b61fbe0319ec1a9447, plus their planning, session transport, scan, SDK write, and commit/abort interactions. This was a static review: no builds or product tests were run and no source files were changed. The concrete failures are derived from the admitted control flow and pinned upstream implementations; the ORC bucket values were independently calculated from the checked-in BinaryRow encoding/hash algorithm, matching the existing abc golden.

The reviewed write flow is:

session flag -> ConnectorSession -> PaimonWritePlanProvider
  -> WriteBinding (backend table option + FE static overwrite filter)
  -> BE Arrow/JNI -> PaimonWriteSchema.tableRow -> SDK key/bucket/write
  -> prepared commit messages -> FE transaction -> Paimon snapshot
Required checkpoint Conclusion
Goal and proof Normal scalar/nested CHAR insertion interoperability is implemented; the two mutation failures and Avro map-entry loss prevent a correctness approval. Unit/regression cases were inspected, not executed.
Scope, clarity, reuse Focused change using precomputed per-column padders, Paimon string utilities, existing table options and existing routing fallback.
Concurrency and locks New state is per statement/writer and effectively immutable; row conversion is synchronous. No new lock order or shared mutable state. Existing transaction locks remain short and SDK commit I/O stays outside them.
Lifecycle and initialization Borrowed Arrow data remains within the synchronous write call; the existing SDK close/abort boundary remains in force. No new thread, circular ownership or cross-unit static initialization dependency.
Configuration dynamics enable_paimon_write_char_padding defaults to true, is forwarded with statements, is extracted into the connector session and is observed for each new planned write without restart.
Compatibility and upgrades JNI signatures and storage metadata layout are unchanged. Missing backend option means false for old-FE/new-BE compatibility. The declared upgrade order is BE before FE; a new FE with an old BE can fail padded static overwrites, as the PR acknowledges.
Thrift/protobuf field compatibility No gensrc definitions are changed; existing serialized-table transport is reused.
Parallel and special-case paths INSERT, static/dynamic overwrite and SQL row-level DML were traced. The control-field, ORC mutation and Avro map paths have the inline failures. Fixed-bucket GATHER resolves the earlier exchange collision; dynamic buckets already gather. FE selects JNI and FFI is unimplemented.
Conditional checks Short inserted MERGE CHAR keys fail before padding/SDK write; full-length keys remain unchanged. Static partition and writer control-field exemptions agree. NULL propagation is preserved; nested map collision handling has the reported Avro format defect.
Test coverage and negative cases Added coverage includes scalar/nested/null/Unicode padding, opt-out, backend option isolation, static partitions, route fallback, legacy-key SQL DML, rowkind tokens and MERGE rejection/duplicates. Missing cases are CHAR-key control events, multi-bucket ORC mutations and colliding CHAR map keys through Avro.
Changed test results All 16 ordered query labels match the golden blocks; expected rows and errors were traced, including empty overwrites and MERGE values. Golden generation and author test claims were not independently executed or reproduced.
Observability Existing write/prepare/abort and table/transaction diagnostics remain available; the new MERGE rejection identifies the column, width and remedy. No additional observability defect found.
Transactions, persistence and failover The flag is statement configuration, carried in the sink rather than persisted catalog metadata. Snapshot reconciliation and existing failover state are unchanged; no new EditLog record is required. No runtime failover validation was performed.
Data writes, atomicity and crashes New exceptions reach JNI non-OK Status; native close aborts staged output and publishes commit payloads only after successful preparation/shutdown. The reported semantic failures can nevertheless commit successfully with the wrong key/bucket or map entries that disappear on Avro readback. No separate new crash cleanup or lock defect found.
FE-to-BE variable propagation Every planned Paimon sink carries the flag on its backend table; the FE committer retains its catalog-aware table and matching static filter. Non-write point-query/constant-folding paths do not require this option.
Performance and memory Padding traverses affected nested values and output bytes, with schema traversal done once. GATHER's reduced parallelism is intentional. The existing untracked nested heap amplification concern remains applicable and is deduplicated below.
Other concerns and user focus No additional user focus was supplied. All risk questions and candidates have explicit dispositions; no changed file remains unreviewed.

Existing-thread dispositions: the fixed-bucket routing P1 and MERGE key collision P1 are fixed for their reported scenarios at this head, so neither is carried as an existing blocker. The control-token P2 and static control-field partition P2 are also fixed. The nested CHAR heap amplification P2 still applies and is not reposted. The known planner-proven-empty overwrite omission described in the static-partition reply is retained as existing context; the new regression covers sources that become empty at runtime.

Coverage limits: this review proves static paths and arithmetic, not runtime behavior, executed test results, golden-generation provenance, crash/failover behavior or a running mixed-version cluster. The reverse upgrade order and known planner-proven-empty overwrite limitation remain disclosed above. These are explicit validation boundaries; no candidate or assigned review scope remains unresolved.

Review converged after 3 round(s).

// 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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, 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

value, insertedKeyCharLengths[i], inputType.getFields().get(i).name(),
insertedKeyCharLengths[i]));
}
value = charPadders[i].pad(value);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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, 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 UPDATE/DELETE with multiple buckets.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 from 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.

return value -> {
InternalMap map = (InternalMap) value;
return new PaddedMap(
keyPadder == null ? map.keyArray() : padArray(map.keyArray(), keyGetter, keyPadder),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.53 s
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

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29584 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152207 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.52 s
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

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 100.00% (1/1) 🎉
Increment coverage report
Complete coverage report

1 similar comment
@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 100.00% (1/1) 🎉
Increment coverage report
Complete coverage report

…reject colliding map keys

Paimon stores CHAR(n) as ORC char(n): ORC pads every CHAR value it
writes and strips the trailing spaces from one it reads, and so does
Doris's ORC reader. Padding in the writer changed nothing ORC stores,
only the bucket of a CHAR key, and an UPDATE or DELETE, which reads the
key back without its spaces, then hashed another bucket. On a local
cluster, a DELETE of four short keys that Doris had inserted into a
CHAR(3)-keyed ORC table with bucket = 4 returned COMMITTED and removed
none of them. 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: a partition value is read back as it is stored.

Paimon takes the kind of a row of a table with a rowkind.field from
that field's value when it writes the row, and the writer padded every
row Doris inserted. A '-D' row whose key Doris read back as 'k' deleted
the padded 'k  ' instead, and missed the 'k' it was meant for. The
writer now pads a row of such a table only when that value is '+I': a
'-D', '-U' or '+U' row carries a key to match, as UPDATE and DELETE rows
do.

Padding turned the distinct keys 'k' and 'k ' of a MAP<CHAR(2), INT>
into one key 'k ', of which Paimon's Avro reader kept one entry. Such a
map now fails the row; keys that were equal before padding are kept as
they are.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29234 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152586 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.79 s
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

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 100.00% (1/1) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 100.00% (1/1) 🎉
Increment coverage report
Complete coverage report

… entry order

The map key collision check compared the padded keys by their serialized
bytes, which follow the order of the entries of a map nested in a key.
Paimon compares maps regardless of that order (GenericMap.equals), and
its Avro reader puts the keys of a map in a hash map. So in a
MAP<ROW<c CHAR(2), m MAP<STRING, INT>>, INT> column the keys
('k', {a: 1, b: 2}) and ('k ', {b: 2, a: 1}), which padding makes one key,
passed the check: an Avro table read back one entry and silently dropped
the other value, and a Parquet table held two equal keys. The entries of
the maps nested in a key are now sorted by their key bytes before the
key is compared, so such keys fail the row like other keys that padding
makes equal. Keys that were equal before padding are kept as before.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@morningman

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of #69083 at 359271837c1645906ec22bc27e06d34acaa66198 covered all 12 changed files and their planning, Arrow, SDK write and commit paths. One new P2 finding is inline: opposite-sign NaNs in ROW map keys bypass CHAR-padding collision detection and can lose an entry on Avro readback. Review converged after 2 rounds; no unresolved candidates or uncovered changed files remain.

The four earlier P1 findings (fixed-bucket routing, short-key MERGE, rowkind mutation keys and ORC mutation identity) are addressed at this head; no existing P0/P1 blocking IDs remain applicable. Earlier control/default/overwrite/Arrow/map-order scenarios also have fixes. The previously reported P2 untracked expanded Java heap allocation still applies and is not reposted. No additional user review focus was supplied.

Critical checkpoint Conclusion
Goal and correctness Recursive CHAR padding, control handling and partition alignment implement the intended write behavior; the inline NaN collision is a remaining correctness bug.
Scope and clarity Changes are localized to Paimon normalization, binding/distribution, one session option and associated tests.
Concurrency and locking Mutable padding helpers are confined to each synchronous JNI writer; no new shared lock or thread was introduced.
Lifecycle and initialization Arrow views remain valid through synchronous SDK consumption; retained dynamic-index keys are copied. No new static initialization dependency.
Configuration The default-on session boolean is read per statement and changes without restart.
Compatibility An absent backend option preserves old-FE/new-BE behavior. Supported BE-before-FE upgrade order is respected; new-FE/old-BE pairing can fail CHAR static overwrites and is outside that order. No new storage format or symbol.
Thrift/protobuf IDs No definitions changed.
Parallel paths Fixed, dynamic and global bucket writes normalize before assignment; CHAR native routing falls back appropriately, with explicit ORC, mutation, rowkind and MERGE policies.
Conditional checks and errors Concrete failure scenarios are documented; new rejection paths propagate through existing JNI failure/abort handling.
Test coverage Added unit/e2e cases cover Unicode, nested values/maps, switches, defaults/controls, overwrite, routing/compaction, ORC and MERGE. The signed-NaN case is missing.
Test results All 24 ordered regression labels match the golden sections; expected rows are consistent on static inspection. Runtime results and golden generation were not independently executed.
Observability Existing writer/transaction context and column-specific failure messages cover the localized behavior.
Persistence and failover Existing binding, commit, abort and reconciliation mechanisms remain intact; no EditLog, visible-version or delete-bitmap change.
Write atomicity and crash handling New validation failures use existing abort/cleanup paths. The inline issue concerns committed map contents, rather than a new publication protocol.
FE/BE variable passing Reflection-based session extraction/forwarding and the backend-only serialized table option reach all selected write modes.
Performance and memory Padders are built once per schema; nested-map sorting and CHAR routing consolidation have explicit costs. The existing untracked-heap P2 remains a limit.
Other concerns and coverage limits Defaults, per-level format normalization, partition readback, prepared-statement replanning, duplicate threads and upstream equality were checked. Static-only review: no builds or tests were run and repository sources were not modified.

Review converged after 2 round(s).

*/
private BinaryRow bytes(Object key) {
Object sorted = key == null || entrySorter == null ? key : entrySorter.sort(key);
return serializer.toBinaryRow(GenericRow.of(sorted)).copy();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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, so this byte comparison accepts both keys. On read, GenericRow equality/hash treats these NaNs as equal, and Avro's ArrayMapReader inserts the now-equal keys 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29615 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 153042 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.62 s
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

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 100.00% (1/1) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 100.00% (1/1) 🎉
Increment coverage report
Complete coverage report

The map key collision check compared the padded keys by their serialized
bytes, and a ROW keeps the bits of a NaN when it is serialized, while
Paimon compares a FLOAT or DOUBLE as Double.equals does, taking every
NaN as one value, and its Avro reader puts the keys of a map in a hash
map. A query that casts 'NaN' and '-NaN' at run time keeps their signs
through the Arrow batch, so in a MAP<ROW<c CHAR(2), d DOUBLE>, INT>
column the keys ('k', NaN) and ('k ', -NaN), which padding makes one
key, passed the check: an Avro table read back one entry and silently
dropped the other value, and a Parquet table held two equal keys. Each
NaN in a key, at any depth, is now made the canonical NaN before the key
is compared, so such keys fail the row like other keys that padding
makes equal. Keys that were equal before padding are kept as before,
and -0.0 stays apart from 0.0, as it does for Paimon.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@morningman

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed PR #69083 at c4acf1b. All 12 changed files and their relevant upstream/downstream paths were inspected. Two new P2 findings are inline: an Avro outer-map value can still be lost after descendant-map key normalization, and unchanged ORC CHAR bucket keys unnecessarily lose parallel writer routing. Neither finding duplicates the original scalar-NaN, unique-map-order, or changed-routing-byte threads.

The review converged in round 2: all three assigned passes returned NO_NEW_VALUABLE_FINDINGS against this unchanged two-comment set. Every candidate and initial risk has an explicit disposition, and the final sweep found no unresolved question or uncovered changed file. This is a static review; no builds or tests were run.

The four previously reported P1 findings (4273514658, 4274646965, 4276034908, 4276034922) no longer apply to this head: CHAR routing falls back where padding changes routing bytes, MERGE rejects short inserted CHAR primary keys, effective rowkind/default/case semantics preserve mutation keys, and ORC nonpartition keys remain unpadded. No existing P0/P1 is carried forward. Other original correctness examples are addressed; the existing P2 Java-heap amplification concern (4273514673), which the author declined to change, remains known context and is not reposted.

Critical checkpoint conclusions:

  1. Goal and proof: the intended change is write-side CHAR interoperation with Spark/Flink plus a legacy opt-out. Scalar/nested/Unicode, static partition, mutation and overwrite cases are covered in the added tests; the two inline findings identify remaining behavior and coverage gaps.
  2. Scope and abstraction: changes stay within the writer/schema, Paimon binding/planning, one session variable and tests. Recursive padders and comparison normalization are isolated and created per schema.
  3. Concurrency and locks: new mutable serializers/default wrappers belong to one writer and execute synchronously in writeBatch. No shared lock, lock order or new thread is introduced; existing compaction workers consume SDK-owned data.
  4. Lifecycle: imported Arrow roots outlive synchronous row consumption; comparison bytes are copied before serializer reuse. Existing prepare/abort/close paths and statement-scoped binding ownership are retained; no global initialization dependency is added.
  5. Configuration: enable_paimon_write_char_padding defaults true, is session-mutable and needForward=true, is extracted into ConnectorSession, and controls both planning and the BE-bound table option.
  6. Compatibility: no ABI or storage-layout migration. Missing option means false for an old FE with a new BE; existing mutation keys and the opt-out remain supported. The mixed-version analysis follows the documented BE-before-FE upgrade order. Read-side CHAR comparison is intentionally unchanged.
  7. Thrift/protobuf and SPI: no definitions, numeric ids or shared connector API surface change; the setting uses existing serialized-table transport.
  8. Parallel paths: reviewed fixed/hash-dynamic/key-dynamic writes, INSERT/UPDATE/DELETE/MERGE, static/dynamic/full overwrite, rowkind control/default/case handling, ORC/per-level formats, and nested ARRAY/MAP/ROW paths. The inline findings cover the residual map-equivalence and ORC parallelism cases.
  9. Conditions and errors: short-MERGE-key, map-collision, rowkind and ORC exclusions have concrete triggers and explanations. Invalid rowkind parsing is deferred to the SDK's authoritative validation; write errors propagate to statement failure.
  10. Test coverage: unit and regression additions exercise option transport, padding on/off, Unicode, nested values, key collisions, legacy mutations, rowkind behavior, MERGE guards, ORC and compaction snapshots. The two inline scenarios need additional coverage.
  11. Expected results: all 25 regression query tags match golden labels and their expected rows were traced through the statements. Golden generation and author-reported test runs were not independently executed or treated as test evidence.
  12. Observability: new diagnostics identify the column/key and corrective action. Existing JNI/commit error context remains; no new high-rate logging or critical service metric is needed.
  13. Persistence and FE failover: no new EditLog state, durable catalog mutation or transaction recovery format; the extra option is confined to the serialized writer table. Existing commit/reconciliation behavior is unchanged.
  14. Write atomicity and crash handling: normalization/validation precede SDK row writes and successful FE commit; failures use existing abort/resource cleanup. Native visible-version and MoW delete-bitmap mechanisms are untouched.
  15. FE/BE propagation: the option spelling matches both modules; the writer receives primary/partition keys, rowkind field, format and overwrite status on the common creation path. No separate constant-fold or point-query parameter path is introduced.
  16. Performance and memory: padders reuse schema/type decisions and borrow unchanged descendants; comparison copies own their bytes. The ORC gather regression is reported inline without claiming measured throughput. Existing expanded Java-heap allocation remains the already-reported concern noted above.
  17. Other concerns and limits: review_focus.txt supplies no additional focus. No unresolved candidate or changed-file coverage gap remains after the static review. Builds, tests, package checks and live Doris/Avro execution were not run, as the review contract prohibits them; no source code was modified.

Review converged after 2 round(s).

order[i] = i;
}
Arrays.sort(order, (left, right) -> compareBytes(keyBytes[left], keyBytes[right]));
Object[] sortedKeys = new Object[keys.length];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 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 equivalent collision rejection. Cover this two-entry versus one-entry case through Arrow and Avro.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private boolean supportsNativeRouting(DataTypeRoot type, boolean charPadding) {
switch (type) {
case CHAR:
return !charPadding;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 no 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.

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29553 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152471 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.47 s
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

morningman and others added 2 commits October 16, 2026 20:49
…f each key

The map key collision check sorted the entries of a map nested in a key
before it compared the keys by their bytes, but kept every entry, while
Paimon's Avro reader puts the entries of a map in a hash map, which keeps
the value of the last of the entries with equal keys. Doris's map() keeps
NaN and -NaN apart, since their bits differ, so in a
MAP<ROW<c CHAR(2), mm MAP<DOUBLE, INT>>, INT> column the keys
('k', {NaN: 1, -NaN: 2}) and ('k ', {NaN: 2}), which padding makes one key
once the first nested map is read back as {NaN: 2}, passed the check: an
Avro table read back one entry and silently dropped the other value. The
copy of a nested map that the check compares now keeps only the last of
the entries whose keys are equal, so such keys fail the row like other
keys that padding makes equal. Keys that were equal before padding are
kept as before, and the nested maps are written whole.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…atively

FE kept every CHAR routing field off the native bucket route while
padding was on, but the writer does not pad every CHAR field: it leaves
the table's rowkind.field alone, and of a table that writes ORC files it
pads only the partition fields, since ORC pads every CHAR value it
writes. A fixed-bucket primary-key ORC table with a CHAR bucket key was
therefore written through a single writer, though BE hashes the bytes
the writer writes. FE now decides it per field, by the writer's rule,
which the static partition padding shares: such a table keeps its
parallel bucket writers, and a CHAR field that the writer pads still
falls back as before.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@morningman

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of all 12 changed paths at 1012317776ab401cbf6f76ca82fd72a8431dd49b: no additional verified inline findings. The existing P1 instances for fixed-bucket routing (4273514658), MERGE short keys (4274646965), ORC mutation identity (4276034922), and effective row-kind decoding (4276034908) are addressed at this head; no existing P0/P1 blocker is carried forward.

The existing P2 heap-accounting concern still applies: expanded CHAR values and comparison copies allocate Java heap before managed writer accounting. It is acknowledged here without another inline comment. Other existing P2 instances have corresponding repairs and targeted tests.

The reviewed write flow is session setting -> FE distribution/static-partition binding -> backend-only table option -> JNI/Arrow row mapping and padding -> SDK write/prepare -> FE commit. Conclusions for the required checkpoints:

  1. Goal and evidence: scalar and nested inserted CHAR values pad by code points, with documented mutation, control-field and ORC exemptions. Unit and regression coverage exercises these policies; tests were inspected, not executed.
  2. Scope and reuse: changes stay within Paimon write policy, its session switch and tests, and reuse SDK row-kind/default conversion, typed serializers and casts. Connector boundaries and shared SPI remain unchanged.
  3. Concurrency and locks: each local writer owns its schema; synchronous Arrow consumption does not race SDK compaction. Restored native routes use unchanged bytes; padded routing fields fall back appropriately. No new lock or lock-order change found.
  4. Lifecycle and initialization: Arrow views remain valid during synchronous consumption; comparison bytes are copied before serializer reuse. Existing shutdown releases schema/adapter after SDK users stop. No new static initialization dependency.
  5. Configuration: the default-true switch is captured for each statement, supports changes without restart and is forwarded through the annotated generic session-variable path.
  6. Compatibility: no JNI signature or storage-format definition changes. An absent option preserves old FE behavior on new BE. New-FE/old-BE static-filter mismatch was considered and dismissed under the documented BE-before-FE upgrade sequence; this review does not assert support for the reverse order.
  7. Thrift/protobuf: no definitions or field IDs changed.
  8. Parallel paths: INSERT, OVERWRITE and changelog writes share option transport; fixed, dynamic and unaware bucket modes, ORC/per-level formats, rowkind controls, partial/default inputs and static empty overwrites were checked.
  9. Conditions: field exemptions, effective row-kind/default decoding, overwrite padding and MERGE short-key rejection have concrete documented reasons and match the FE static-filter policy.
  10. Test coverage: added unit tests cover Unicode, NULL, overlength, nested/Arrow keys, collisions, NaNs, descendant map order/last-wins reduction, switch behavior, routing and schema copies. Regression cases cover Spark interoperability, DML, MERGE, partition overwrite and full-compaction snapshots, including negative cases.
  11. Expected results: all 29 regression query labels pair with output blocks and agree with the traced statements. Runtime correctness and automatic output-generation provenance were not independently reproduced.
  12. Observability: validation errors identify affected columns/keys and remedies; existing JNI/transaction error context and writer counters remain adequate for this local transformation.
  13. Persistence and failover: no EditLog or persisted table-schema change; FE retains its catalog-aware committer table and existing transaction/reconciliation protocol.
  14. Atomicity and crashes: validation exceptions propagate to the native failure/abort path; commit messages publish only after successful preparation and shutdown. No additional atomicity regression found in the changed flow.
  15. FE/BE passing: the identical option key reaches every JNI write mode through the serialized table; overwrite options survive the backend-only schema copy.
  16. Performance and memory: policy structure is precomputed per writer; native parallelism remains for unchanged ORC/control fields. Nested-map comparisons require copying and sorting. The known untracked heap expansion remains the limitation noted above.
  17. Further review and user focus: defaults, nested equality, serializer ownership, error cleanup and supported mixed-version sequencing were rechecked. No additional user focus was supplied, and no unread changed path or unresolved code candidate remains.

Validation limits: static analysis only, as required; no builds, unit/regression tests, source changes or cluster execution were performed. Author-reported runtime results are not presented as independently verified.

Review converged after 2 round(s).

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29130 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 151793 ms
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

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.59 s
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

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 100.00% (1/1) 🎉
Increment coverage report
Complete coverage report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants