Skip to content

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

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

morningman wants to merge 6 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. The entries of a map nested in a key are sorted by their key bytes first, because Paimon compares maps regardless of the order of their entries.
  • 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 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. 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.
    • PaimonWriteBindingTest.staticCharPartitionIsPaddedAsTheRowsAre, PaimonWriteBindingTest.staticRowKindFieldPartitionIsLeftUnpaddedAsTheRowsAre, PaimonBackendBoundTableTest.writeBindingTellsOnlyTheBackendWhetherToPadCharValues, PaimonWritePlanProviderTest.charPaddingFollowsTheSessionVariableAndDefaultsToOn and PaimonWritePlanProviderTest.charRouteFieldsFallBackToSingleWriterWhenCharPaddingIsOn (a CHAR primary key or partition key: one writer with padding on, paimon_fixed_bucket with it off).
    • 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, and comparing keys without sorting the entries of their nested maps.
    • All 739 tests of fe-connector-paimon (not changed by the last commit) and 138 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 routing 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
    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, 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, 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), and with the one from before the last commit at the pair of keys whose nested maps differ only in their entry order, which it writes. 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 whose partition or bucket key is CHAR is written by one writer, 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.

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

@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 f2c45d7, using the supplied authoritative diff and existing-thread context. Three new P2 findings are inline: effective INSERT controls can bypass padding, +U static overwrite rows can disagree with the FE partition filter, and a single nested map key can fail collision checking. No new P0/P1 finding was verified.

Converged after 3 review rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS in the latest round against this exact three-comment set. Required coverage is complete and every candidate and initial risk is accepted, dismissed or deduplicated with evidence; no unresolved review question remains within the static scope.

The reviewed write chain is:

SessionVariable -> connector session -> distribution / write binding
  -> serialized backend table option -> JNI Arrow row -> CHAR normalization
  -> SDK defaults / row kind / partition / bucket -> prepare -> FE commit

Critical checkpoint conclusions:

  1. Goal and correctness: ordinary inserted CHAR values, Unicode lengths, nested values and static +I partitions are padded, with a session switch for legacy keys. The three inline cases leave supported gaps. Unit/regression sources support the ordinary cases; no test execution was permitted.
  2. Scope and reuse: changes are localized to Paimon normalization/planning, one session variable and tests; SDK getters/string utilities are reused. No shared SPI or unrelated production code changed.
  3. Concurrency and locks: normalization is writer-local and synchronous. Enabled CHAR routing fields fall back from native fixed-bucket hashing to the existing safe distribution, addressing the earlier multiple-writer compaction issue. Existing transaction state locks remain short; no new lock order or shared mutable flag was introduced.
  4. Lifecycle and initialization: imported Arrow data remains alive through synchronous SDK consumption; newly created wrappers do not escape that call. Backend options are statement-bound, the FE retains its catalog-aware commit table, and no new native resource or static initialization dependency was added.
  5. Configuration: the new boolean defaults true and can be changed with SET. VariableMgr, ConnectorSessionBuilder and needForward=true carry its value through statement forwarding and into both routing and sink planning.
  6. Compatibility: the existing serialized table options carry explicit true/false; absent means false for old FE to new BE. The PR requires BE-first upgrades. No function ABI or file-format definition changes were introduced; written CHAR bytes intentionally change on the enabled paths.
  7. Thrift/protobuf: no definitions, field ids or enum values changed.
  8. Parallel paths: INSERT, OVERWRITE, MERGE, UPDATE/DELETE and fixed/dynamic bucket paths were traced. MERGE guards inserted short padded keys; mutations retain stored identity; ORC nonpartition columns remain unpadded, including per-level ORC formats. The inline findings identify the missed control/default and nested-key combinations.
  9. Special conditions: rowkind.field token exemption and its own static-partition exemption match; ORC formats canonicalize case. The effective-kind predicate and ordinary data-partition overwrite cross-product are defective as described inline. Upstream full-schema projection rules out the initially suspected omitted-default bypass.
  10. Tests: added JUnit/regression coverage includes flag on/off, Unicode/nested padding, Spark key replacement, routing/compaction snapshots, legacy mutations, ORC, scalar map collisions and MERGE failures. It misses lowercase/defaulted INSERT controls, a separate control field with +U static overwrite, and real Arrow-backed nested map keys.
  11. Results: all 20 ordered regression query tags match the 20 output sections and their expected values are consistent on inspection. Generated-file provenance and passing results were not independently established; no builds or tests were run.
  12. Observability and errors: new validation errors identify relevant columns/keys, and existing JNI/status boundaries propagate failures through abort/commit handling. Existing sink profiles and FE transaction logs remain available; no new daemon or long-running service needs metrics.
  13. Persistence and failover: the flag is statement-local; no new journal state, OLAP visible-version or delete-bitmap behavior changed. Existing SDK snapshot reconciliation and FE transaction state transitions remain intact.
  14. Writes, atomicity and crashes: normalization precedes prepare/commit; native errors abort and commit payloads publish only after successful backend close. +U overwrite mismatch fails the SDK partition check before publication. No additional crash leak, deadlock or partial-commit regression was established.
  15. FE/BE transport: the flag uses the same option literal in FE and BE, with pinned primary/partition/control-field metadata from the serialized table. No separate constant-folding or point-query write transport needs updating.
  16. Performance and memory: recursive padders are compiled once and scalar collision checks are linear. GATHER deliberately costs parallelism for CHAR routes, conservatively including ORC. The existing untracked nested-CHAR heap-expansion P2 remains known and was not reposted.
  17. Further checks and remaining limits: all candidates and initial risks have evidence-backed accepted, dismissed or duplicate dispositions. Review was static only. The documented INSERT-only mutation behavior, historical cross-engine ORC key limitation and already-discussed planner-proven-empty/GATHER overwrite limitation remain known context, not new inline findings.

