Repository navigation
[fix](paimon) Pad CHAR values written to Paimon tables as Spark and Flink do - #69083
morningman wants to merge 9 commits into
Conversation
…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>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
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())); |
There was a problem hiding this comment.
[P1] Keep CHAR padding consistent with fixed-bucket routing
Enabling padding here changes routing fields after the native exchange has already hashed their original bytes. PaimonWritePlanProvider.getWriteDistribution still selects paimon_fixed_bucket for CHAR partition/bucket keys before its single-writer fallback. For an unpartitioned CHAR(3) primary key with bucket=4 and two writers, 'k' and 'k ' route to native buckets 2 and 3 (different writers), but both become 'k ' and the SDK writes bucket 1. With changelog-producer=full-compaction and an existing 'k ' row, both writers restore and compact that same bucket, producing overlapping higher-level files that Paimon rejects. 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.
There was a problem hiding this comment.
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 andpaimon_fixed_bucketwith it off.test_paimon_write_char_paddingnow writes a CHAR(3) primary-key table withbucket = 4andchangelog-producer = full-compactionfromnumbers()withparallel_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) { |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
[P2] Account for expanded nested CHAR values before materializing them
This loop retains all padded values on the Java heap before writer.write can apply the managed-page budget. A supported Paimon ARRAY<CHAR(65535)> containing 4096 copies of 'a' arrives as roughly 20 KiB of Arrow data/offsets (CHAR>255 maps to Doris STRING), but the loop retains about 256 MiB of new byte arrays: blankString 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.
There was a problem hiding this comment.
Not changing this. The padded value is the value Paimon stores: an ARRAY<CHAR(65535)> with 4096 'a' elements is a 256 MiB row once padded, Paimon serializes that row when it writes it, and Spark and Flink build the same row in their JVMs. The writer pads one row at a time, and the padded values are garbage once writer.write has taken the row. The amplification comes from the CHAR length the table was declared with; Doris itself does not create a CHAR longer than 255 and reads a longer Paimon CHAR as STRING. Charging the padded bytes to the query would need a new bridge from these Java heap allocations to Doris's memory tracker, which is out of proportion to this case.
TPC-H: Total hot run time: 29605 ms |
TPC-DS: Total hot run time: 152089 ms |
ClickBench: 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>
|
run buildall |
|
/review |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
[P1] Normalize CHAR keys before MERGE matching and cardinality checks
Padding here changes primary-key identity after ConnectorChangelogPlanBuilder.addCardinalityChecks has validated it. With an empty (c CHAR(3), v INT) primary-key target, MERGE source rows (CAST('k' AS CHAR(3)), 1) and (CAST('k ' AS CHAR(3)), 2) using ON t.c = s.c WHEN NOT MATCHED THEN INSERT (c,v) VALUES (s.c,s.v) follow this reduced plan:
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.
There was a problem hiding this comment.
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), andtestShortCharKeysThatMergeDoesNotInsertAreWrittenAsBefore(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_paddingnow runs a MERGE from a Doris table: the short key fails and nothing is written; therpadMERGE inserts'k 'and, run again, matches it; therpadMERGE 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()); |
There was a problem hiding this comment.
[P2] Mirror the rowkind.field exemption in static partition padding
The writer now leaves the column named by rowkind.field unpadded, but this branch still pads it when it is a CHAR partition column. For a valid table (id INT, rk CHAR(3)) with primary key (id,rk), partition key rk, bucket=1 and rowkind.field=rk, INSERT OVERWRITE TABLE t PARTITION(rk='+I') SELECT 1 puts '+I ' in the FE overwrite filter while the BE writes partition '+I'. Paimon rejects that file as outside the specified partition; a SELECT that returns no rows at runtime instead succeeds without clearing the existing '+I' partition. Apply the writer's control-field exemption here too, and cover nonempty and empty static overwrites. This is distinct from the earlier token-padding finding: its BE fix is present, but the FE overwrite filter now disagrees with it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A correction to the second paragraph of my reply above. With 1012317776a this table is no longer written by one writer: its CHAR partition column is the rowkind.field, which the writer never pads, so it is routed natively. And since #69010, fe-core runs the transaction of an external INSERT OVERWRITE even when planning proves its source empty. On a cluster with both, INSERT OVERWRITE TABLE t PARTITION (rk = '+I') SELECT 3 LIMIT 0 now commits an OVERWRITE snapshot and clears the partition, as a source that is empty at run time does; the regression still uses the latter.
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>
|
run buildall |
|
/review |
TPC-H: Total hot run time: 29651 ms |
TPC-DS: Total hot run time: 152669 ms |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
[P1] Resolve rowkind.field before deciding whether to pad keys
An ordinary INSERT starts with GenericRow's INSERT kind, so this branch pads CHAR data keys even when rowkind.field carries -D, -U or +U. The SDK resolves that operation later in TableWriteImpl.writeAndReturn, 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.
There was a problem hiding this comment.
Fixed in f2c45d7e38c. This reproduces: with a key 'k' written with padding off and 'k ' written with it on, INSERT INTO t SELECT c, '-D', v FROM t WHERE c = 'k' deleted 'k ' and left 'k', and the same -D for a key 'j' written unpadded, with no padded twin, deleted nothing. Doris refuses UPDATE, DELETE and MERGE on a table with a rowkind.field, so such an INSERT is how Doris deletes rows of it, with keys read back from the table.
The writer now goes by the kind Paimon will give the row: a row of such a table is padded only when its rowkind.field value is '+I', whether Doris writes that value or the field's default supplies it. A '-D', '-U' or '+U' row carries a key to match, as Doris's own UPDATE and DELETE rows do, and is written as Doris holds it, so both -D rows above now delete the row they were read from. A short key written as a literal in such a row is not padded either, and so does not reach a key that Spark or Flink wrote padded, as on master; it has to be written full length.
Tests: PaimonWriteSchemaTest.testRowKindFieldValueDecidesWhetherARowIsPadded ('+I' rows are padded; '-D', '-U', '+U' and NULL rows are not; a default '+I' or '-D' decides the same way); padding by the row's own kind alone, or looking the value up in the input only, turns it red. test_paimon_write_char_padding deletes an unpadded and a padded key of a CHAR-keyed rowkind.field table with -D rows read back from it.
| value, insertedKeyCharLengths[i], inputType.getFields().get(i).name(), | ||
| insertedKeyCharLengths[i])); | ||
| } | ||
| value = charPadders[i].pad(value); |
There was a problem hiding this comment.
[P1] Preserve ORC CHAR key identity for UPDATE and DELETE
Padding inserted keys breaks subsequent row-level DML on ORC tables because both native and SDK ORC readers strip CHAR trailing spaces. For (c CHAR(3), v INT) with primary-key=c, bucket=4 and file.format=orc, INSERT INTO t VALUES ('k',1) now supplies 'k ' to Paimon's bucket extractor, 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.
There was a problem hiding this comment.
Fixed in f2c45d7e38c. This reproduces, and for most short keys rather than one pair: Paimon stores CHAR(n) as ORC char(n), and ORC pads every CHAR value it writes (CharTreeWriter) and strips the trailing spaces from one it reads (CharTreeReader), as Doris's ORC reader does too. Padding in the writer therefore changed nothing that ORC stores, only the bucket a CHAR key is hashed to, and UPDATE and DELETE, which read the key back without its spaces, hashed another one. On a local cluster, in an ORC table (c CHAR(3), v INT) keyed by c with bucket = 4, DELETE ... WHERE v IN (1, 3, 4, 5) of the keys 'k', 'x', 'm' and 'zz' that Doris had just inserted returned COMMITTED and removed none of them. With padding off, and in a Parquet table with padding on, it removed all four.
The writer now pads only the partition columns of a table that writes ORC files, by file.format or at some level of file.format.per.level (PaimonWriteSchema.writesOrcFiles). A partition value is read back as it is stored, so padding it still makes Doris and Spark write one partition, and UPDATE and DELETE of a CHAR-partitioned ORC table were already correct. The ORC files hold the same bytes as before, and a key Doris writes into such a table gets the bucket it had before this PR, so UPDATE, DELETE and MERGE reach it, as they reach the keys an earlier Doris wrote; MERGE's short-key check does not apply to a key that is not padded. While padding is on, FE still writes a fixed-bucket table with a CHAR key through one writer, since it does not look at the file format; that only costs parallelism. One case stays as it is on master: a short CHAR key that Spark or Flink wrote padded into a multi-bucket ORC table sits in another bucket than the one Doris's UPDATE and DELETE hash 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), |
There was a problem hiding this comment.
[P2] Reject padded map-key collisions that Avro cannot preserve
Retaining both padded key-array entries does not preserve them through the supported Avro format. For a table with m MAP<CHAR(2), INT> and file.format=avro, inserting map('k',1,'k ',2) turns the two distinct keys into two identical 'k ' keys here. Paimon's Avro writer emits both entries, but its generic MapReader 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.
There was a problem hiding this comment.
Fixed in f2c45d7e38c. This reproduces: map('k', 1, 'k ', 2) written into a MAP<CHAR(2), INT> column read back as {"k ":2} from an Avro table, and as a map holding the key 'k ' twice from a Parquet one. A map holds one entry per key, and padding is what made these two keys one, so the writer now fails such a row (PaimonWriteSchema.padMapKeys). The message names the column and the two keys, and asks for distinct keys or for padding to be turned off. Keys that were equal before padding are kept as they are, as before, and keys that stay distinct are padded. Spark, with the paimon-spark extensions, rejects the same insert with [DUPLICATED_MAP_KEY] Duplicate map key k was found. An ORC table, which is no longer padded (see the ORC thread), gets both keys as before; ORC pads them itself, as on master.
Tests: PaimonWriteSchemaTest.testMapKeysThatPaddingMakesEqualFailTheRow; removing the check, or failing keys that were equal before padding too, turns it red. test_paimon_write_char_padding writes such a map into an Avro table, which fails with padding on and keeps both keys with it off, and a map of distinct keys, which is padded. With the jars from before this commit the suite fails at that check, its first new one.
ClickBench: Total hot run time: 24.53 s |
TPC-H: Total hot run time: 29584 ms |
TPC-DS: Total hot run time: 152207 ms |
ClickBench: Total hot run time: 24.52 s |
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
…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>
|
run buildall |
TPC-H: Total hot run time: 29234 ms |
TPC-DS: Total hot run time: 152586 ms |
ClickBench: Total hot run time: 24.79 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
… 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>
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of #69083 at 359271837c1645906ec22bc27e06d34acaa66198 covered all 12 changed files and their planning, Arrow, SDK write and commit paths. One new P2 finding is inline: opposite-sign NaNs in ROW map keys bypass CHAR-padding collision detection and can lose an entry on Avro readback. Review converged after 2 rounds; no unresolved candidates or uncovered changed files remain.
The four earlier P1 findings (fixed-bucket routing, short-key MERGE, rowkind mutation keys and ORC mutation identity) are addressed at this head; no existing P0/P1 blocking IDs remain applicable. Earlier control/default/overwrite/Arrow/map-order scenarios also have fixes. The previously reported P2 untracked expanded Java heap allocation still applies and is not reposted. No additional user review focus was supplied.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and correctness | Recursive CHAR padding, control handling and partition alignment implement the intended write behavior; the inline NaN collision is a remaining correctness bug. |
| Scope and clarity | Changes are localized to Paimon normalization, binding/distribution, one session option and associated tests. |
| Concurrency and locking | Mutable padding helpers are confined to each synchronous JNI writer; no new shared lock or thread was introduced. |
| Lifecycle and initialization | Arrow views remain valid through synchronous SDK consumption; retained dynamic-index keys are copied. No new static initialization dependency. |
| Configuration | The default-on session boolean is read per statement and changes without restart. |
| Compatibility | An absent backend option preserves old-FE/new-BE behavior. Supported BE-before-FE upgrade order is respected; new-FE/old-BE pairing can fail CHAR static overwrites and is outside that order. No new storage format or symbol. |
| Thrift/protobuf IDs | No definitions changed. |
| Parallel paths | Fixed, dynamic and global bucket writes normalize before assignment; CHAR native routing falls back appropriately, with explicit ORC, mutation, rowkind and MERGE policies. |
| Conditional checks and errors | Concrete failure scenarios are documented; new rejection paths propagate through existing JNI failure/abort handling. |
| Test coverage | Added unit/e2e cases cover Unicode, nested values/maps, switches, defaults/controls, overwrite, routing/compaction, ORC and MERGE. The signed-NaN case is missing. |
| Test results | All 24 ordered regression labels match the golden sections; expected rows are consistent on static inspection. Runtime results and golden generation were not independently executed. |
| Observability | Existing writer/transaction context and column-specific failure messages cover the localized behavior. |
| Persistence and failover | Existing binding, commit, abort and reconciliation mechanisms remain intact; no EditLog, visible-version or delete-bitmap change. |
| Write atomicity and crash handling | New validation failures use existing abort/cleanup paths. The inline issue concerns committed map contents, rather than a new publication protocol. |
| FE/BE variable passing | Reflection-based session extraction/forwarding and the backend-only serialized table option reach all selected write modes. |
| Performance and memory | Padders are built once per schema; nested-map sorting and CHAR routing consolidation have explicit costs. The existing untracked-heap P2 remains a limit. |
| Other concerns and coverage limits | Defaults, per-level format normalization, partition readback, prepared-statement replanning, duplicate threads and upstream equality were checked. Static-only review: no builds or tests were run and repository sources were not modified. |
Review converged after 2 round(s).
| */ | ||
| private BinaryRow bytes(Object key) { | ||
| Object sorted = key == null || entrySorter == null ? key : entrySorter.sort(key); | ||
| return serializer.toBinaryRow(GenericRow.of(sorted)).copy(); |
There was a problem hiding this comment.
[P2] Canonicalize NaNs before comparing padded ROW map keys
For an Avro MAP<ROW<c CHAR(2), d DOUBLE>, INT> column, read VARCHAR source fields containing 'NaN' and '-NaN', and construct keys named_struct('c','k','d',CAST(pos AS DOUBLE)) -> 10 and named_struct('c','k ','d',CAST(neg AS DOUBLE)) -> 20 in INSERT ... SELECT. These runtime casts preserve different NaN signs through Arrow. Padding makes both CHAR fields 'k ', but Paimon's serializer preserves raw NaN bits, so this byte comparison accepts both keys. On read, GenericRow equality/hash treats these NaNs as equal, and Avro's ArrayMapReader inserts the now-equal keys into a HashMap, silently dropping one value. Before padding the CHAR fields differ and both entries survive. Canonicalize NaNs in comparison copies or use matching semantic equality, and cover this Arrow-to-Avro case. Unlike the existing MAP-order thread, this key has no MAP descendant for entrySorter to normalize.
There was a problem hiding this comment.
Fixed in c4acf1b45e1. This reproduces as described. In an Avro table created through the Paimon API with m MAP<ROW<c CHAR(2), d DOUBLE>, INT>, INSERT ... SELECT 1, map(named_struct('c', 'k', 'd', cast(pos AS DOUBLE)), 10, named_struct('c', 'k ', 'd', cast(neg AS DOUBLE)), 20) from a Doris table holding 'NaN' and '-NaN' succeeded, and Doris (native and JNI reads) read back the single entry {"c":"k ", "d":NaN}: 20; a FLOAT field behaves the same. A Parquet table kept both entries, two keys that Paimon takes as equal. With padding off both keys are kept, as before this PR, and the same pair with two positive NaNs already failed with the intended error.
Before the check serializes a key, it now also makes each FLOAT and DOUBLE NaN in the key, at any depth, the canonical NaN, besides sorting the entries of its nested maps. -0.0 stays apart from 0.0, as Double.equals keeps them apart for Paimon. All three tables now fail that write with turns the keys '{k, NaN}' and '{k , NaN}' into the same key '{k , NaN}'. Keys that were one key before padding, such as ('k ', NaN) and ('k ', -NaN), are still written as they are; an Avro read keeps one of them with padding on or off, as on master.
Tests: PaimonWriteSchemaTest.testNaNsInMapKeysAreComparedRegardlessOfTheirBits feeds rows read back from an Arrow batch: NaNs of opposite signs collide in a DOUBLE field, a FLOAT field and a nested ROW, keys equal before padding are kept, and ('k', 0.0) and ('k ', -0.0) are padded and kept. Not normalizing the NaNs, or only the DOUBLE ones, or making -0.0 into 0.0, turns it red. test_paimon_write_char_padding now creates such an Avro table through Spark, fails the pair cast at run time from a Doris table and pads a single key holding a NaN; with the paimon-scanner jar from before this commit the suite fails at that write.
TPC-H: Total hot run time: 29615 ms |
TPC-DS: Total hot run time: 153042 ms |
ClickBench: Total hot run time: 24.62 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
The map key collision check compared the padded keys by their serialized
bytes, and a ROW keeps the bits of a NaN when it is serialized, while
Paimon compares a FLOAT or DOUBLE as Double.equals does, taking every
NaN as one value, and its Avro reader puts the keys of a map in a hash
map. A query that casts 'NaN' and '-NaN' at run time keeps their signs
through the Arrow batch, so in a MAP<ROW<c CHAR(2), d DOUBLE>, INT>
column the keys ('k', NaN) and ('k ', -NaN), which padding makes one
key, passed the check: an Avro table read back one entry and silently
dropped the other value, and a Parquet table held two equal keys. Each
NaN in a key, at any depth, is now made the canonical NaN before the key
is compared, so such keys fail the row like other keys that padding
makes equal. Keys that were equal before padding are kept as before,
and -0.0 stays apart from 0.0, as it does for Paimon.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed PR #69083 at c4acf1b. All 12 changed files and their relevant upstream/downstream paths were inspected. Two new P2 findings are inline: an Avro outer-map value can still be lost after descendant-map key normalization, and unchanged ORC CHAR bucket keys unnecessarily lose parallel writer routing. Neither finding duplicates the original scalar-NaN, unique-map-order, or changed-routing-byte threads.
The review converged in round 2: all three assigned passes returned NO_NEW_VALUABLE_FINDINGS against this unchanged two-comment set. Every candidate and initial risk has an explicit disposition, and the final sweep found no unresolved question or uncovered changed file. This is a static review; no builds or tests were run.
The four previously reported P1 findings (4273514658, 4274646965, 4276034908, 4276034922) no longer apply to this head: CHAR routing falls back where padding changes routing bytes, MERGE rejects short inserted CHAR primary keys, effective rowkind/default/case semantics preserve mutation keys, and ORC nonpartition keys remain unpadded. No existing P0/P1 is carried forward. Other original correctness examples are addressed; the existing P2 Java-heap amplification concern (4273514673), which the author declined to change, remains known context and is not reposted.
Critical checkpoint conclusions:
- Goal and proof: the intended change is write-side CHAR interoperation with Spark/Flink plus a legacy opt-out. Scalar/nested/Unicode, static partition, mutation and overwrite cases are covered in the added tests; the two inline findings identify remaining behavior and coverage gaps.
- Scope and abstraction: changes stay within the writer/schema, Paimon binding/planning, one session variable and tests. Recursive padders and comparison normalization are isolated and created per schema.
- Concurrency and locks: new mutable serializers/default wrappers belong to one writer and execute synchronously in writeBatch. No shared lock, lock order or new thread is introduced; existing compaction workers consume SDK-owned data.
- Lifecycle: imported Arrow roots outlive synchronous row consumption; comparison bytes are copied before serializer reuse. Existing prepare/abort/close paths and statement-scoped binding ownership are retained; no global initialization dependency is added.
- Configuration: enable_paimon_write_char_padding defaults true, is session-mutable and needForward=true, is extracted into ConnectorSession, and controls both planning and the BE-bound table option.
- Compatibility: no ABI or storage-layout migration. Missing option means false for an old FE with a new BE; existing mutation keys and the opt-out remain supported. The mixed-version analysis follows the documented BE-before-FE upgrade order. Read-side CHAR comparison is intentionally unchanged.
- Thrift/protobuf and SPI: no definitions, numeric ids or shared connector API surface change; the setting uses existing serialized-table transport.
- Parallel paths: reviewed fixed/hash-dynamic/key-dynamic writes, INSERT/UPDATE/DELETE/MERGE, static/dynamic/full overwrite, rowkind control/default/case handling, ORC/per-level formats, and nested ARRAY/MAP/ROW paths. The inline findings cover the residual map-equivalence and ORC parallelism cases.
- Conditions and errors: short-MERGE-key, map-collision, rowkind and ORC exclusions have concrete triggers and explanations. Invalid rowkind parsing is deferred to the SDK's authoritative validation; write errors propagate to statement failure.
- Test coverage: unit and regression additions exercise option transport, padding on/off, Unicode, nested values, key collisions, legacy mutations, rowkind behavior, MERGE guards, ORC and compaction snapshots. The two inline scenarios need additional coverage.
- Expected results: all 25 regression query tags match golden labels and their expected rows were traced through the statements. Golden generation and author-reported test runs were not independently executed or treated as test evidence.
- Observability: new diagnostics identify the column/key and corrective action. Existing JNI/commit error context remains; no new high-rate logging or critical service metric is needed.
- Persistence and FE failover: no new EditLog state, durable catalog mutation or transaction recovery format; the extra option is confined to the serialized writer table. Existing commit/reconciliation behavior is unchanged.
- Write atomicity and crash handling: normalization/validation precede SDK row writes and successful FE commit; failures use existing abort/resource cleanup. Native visible-version and MoW delete-bitmap mechanisms are untouched.
- FE/BE propagation: the option spelling matches both modules; the writer receives primary/partition keys, rowkind field, format and overwrite status on the common creation path. No separate constant-fold or point-query parameter path is introduced.
- Performance and memory: padders reuse schema/type decisions and borrow unchanged descendants; comparison copies own their bytes. The ORC gather regression is reported inline without claiming measured throughput. Existing expanded Java-heap allocation remains the already-reported concern noted above.
- Other concerns and limits: review_focus.txt supplies no additional focus. No unresolved candidate or changed-file coverage gap remains after the static review. Builds, tests, package checks and live Doris/Avro execution were not run, as the review contract prohibits them; no source code was modified.
Review converged after 2 round(s).
| order[i] = i; | ||
| } | ||
| Arrays.sort(order, (left, right) -> compareBytes(keyBytes[left], keyBytes[right])); | ||
| Object[] sortedKeys = new Object[keys.length]; |
There was a problem hiding this comment.
[P2] Reduce duplicate descendant keys before comparing padded map keys
For an Avro MAP<ROW<c CHAR(2), m MAP<DOUBLE,INT>>,INT>, insert outer keys ROW('k',map(CAST(pos AS DOUBLE),1,CAST(neg AS DOUBLE),2)) -> 10 and ROW('k ',map(CAST(pos AS DOUBLE),2)) -> 20, reading VARCHAR pos='NaN', neg='-NaN' at run time. Doris's map() deduplicates serialized bytes, so the signed NaNs remain separate through Arrow. This normalizer canonicalizes their bits but keeps both entries, so the padded keys still compare differently (two inner entries versus one). Paimon's Avro reader reduces the first inner map to {NaN:2}, then considers the outer keys equal and drops value 10. With padding off their CHAR fields differ and both outer values survive. This differs from the existing scalar-NaN and entry-order threads: equal descendant keys need last-wins reduction in the comparison copy before sorting/serialization, or equivalent collision rejection. Cover this two-entry versus one-entry case through Arrow and Avro.
There was a problem hiding this comment.
Fixed in 41d351b3b90. This reproduces as described. In an Avro table created through the Paimon API with m MAP<ROW<c CHAR(2), mm MAP<DOUBLE, INT>>, INT>, INSERT ... SELECT 1, map(named_struct('c', 'k', 'mm', map(cast(pos AS DOUBLE), 1, cast(neg AS DOUBLE), 2)), 10, named_struct('c', 'k ', 'mm', map(cast(pos AS DOUBLE), 2)), 20) from a Doris table holding 'NaN' and '-NaN' succeeded, and Doris read back the single entry {"c":"k ", "mm":{NaN:2}}: 20; a Parquet table kept both entries, two keys that Paimon takes as equal. With padding off both entries are kept, as before this PR.
The copy of a nested map that the check compares now keeps, once its NaNs are canonical and its entries sorted by their key bytes, only the last of the entries whose keys are equal, the one whose value Paimon's Avro reader keeps (ArrayMapReader puts the entries into a HashMap in order). Both tables now fail that write with turns the keys '{k, {NaN -> 1, NaN -> 2}}' and '{k , {NaN -> 2}}' into the same key '{k , {NaN -> 2}}'. Keys whose nested maps keep other last values, such as (k, {NaN: 1, -NaN: 2}) and (k , {NaN: 1}), are padded and written, and read back as two entries; the nested maps are written whole, as before.
Tests: PaimonWriteSchemaTest.testMapsNestedInMapKeysAreComparedByTheLastEntryOfEachKey feeds rows read back from an Arrow batch: the pair above collides, keys that were one key before padding are kept, and keys whose nested maps keep other last values are padded, with their nested maps whole. Not reducing the nested entries, keeping the first of them instead of the last, or comparing the nested maps by their keys alone turns it red. test_paimon_write_char_padding creates such an Avro table through Spark, fails the pair cast at run time from a Doris table and pads the other pair; with the paimon-scanner jar from before this commit the suite fails at that write.
| private boolean supportsNativeRouting(DataTypeRoot type, boolean charPadding) { | ||
| switch (type) { | ||
| case CHAR: | ||
| return !charPadding; |
There was a problem hiding this comment.
[P2] Preserve native routing for unpadded ORC CHAR bucket keys
For an unpartitioned ORC table (c CHAR(3), v INT) with primary key c, bucket=4 and no key default, PaimonWriteSchema.create installs no padders: ORC's nonpartition fields are exempt. The new default-on flag nevertheless rejects its CHAR bucket key here, making fixedBucketDistribution return null and requiresSingleWriter select GATHER. A distributed INSERT ... SELECT therefore goes through one sink instance instead of the existing parallel bucket writers, even though none of its routing bytes changes. This differs from the earlier routing thread, whose padded fields actually changed bucket identity. Make routing eligibility follow the writer's per-field padding eligibility, preserving native routing for this unchanged ORC case, and add an ORC padding-on distribution test.
There was a problem hiding this comment.
Fixed in 1012317776a. Confirmed: for a primary-key ORC table (c CHAR(3), v INT) with bucket = 4, EXPLAIN SHAPE PLAN INSERT ... SELECT ... FROM numbers("number" = "100") planned DistributionSpecGather with padding on and DistributionSpecExternalTableSinkHashPartitioned with it off, though the writer leaves that key unpadded.
FE now decides it per routing field by the writer's rule, in PaimonWriteBinding.charPaddedFields: with padding on, the writer pads every CHAR field but the table's rowkind.field, and of a table that writes ORC files, by file.format or at some level of file.format.per.level, only the partition fields. A CHAR routing field it pads still falls back, as before, and the others are routed natively. The static partition values follow the same rule, which leaves them as they were. The remark in r4279897764 that FE does not look at the file format no longer holds. With padding on, the ORC table above and one with file.format.per.level = '5:orc' now plan DistributionSpecExternalTableSinkHashPartitioned, while a Parquet table and an ORC table partitioned by a CHAR field still plan DistributionSpecGather. Writing the ORC table with changelog-producer = full-compaction from numbers() with four writers commits an APPEND and a COMPACT snapshot with 100 changelog records, with no LSM conflict, and a DELETE and an UPDATE then reach its keys.
Tests: PaimonWritePlanProviderTest.charRouteFieldsThatTheWriterLeavesUnpaddedAreRoutedNatively routes, with padding on, the CHAR key of an ORC table, by file.format and by file.format.per.level, and a CHAR partition that is the rowkind.field natively, and the CHAR partition of an ORC table through one writer; PaimonWriteBindingTest.staticCharPartitionOfAnOrcTableIsPaddedAsTheRowsAre keeps the static value of that partition padded. Falling back for every CHAR routing field, ignoring file.format.per.level or the rowkind.field, or routing the partition of an ORC table natively and leaving it unpadded turns them red. test_paimon_write_char_padding checks the plan of such a full-compaction ORC table, next to the Parquet one's, and its $snapshots after a four-writer write; with the fe-connector-paimon jar from before this commit the suite fails at that plan.
TPC-H: Total hot run time: 29553 ms |
TPC-DS: Total hot run time: 152471 ms |
ClickBench: Total hot run time: 24.47 s |
…f each key
The map key collision check sorted the entries of a map nested in a key
before it compared the keys by their bytes, but kept every entry, while
Paimon's Avro reader puts the entries of a map in a hash map, which keeps
the value of the last of the entries with equal keys. Doris's map() keeps
NaN and -NaN apart, since their bits differ, so in a
MAP<ROW<c CHAR(2), mm MAP<DOUBLE, INT>>, INT> column the keys
('k', {NaN: 1, -NaN: 2}) and ('k ', {NaN: 2}), which padding makes one key
once the first nested map is read back as {NaN: 2}, passed the check: an
Avro table read back one entry and silently dropped the other value. The
copy of a nested map that the check compares now keeps only the last of
the entries whose keys are equal, so such keys fail the row like other
keys that padding makes equal. Keys that were equal before padding are
kept as before, and the nested maps are written whole.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…atively FE kept every CHAR routing field off the native bucket route while padding was on, but the writer does not pad every CHAR field: it leaves the table's rowkind.field alone, and of a table that writes ORC files it pads only the partition fields, since ORC pads every CHAR value it writes. A fixed-bucket primary-key ORC table with a CHAR bucket key was therefore written through a single writer, though BE hashes the bytes the writer writes. FE now decides it per field, by the writer's rule, which the static partition padding shares: such a table keeps its parallel bucket writers, and a CHAR field that the writer pads still falls back as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all 12 changed paths at 1012317776ab401cbf6f76ca82fd72a8431dd49b: no additional verified inline findings. The existing P1 instances for fixed-bucket routing (4273514658), MERGE short keys (4274646965), ORC mutation identity (4276034922), and effective row-kind decoding (4276034908) are addressed at this head; no existing P0/P1 blocker is carried forward.
The existing P2 heap-accounting concern still applies: expanded CHAR values and comparison copies allocate Java heap before managed writer accounting. It is acknowledged here without another inline comment. Other existing P2 instances have corresponding repairs and targeted tests.
The reviewed write flow is session setting -> FE distribution/static-partition binding -> backend-only table option -> JNI/Arrow row mapping and padding -> SDK write/prepare -> FE commit. Conclusions for the required checkpoints:
- Goal and evidence: scalar and nested inserted CHAR values pad by code points, with documented mutation, control-field and ORC exemptions. Unit and regression coverage exercises these policies; tests were inspected, not executed.
- Scope and reuse: changes stay within Paimon write policy, its session switch and tests, and reuse SDK row-kind/default conversion, typed serializers and casts. Connector boundaries and shared SPI remain unchanged.
- Concurrency and locks: each local writer owns its schema; synchronous Arrow consumption does not race SDK compaction. Restored native routes use unchanged bytes; padded routing fields fall back appropriately. No new lock or lock-order change found.
- Lifecycle and initialization: Arrow views remain valid during synchronous consumption; comparison bytes are copied before serializer reuse. Existing shutdown releases schema/adapter after SDK users stop. No new static initialization dependency.
- Configuration: the default-true switch is captured for each statement, supports changes without restart and is forwarded through the annotated generic session-variable path.
- Compatibility: no JNI signature or storage-format definition changes. An absent option preserves old FE behavior on new BE. New-FE/old-BE static-filter mismatch was considered and dismissed under the documented BE-before-FE upgrade sequence; this review does not assert support for the reverse order.
- Thrift/protobuf: no definitions or field IDs changed.
- Parallel paths: INSERT, OVERWRITE and changelog writes share option transport; fixed, dynamic and unaware bucket modes, ORC/per-level formats, rowkind controls, partial/default inputs and static empty overwrites were checked.
- Conditions: field exemptions, effective row-kind/default decoding, overwrite padding and MERGE short-key rejection have concrete documented reasons and match the FE static-filter policy.
- Test coverage: added unit tests cover Unicode, NULL, overlength, nested/Arrow keys, collisions, NaNs, descendant map order/last-wins reduction, switch behavior, routing and schema copies. Regression cases cover Spark interoperability, DML, MERGE, partition overwrite and full-compaction snapshots, including negative cases.
- Expected results: all 29 regression query labels pair with output blocks and agree with the traced statements. Runtime correctness and automatic output-generation provenance were not independently reproduced.
- Observability: validation errors identify affected columns/keys and remedies; existing JNI/transaction error context and writer counters remain adequate for this local transformation.
- Persistence and failover: no EditLog or persisted table-schema change; FE retains its catalog-aware committer table and existing transaction/reconciliation protocol.
- Atomicity and crashes: validation exceptions propagate to the native failure/abort path; commit messages publish only after successful preparation and shutdown. No additional atomicity regression found in the changed flow.
- FE/BE passing: the identical option key reaches every JNI write mode through the serialized table; overwrite options survive the backend-only schema copy.
- Performance and memory: policy structure is precomputed per writer; native parallelism remains for unchanged ORC/control fields. Nested-map comparisons require copying and sorting. The known untracked heap expansion remains the limitation noted above.
- Further review and user focus: defaults, nested equality, serializer ownership, error cleanup and supported mixed-version sequencing were rechecked. No additional user focus was supplied, and no unread changed path or unresolved code candidate remains.
Validation limits: static analysis only, as required; no builds, unit/regression tests, source changes or cluster execution were performed. Author-reported runtime results are not presented as independently verified.
Review converged after 2 round(s).
TPC-H: Total hot run time: 29130 ms |
TPC-DS: Total hot run time: 151793 ms |
ClickBench: Total hot run time: 24.59 s |
FE UT Coverage ReportIncrement line coverage |
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_paddingthat 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 byPaimonWriteSchema.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:
'k'into a CHAR(3) primary key, then the Paimon API writes'k ', which is what Spark and Flink write for'k'[k],[k ][k ]['a'],map('k', 'v')andnamed_struct('code', 'a', 'n', 1)intoARRAY<CHAR(3)>,MAP<CHAR(2), CHAR(3)>andROW<code CHAR(3), n INT>[a],[k] -> [v],[a][a ],[k ] -> [v ],[a ]INSERT OVERWRITE ... PARTITION (p = 'x')andINSERT ... VALUES (2, 'y')into a CHAR(3) partition column[x],[y][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.
PaimonWriteSchemapads 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 neitherhashCode,equalsnortoString. Before a key is compared, each NaN in it is made the canonical NaN, and the entries of each map nested in it are sorted by their key bytes, keeping only the last of the entries whose keys are then equal: Paimon takes NaNs of any bits as one value and compares maps regardless of the order of their entries, and its Avro reader keeps the value of the last of the entries with equal keys, while the bytes of a ROW keep the bits of a NaN and Doris'smap()keeps NaN and -NaN apart.'-D','-U'and'+U'rows of a table with arowkind.fieldpadded, 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.'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 withrpad, 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.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, byfile.formator at some level offile.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.PaimonWriteBinding.resolveStaticPartitionpads 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 theirrowkind.fieldvalue, and a'+U'row ofINSERT OVERWRITE ... PARTITION (p = 'x')lands in'x '.paimon_fixed_bucket). BE hashes the unpadded value while Paimon buckets the padded row, so with padding on a CHAR partition or bucket field that the writer pads is not routed natively, and the table gets the distribution of a routing field BE cannot hash: one writer for a primary-key table or one that compacts. Otherwise several writers would write and compact one bucket, and Paimon would give up committing the compaction, and with it the changelog of a full-compaction table, after committing the rows. FE decides it per field by the writer's rule (PaimonWriteBinding.charPaddedFields, which the static partition values use too), so a CHAR field that the writer leaves unpadded, therowkind.fieldor a field other than a partition field of a table that writes ORC files, is routed natively as before. With padding off the route is unchanged.rowkind.fieldis 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 ownDefaultValueRowandRowKindGenerator:'+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: afterINSERT 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 inINSERT INTO t SELECT c, '-D', v FROM t WHERE ....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 optiondoris.write.char-paddingon the table it serializes for the writer, the table that already carries the write options FE sets, such asdynamic-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; therowkind.fieldcolumn 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, byfile.formatorfile.format.per.level, padded in its partition values only; reading the option; a CHAR primary-key value shorter than the column in a row that MERGE inserts fails ('k','k ','中中'), while a full-length or longer key, the rows of UPDATE and DELETE, padding off and a plain INSERT do not; map keys read from an Arrow batch, a ROW holding a ROW padded and colliding ROW, ARRAY and ROW-with-ARRAY keys failing with the keys printed; keys whose nested maps differ only in the order of their entries colliding, directly and inside an array, while keys equal before padding are kept; keys holding NaNs of opposite signs colliding, in a DOUBLE field, a FLOAT field and a nested ROW, while such keys equal before padding are kept and -0.0 stays apart from 0.0; keys whose nested maps are one map once each keeps the last of its entries with equal keys colliding ((k, {NaN: 1, -NaN: 2})and(k , {NaN: 2})), while keys whose nested maps keep other last values are padded, with their nested maps written whole.PaimonWriteBindingTest.staticCharPartitionIsPaddedAsTheRowsAre,PaimonWriteBindingTest.staticRowKindFieldPartitionIsLeftUnpaddedAsTheRowsAre,PaimonBackendBoundTableTest.writeBindingTellsOnlyTheBackendWhetherToPadCharValues,PaimonWriteBindingTest.staticCharPartitionOfAnOrcTableIsPaddedAsTheRowsAre,PaimonWritePlanProviderTest.charPaddingFollowsTheSessionVariableAndDefaultsToOn,PaimonWritePlanProviderTest.charRouteFieldsFallBackToSingleWriterWhenCharPaddingIsOn(a CHAR primary key or partition key: one writer with padding on,paimon_fixed_bucketwith it off) andPaimonWritePlanProviderTest.charRouteFieldsThatTheWriterLeavesUnpaddedAreRoutedNatively(with padding on, the CHAR primary key of an ORC table, byfile.formatorfile.format.per.level, and a CHAR partition key that is therowkind.field:paimon_fixed_bucket; the CHAR partition key of an ORC table: one writer).rowkind.fieldcolumn 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, ignoringfile.format.per.level, padding by a row's own kind regardless of itsrowkind.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 withtoString, comparing keys without sorting the entries of their nested maps, comparing them without making their NaNs the canonical NaN, or only their DOUBLE ones, making -0.0 into 0.0, comparing keys without reducing the entries of their nested maps to one per key, keeping the first of those entries instead of the last, comparing nested maps by their keys alone, falling back for every CHAR routing field while padding is on, ignoringfile.format.per.levelor therowkind.fieldwhen routing, routing the CHAR partition field of an ORC table natively, and leaving the static partition value of an ORC table unpadded.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 = 3removes'u'andUPDATE ... SET v = 40 WHERE v = 4updates'w'in place, with no padded duplicate. More cases, with padding on:INSERT ... SELECT ... FROM numbers("number" = "100")with four writers into a CHAR(3) primary-key table withbucket = 4andchangelog-producer = full-compactionLSM conflicts detected, thenFile deletion conflicts detected) and the table has only APPEND snapshots, without changelogDistributionSpecGather); each INSERT commits an APPEND and a COMPACT snapshot with its changelogDistributionSpecGather)DistributionSpecExternalTableSinkHashPartitioned); an APPEND and a COMPACT snapshot with 100 changelog records, and a DELETE and an UPDATE then reach the keysINSERT ... VALUES (3, '+I', 'c', 30)into a table whoserowkind.fieldisrk CHAR(3)Unsupported short string '+I ' for row kind'-D'row deletes its keyMERGE 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 byc CHAR(3)'k ': the duplicate-insert check is bypassedrpador to turn padding off; withrpadin ON and in the inserted values, the duplicate-insert check rejects the two rows('k', 9)into a table holding'k ''k 'that it did not matchrpad, a MATCHED clause updates'k 'INSERT OVERWRITE TABLE t PARTITION (rk = '+I') ...into a table partitioned by itsrowkind.fieldrk CHAR(3), holding two'+I'rowsSELECT 3fails (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 rowsSELECT 3replaces the partition with one row; the empty source clears itDELETE ... WHERE v IN (1, 3, 4, 5)of four of six short keys that Doris inserted into an ORC table keyed byc CHAR(3)withbucket = 4'k 'and the like, DELETE reads'k'back and hashes another bucketINSERT INTO t SELECT c, '-D', v FROM t WHERE c = 'k'into a table keyed byc CHAR(3)withrowkind.field, holding'k'(written with padding off) and'k ''k 'and leaves'k'; for a key'j'with no padded twin, deletes nothing'k', and a'-D'read back from'k 'deletes'k 'INSERT ... VALUES (1, map('k', 1, 'k ', 2))into aMAP<CHAR(2), INT>column of an Avro table'k 'keys, read back as{"k ":2}map('k', 1, 'kk', 2)is written paddedINSERT ... VALUES (1, map(named_struct('c', 'k', 'r', named_struct('i', 1)), 7))into aMAP<STRUCT<c: CHAR(2), r: STRUCT<i: INT>>, INT>columnColumnarRowData do not support hashCode{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 byc CHAR(3)whoserowkind.fieldisrk STRING DEFAULT '+I''k'and'j'unpadded,'k 'and'j 'padded'k 'and'j 'INSERT OVERWRITE TABLE t PARTITION (p = 'x') SELECT 1, '+U', 9into a table partitioned byp CHAR(3)with a separaterowkind.fieldTrying to overwrite partition {p=x }, but the changes in p=x/ does not belong to this partition'x '; a'+I'and a'-D'of one key in an overwrite ofp = 'y'cancel outRegression. 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 fromnumbers()withparallel_pipeline_task_num = 4and checks its$snapshots(an APPEND, then a COMPACT with 100 changelog records), and therowkind.fieldtable. It runs the MERGE cases above against a Doris source table (the failing MERGE writes nothing, therpadMERGE 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 CHARrowkind.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 withbucket = 4, writes a CHAR-keyed full-compaction ORC table withbucket = 4fromnumbers()with four writers, checking that it is routed natively (DistributionSpecExternalTableSinkHashPartitioned, where the Parquet table above getsDistributionSpecGather) and that both snapshots are committed, deletes an unpadded and a padded key of a CHAR-keyedrowkind.fieldtable with'-D'rows read back from it, and writes a map whose keys padding makes equal into an Avro table, which fails with padding on and keeps both keys with it off. It also writes a map with a ROW-of-ROW key and fails a pair of such keys that padding makes equal, writes a map with a key holding a map into an Avro table and fails a pair of keys that differ only in the order of that map's entries once padded, does the same with a key holding a DOUBLE and a pair of keys holding NaNs of opposite signs, cast at run time from a Doris table, and with a key holding a map with DOUBLE keys and a pair of keys whose nested maps are one map once each keeps the last of its entries with equal keys, writes'+i'and a NULL underDEFAULT '+I'into arowkind.fieldtable and then'+I'rows of the same keys (one padded row per key), and overwrites a CHAR partition of a table with a separaterowkind.fieldwith a'+U'row and with a'+I'and a'-D'of one key. Its.outwas 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 1instead of[k ] 3 1), with those from before the routing fix it fails at the$snapshotscheck, with no COMPACT snapshot, with those from before the MERGE fix at the MERGE that must fail, with those from before the ORC,rowkind.fieldand map fix at the map that must fail, with the paimon-scanner jar from before the commit that compares map keys by bytes at the ROW-of-ROW map key (ColumnarRowData do not support hashCode), with the one from before the commit that sorts nested map entries at the pair of keys whose nested maps differ only in their entry order, with the one from before the commit that compares NaNs regardless of their bits at the pair of keys holding NaNs of opposite signs, and with the one from before the commit that keeps the last entry of each key at the pair of keys whose nested maps keep the last entry; each of those jars writes its pair. With the fe-connector-paimon jar from before the last commit it fails at the route of the ORC table (DistributionSpecGather). The other suites underexternal_table_p0/paimon/writepass on the same cluster, excepttest_paimon_write_thread_lifecycle, which reads BE's/api/be_process_thread_numand 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 variableenable_paimon_write_char_paddingto 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 withrowkind.fieldis padded only when Paimon decodes that field as an insert ('+I'in any case, or its default), except in an overwrite, which pads every row, so a'-D'row written with a short literal key no longer deletes a key that Doris wrote padded: write the key at full length, or read it back from the table; and a map whose CHAR keys padding would make equal fails the write. While padding is on, a fixed-bucket table is written by one writer when a partition or bucket key is a CHAR column that the writer pads, which leaves out therowkind.fieldand, in a table that writes ORC files, the columns other than partition columns, and a MERGE that inserts a CHAR primary-key value shorter than the column fails: pad the key in the MERGE withrpad, or turn padding off.Check List (For Author)
Test
test_paimon_write_char_padding, as above.PaimonWriteSchemaTest,PaimonWriteBindingTest,PaimonBackendBoundTableTestandPaimonWritePlanProviderTest, as above; all tests of fe-connector-paimon and paimon-scanner pass.Behavior changed:
enable_paimon_write_char_paddingis false; in an ORC table only the partition values.Does this need documentation?
enable_paimon_write_char_padding.Check List (For Reviewer who merge this PR)