Existing P1 threads 4273514658 (routing), 4274646965 (MERGE identity), 4276034908 (explicit noninsert controls) and 4276034922 (ORC mutations) were independently confirmed addressed at this head and are not repeated or carried as blocking IDs. Existing scalar control-token, control-partition and map-collision reports are addressed for their original cases; the new inlines explain their distinct branches. Memory-expansion thread 4273514673 remains an existing P2 concern.

User focus: no additional focus was supplied. Full changed-file scope was reviewed. No repository source was modified.

Review converged after 3 round(s).

*/
private boolean insertsRow(GenericRow row) {
return row.getRowKind() == RowKind.INSERT
&& (rowKindFieldIndex < 0 || INSERT_ROW_KIND.equals(row.getField(rowKindFieldIndex)));

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] Resolve the effective row kind before deciding to pad

For a Parquet table (c CHAR(3), rk STRING, v INT) with primary key c, bucket=1 and rowkind.field=rk, INSERT INTO t VALUES ('k', '+i', 1) skips padding here, although Paimon's RowKind.fromShortString uppercases its input and accepts this as INSERT. A subsequent INSERT ... VALUES ('k', '+I', 2) writes 'k ' and leaves two primary-key rows instead of replacing the first. Nullable rk DEFAULT '+I' with an explicit NULL has the same failure: the SDK substitutes its default before decoding the row kind. Determine the effective kind with the SDK's default/case rules, and cover both paths. These are distinct from the fixed explicit -D case: the replacement predicate now misses valid INSERT events instead of changing deletion keys.

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 1740cfa4637. This reproduces: in a Parquet table keyed by c CHAR(3) with rowkind.field = rk, ('k', '+i', 1) was written unpadded, and so was ('j', NULL, 2) when rk has DEFAULT '+I', so a later '+I' of each key added a second row 'k ' or 'j '. Paimon fills a NULL with the field's default (DefaultValueRow) and decodes the value ignoring case (RowKind.fromShortString) before it takes the kind of the row (TableWriteImpl#writeAndReturn), while the writer compared the value with '+I' byte by byte. It now decodes the kind with Paimon's own DefaultValueRow and RowKindGenerator, so '+i', and a NULL or an omitted value under a '+I' or '+i' default, insert and are padded. A value Paimon cannot decode is left for Paimon's write to reject, as before.

Tests: PaimonWriteSchemaTest.testRowKindFieldValueDecidesWhetherARowIsPadded now covers '+i', '-d', an unknown value, and an explicit and an omitted NULL under '+I', '+i' and '-D' defaults; not filling the default turns it red. test_paimon_write_char_padding writes '+i' and a NULL under DEFAULT '+I', then '+I' rows of the same keys, and gets one padded row per key.

value = toSdkLocalTime(canonicalName, value);
}
if (charPadding && type.getTypeRoot() == DataTypeRoot.CHAR && !canonicalName.equals(rowKindField)) {
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] Keep +U overwrite rows aligned with the partition filter

For a table (id INT, p CHAR(3), rk STRING, v INT) with primary key (id,p), partition p, bucket=1 and rowkind.field=rk, the supported INSERT OVERWRITE TABLE t PARTITION(p='x') SELECT 1, '+U', 9 pads the FE overwrite filter to 'x ' here. The new PaimonWriteSchema.insertsRow predicate skips all row padding for +U, leaving the materialized partition value 'x'; the SDK then rejects the appended file as outside the overwrite partition. This worked with the previous unpadded path. Use a consistent static partition policy for the filter and every accepted event kind, or reject the combination before writing, and add a +U overwrite regression. Unlike the earlier control-column partition issue, p is an ordinary CHAR partition and rk is a separate STRING column.

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 1740cfa4637. This reproduces, and for more than '+U': in a table keyed by (id, p), partitioned by p CHAR(3) and with rowkind.field = rk, a static overwrite of p = 'x' failed with Trying to overwrite partition {p=x }, but the changes in p=x/ does not belong to this partition for a '+U' row, and likewise for a '-D' row, in an ORC table, and for a '+I' and a '-D' of one key in one statement; with padding off all of them worked. An overwrite replaces the partitions it writes (withIgnorePreviousFiles), so none of its rows matches a stored key, and the reason the previous commit gave for leaving '-D', '-U' and '+U' rows unpadded does not hold there. The writer now pads every row of an overwrite, as FE pads the static partition value of its filter: the '+U' row lands in 'x ', and a '+I' and a '-D' of one key in the overwrite still cancel out. Writes other than overwrites are unchanged.

Tests: PaimonWriteSchemaTest.testOverwritePadsEveryRowOfARowKindFieldTable (the '+I', '+U', '-D' and '-U' rows of an overwrite are padded, only the partition value of an ORC table, and the '+U' row of a plain INSERT is not); padding by the decoded row kind in an overwrite turns it red. test_paimon_write_char_padding overwrites p = 'x' with a '+U' row, which reads back as [x ], and p = 'y' with a '+I' and a '-D' of one key, which leaves the partition empty.

Object key = keyGetter.getElementOrNull(keys, i);
padded[i] = key == null ? null : keyPadder.pad(key);
if (!keyOfPadded.containsKey(padded[i])) {
keyOfPadded.put(padded[i], key);

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] Avoid object hashing of Arrow-backed nested map keys

Copying an existing Parquet Paimon column MAP<ROW<c CHAR(2), r ROW<i INT>>, INT> into another Paimon table (force_jni_scanner=true; INSERT INTO dst SELECT m FROM src) reaches this check with an Arrow-backed key. The ROW padder creates an outer GenericRow for c but retains the unchanged nested r as ColumnarRow. This HashMap invokes GenericRow.hashCode and then ColumnarRow.hashCode, which unconditionally throws UnsupportedOperationException, even for a single map entry. The external-schema mapper, JNI reader and Arrow serializer all recursively support this scan/copy path; Doris DDL's scalar-key validation is not involved. Use type-aware key hashing/comparison or fully materialize descendants, and cover a real Arrow-backed nested ROW key. This differs from the previous scalar collision issue: no duplicate keys are required.

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 1740cfa4637. This reproduces, and for more than this shape: the writer's rows always come from the Arrow batch, so a ROW key holding a ROW is a columnar row whose hashCode throws, and even a map of one such key in MAP<ROW<c CHAR(2), r ROW<i INT>>, INT> failed the row with ColumnarRowData do not support hashCode, whether the source was a Paimon table read natively or through JNI, or a Doris literal. Two ROW or ARRAY keys that padding made equal hit the unsupported equals instead of the intended error, and two ROW keys holding arrays were not caught at all, since ColumnarArray hashes by identity; an Avro table then kept only one of them. Keys are now compared by their bytes as the only field of a BinaryRow (InternalRowSerializer), and the error prints them as Paimon casts them to STRING, since toString of a columnar row calls the same hashCode: turns the keys '{k, 1}' and '{k , 1}' into the same key '{k , 1}'.

Tests: PaimonWriteSchemaTest.testMapKeysOfArrowRowsAreComparedByTheirBytes feeds rows read back from an Arrow batch: a ROW-of-ROW key is padded, and colliding ROW, ARRAY and ROW-with-ARRAY keys fail with the intended message. Hashing the key objects, or printing them with toString, turns it red. test_paimon_write_char_padding writes a map with a ROW-of-ROW key into a Spark-created table and fails a pair of such keys that padding makes equal; with the paimon-scanner jar from before this commit the suite fails at that first write.

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29696 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit f2c45d7e38c5514bc47dad67b822cdb1c6c044d6, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17627	5680	5573	5573
q2	2083	319	295	295
q3	10212	1436	850	850
q4	4683	466	355	355
q5	7460	825	555	555
q6	185	185	160	160
q7	775	802	614	614
q8	9255	1335	1136	1136
q9	5560	4296	4279	4279
q10	6800	1287	994	994
q11	430	275	238	238
q12	635	401	308	308
q13	18072	3537	3032	3032
q14	269	279	245	245
q15	q16	731	723	666	666
q17	1186	762	581	581
q18	6996	6247	6176	6176
q19	1116	979	619	619
q20	383	340	225	225
q21	5354	2493	2662	2493
q22	389	365	302	302
Total cold run time: 100201 ms
Total hot run time: 29696 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	6552	6488	6515	6488
q2	750	602	571	571
q3	5602	5797	5185	5185
q4	2324	2197	1626	1626
q5	5861	5600	5638	5600
q6	264	188	149	149
q7	2420	1997	1788	1788
q8	3125	2736	2733	2733
q9	8175	7601	7543	7543
q10	3658	3616	3225	3225
q11	624	425	404	404
q12	669	683	514	514
q13	3316	3651	3014	3014
q14	284	296	271	271
q15	q16	674	696	625	625
q17	8106	6998	6987	6987
q18	13232	12250	13116	12250
q19	929	807	792	792
q20	2195	2144	1934	1934
q21	5901	4773	5023	4773
q22	517	448	412	412
Total cold run time: 75178 ms
Total hot run time: 66884 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152477 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 f2c45d7e38c5514bc47dad67b822cdb1c6c044d6, data reload: false

query5	4321	584	445	445
query6	424	210	192	192
query7	4806	435	226	226
query8	319	187	156	156
query9	8775	4005	4007	4005
query10	476	316	266	266
query11	6064	3802	3521	3521
query12	165	91	84	84
query13	1240	463	327	327
query14	6171	4052	3757	3757
query14_1	3543	3571	3568	3568
query15	215	200	172	172
query16	940	447	446	446
query17	872	651	539	539
query18	2439	425	314	314
query19	195	162	142	142
query20	88	87	83	83
query21	214	138	117	117
query22	12933	13041	12859	12859
query23	13387	12758	12411	12411
query23_1	12498	12558	12392	12392
query24	7103	840	481	481
query24_1	479	487	479	479
query25	542	420	373	373
query26	1279	260	145	145
query27	2763	440	272	272
query28	4608	1977	1990	1977
query29	1570	598	465	465
query30	297	222	189	189
query31	1008	919	805	805
query32	146	95	92	92
query33	512	304	243	243
query34	914	864	507	507
query35	746	750	635	635
query36	837	804	725	725
query37	132	104	94	94
query38	1797	1733	1703	1703
query39	808	799	754	754
query39_1	742	735	752	735
query40	235	116	99	99
query41	72	70	73	70
query42	88	87	88	87
query43	367	385	335	335
query44	1331	702	716	702
query45	179	187	167	167
query46	850	962	579	579
query47	3013	2932	2931	2931
query48	312	311	254	254
query49	580	395	286	286
query50	668	280	227	227
query51	10511	10630	10285	10285
query52	80	83	70	70
query53	186	208	153	153
query54	229	193	188	188
query55	69	73	62	62
query56	219	221	215	215
query57	1593	1577	1547	1547
query58	292	262	254	254
query59	2134	2142	1992	1992
query60	306	242	223	223
query61	141	147	142	142
query62	393	345	289	289
query63	203	171	167	167
query64	2728	953	770	770
query65	3297	3254	3230	3230
query66	1767	415	303	303
query67	20514	20093	20158	20093
query68	3006	994	619	619
query69	394	326	255	255
query70	891	791	800	791
query71	294	227	210	210
query72	2703	2577	2318	2318
query73	508	536	297	297
query74	4580	4469	4315	4315
query75	2353	2299	1937	1937
query76	2298	1075	630	630
query77	336	380	288	288
query78	9809	9758	9209	9209
query79	968	902	518	518
query80	740	442	333	333
query81	557	321	268	268
query82	584	134	110	110
query83	276	195	182	182
query84	308	123	101	101
query85	925	432	363	363
query86	346	238	231	231
query87	2012	1939	1839	1839
query88	3675	2740	2720	2720
query89	348	299	253	253
query90	1762	185	178	178
query91	166	151	124	124
query92	98	82	95	82
query93	1009	1005	598	598
query94	473	313	267	267
query95	584	390	321	321
query96	690	523	237	237
query97	2470	2426	2295	2295
query98	166	152	149	149
query99	785	765	659	659
Total cold run time: 234496 ms
Total hot run time: 152477 ms

@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 f2c45d7e38c5514bc47dad67b822cdb1c6c044d6, data reload: false

query1	0.00	0.00	0.00
query2	0.10	0.05	0.06
query3	0.26	0.12	0.11
query4	1.61	0.13	0.12
query5	0.25	0.22	0.25
query6	1.17	0.68	0.68
query7	0.04	0.01	0.01
query8	0.05	0.04	0.04
query9	0.41	0.34	0.35
query10	0.58	0.61	0.58
query11	0.21	0.15	0.14
query12	0.19	0.14	0.15
query13	0.48	0.49	0.49
query14	1.02	1.01	1.02
query15	0.63	0.61	0.62
query16	0.36	0.37	0.36
query17	1.15	1.11	1.11
query18	0.23	0.22	0.21
query19	2.09	1.94	2.03
query20	0.02	0.01	0.02
query21	15.48	0.25	0.14
query22	4.43	0.06	0.05
query23	16.27	0.31	0.12
query24	3.28	0.60	0.42
query25	0.11	0.07	0.04
query26	1.12	0.27	0.15
query27	0.05	0.03	0.03
query28	1.98	0.64	0.41
query29	12.51	4.14	3.31
query30	0.28	0.13	0.14
query31	2.77	0.60	0.34
query32	3.23	0.61	0.52
query33	3.27	3.25	3.23
query34	15.54	4.00	3.40
query35	3.36	3.32	3.35
query36	0.55	0.48	0.42
query37	0.09	0.06	0.06
query38	0.05	0.04	0.03
query39	0.03	0.03	0.03
query40	0.17	0.14	0.13
query41	0.08	0.03	0.03
query42	0.03	0.02	0.02
query43	0.04	0.04	0.03
Total cold run time: 95.57 s
Total hot run time: 24.53 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

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

… compare map keys by bytes

Three cases of CHAR padding failed a write or missed a duplicate key:

- Paimon fills a NULL rowkind.field value with the field's default and
  decodes the value ignoring case, so '+i', or a NULL under DEFAULT '+I',
  inserts. The writer compared the value with '+I' byte by byte and left
  such a row unpadded, so a later '+I' of the same key became a second
  primary-key row. It now decodes the kind with Paimon's DefaultValueRow
  and RowKindGenerator; a value Paimon cannot decode is left for Paimon
  to reject, as before.
- INSERT OVERWRITE ... PARTITION (p = 'x') on a table with a
  rowkind.field pads the overwrite filter to 'x  ', but its '+U' and '-D'
  rows stayed unpadded, so Paimon rejected their files as outside the
  overwritten partition. An overwrite replaces the partitions it writes,
  so its rows match no stored key, and all of them are padded now; a
  '+I' and a '-D' of one key in it still cancel out.
- The map key collision check hashed the keys, which come from the
  Arrow batch: a ROW key holding a ROW, or an ARRAY key, is a columnar
  row or array that supports neither hashCode, equals nor toString. One
  such key failed the row with an UnsupportedOperationException, and two
  ROW keys holding arrays that padding made equal were not caught. Keys
  are now compared by their serialized bytes and printed as Paimon casts
  them to STRING.

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 all 12 changed files and the related planning, Arrow/JNI, SDK write/commit, default-conversion and routing paths at 1740cfa4637dea43b1143058633d5ffcc3e9348f. One new P2 finding is inline: the new binary comparator misses padding-created collisions when nested MAP entries have different orders, allowing an Avro read to discard a map value.

The four previously reported P1 scenarios are fixed at this head: fixed-bucket routing, MERGE key identity, effective row-kind keys, and ORC key/bucket identity. No existing P0/P1 remains independently confirmed, so existing_blocking_comment_ids is empty. The existing P2 heap-amplification concern remains applicable and was not reposted. The new inline identifies a distinct failure in MapKeyFormat.bytes; the earlier scalar-collision and unsupported Arrow object-hashing examples are fixed.

Critical checkpoint conclusions:

  1. Goal and evidence: scalar/nested CHAR insert padding and static partition normalization implement the interoperability goal with the documented DML/control/ORC exceptions. The nested-MAP equality defect prevents complete correctness. Added tests express the ordinary intended behavior; they were inspected, not run.
  2. Scope and reuse: changes are focused on Paimon writer/planning/binding, one session variable and their tests; existing SDK casts/serializers and SPI/session channels are reused.
  3. Concurrency and locks: each native local sink owns one synchronous writer and its mutable schema/serializer state. Existing compaction is asynchronous; CHAR routes use the single-writer fallback where bucket ownership requires it. No new lock or lock-order issue found.
  4. Lifecycle: Arrow rows are consumed before imported roots close; copied binary keys avoid serializer-buffer aliasing. Existing abort/prepare/close ownership remains intact; no new resource or static-initialization cycle found.
  5. Configuration: the default-true session boolean is forwarded and captured for the statement; distribution and write binding read the same setting. Changes affect subsequent statements without restart.
  6. Compatibility: no SPI or on-disk schema change. Old FE/new BE uses absent-option=false consistently with old static values. New FE/old BE is the PR's disclosed reverse-order limitation; deployment assumes BEs are upgraded first. No mixed-version execution was performed.
  7. Thrift/protobuf: no definitions or field IDs changed.
  8. Parallel paths: checked INSERT/static and dynamic OVERWRITE, UPDATE/DELETE/MERGE, effective rowkind events, fixed/dynamic buckets, ORC/per-level formats and recursive values. FE selects JNI; the FFI stub is not selected.
  9. Conditional behavior: key-preserving DML, short MERGE insert rejection, control-column exemption, overwrite padding and ORC/routing exceptions have concrete local rationales. Defaulted CHAR values already use padding SDK casts.
  10. Test coverage: unit and regression additions cover Unicode, NULL/nested values, session opt-out, interoperability, partition replacement, compaction snapshots, rowkind, ORC and MERGE errors. Reversed MAP-descendant entry order is missing and requested inline.
  11. Golden results: all 23 ordered query labels match the 23 output blocks; expected state transitions were checked statically. The author reports generated goldens and passing runs; those claims were not independently executed.
  12. Observability: existing writer/transaction logs and counters remain; new diagnostics identify the affected column/value. No additional observability defect found.
  13. Transactions and persistence: statement binding and existing SDK snapshot/commit reconciliation remain; the backend-only padding option is not persisted to the FE committer's table. No EditLog/replay change.
  14. Atomicity and crashes: padding failures propagate to native abort/close; commit messages are published only after successful prepare/close. M1 concerns logical values read from successfully committed Avro data, not a new commit protocol.
  15. FE/BE variables: explicit true/false travels on the serialized backend table; static filters use the same setting and rowkind exemption.
  16. Performance and memory: padders are constructed once and work is linear in visited values/key bytes; GATHER deliberately trades parallelism for bucket ownership. Untracked padded Java-heap expansion remains the existing P2 above.
  17. Other invariants: internal OLAP visible versions/delete bitmaps and cloud metadata persistence are unaffected. No additional verified issue found after the full sweep.

No additional user focus was supplied. Validation was static only: no builds, tests, executable reproduction or source changes. The documented Spark/Flink ORC-key limitation and already-known compile-time-empty overwrite behavior were not reposted.

The review converged after 2 rounds: all three assigned agents returned NO_NEW_VALUABLE_FINDINGS in the latest round, all changed paths were covered, and all candidates and initial risks have explicit evidence-backed dispositions. No unresolved static coverage question remains.

Review converged after 2 round(s).


/** The bytes of {@code key} as the only field of a row, copied since the serializer reuses its buffer. */
private BinaryRow bytes(Object key) {
return serializer.toBinaryRow(GenericRow.of(key)).copy();

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] Compare MAP descendants independently of entry order

For an API-created Avro column MAP<ROW<c CHAR(2), m MAP<STRING,INT>>,INT>, insert the distinct keys ROW('k', map('a',1,'b',2)) -> 10 and ROW('k ', map('b',2,'a',1)) -> 20. Both padded c values become 'k ', but InternalMapSerializer preserves the nested entry order, so these binary representations stay different and padMapKeys accepts both. On read, GenericMap.equals ignores entry order; the outer Avro ArrayMapReader inserts the now-equal ROW keys into a HashMap and silently drops one value. This differs from the earlier unsupported Arrow object-hashing issue: serialization succeeds, but this new byte comparator disagrees with downstream equality. Canonicalize MAP descendants or compare keys recursively by type, and cover reversed inner-map order in an Avro round trip.

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 359271837c1. This reproduces as described. In an Avro table created through the Paimon API with m MAP<ROW<c CHAR(2), m MAP<STRING, INT>>, INT>, inserting map(named_struct('c', 'k', 'm', map('a', 1, 'b', 2)), 10, named_struct('c', 'k ', 'm', map('b', 2, 'a', 1)), 20) succeeded, and Doris (native and JNI reads) and the Paimon API all read back the single entry {"c":"k ", "m":{"a":1, "b":2}}: 20. A Parquet table kept both entries, that is, two keys that Paimon takes as equal. With padding off both keys are kept, as before this PR, and the same pair with the inner maps in the same order already failed with the intended error.

The check now sorts the entries of every map nested in a key, at any depth (in a ROW, an ARRAY or a MAP), by their key bytes before it serializes the key, so two keys have the same bytes when Paimon's GenericMap.equals takes their maps as equal. Both tables now fail that write with turns the keys '{k, {a -> 1, b -> 2}}' and '{k , {b -> 2, a -> 1}}' into the same key '{k , {b -> 2, a -> 1}}'. Keys that were one key before padding, such as ('k ', {a: 1, b: 2}) and ('k ', {b: 2, a: 1}), are still written as they are; an Avro read keeps one of them with padding on or off, as on master.

Tests: PaimonWriteSchemaTest.testMapsNestedInMapKeysAreComparedRegardlessOfTheirEntryOrder feeds rows read back from an Arrow batch: the reversed inner map collides, directly and inside an array, keys equal before padding are kept, and keys whose inner maps differ are padded and kept. Not sorting the entries, or sorting them by bytes left in the serializer's reused buffer, turns it red. test_paimon_write_char_padding now creates such an Avro table through Spark, fails the reversed pair and pads a single key holding a map; with the paimon-scanner jar from before this commit the suite fails at that write.

@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

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