Repository navigation
[fix](paimon) Keep CHAR/VARCHAR lengths and DATETIME precision when creating Paimon tables - #69045
Conversation
…reating Paimon tables ### What problem does this PR solve? Issue Number: None Related PR: apache#66227 Problem Summary: Ported from the DDL type mapping change in branch-4.1 apache#66227. When Doris creates or alters a Paimon table, PaimonTypeMapping.toPaimonType turns the declared column type into the Paimon schema. On master CHAR(n), VARCHAR(n) and STRING all became VARCHAR(2147483647), and DATETIME became TIMESTAMP(6). Paimon uses these bounds when it reads a data file written under an older schema: it casts the file's column type to the current one, truncating to the declared length and reading numeric timestamp strings in units of the declared precision. With the bounds dropped, the Paimon table did not have the schema the user declared, and Doris read the column back as STRING. CHAR(n) now maps to CHAR(n), VARCHAR(n) to VARCHAR(n), STRING to VARCHAR(2147483647), DATETIME to TIMESTAMP(0), and DATETIMEV2(p) to TIMESTAMP(p) as before, also inside ARRAY, MAP and STRUCT, as on branch-4.1 and branch-4.2. ### Release note Paimon tables created by Doris keep the declared CHAR(n) / VARCHAR(n) length; DATETIME maps to TIMESTAMP(0). ### Check List (For Author) - Test: Unit Test - PaimonTypeMappingToPaimonTest: bounds, precision, nested types, and Paimon's own casts on the mapped types - Behavior changed: Yes. CHAR(n) / VARCHAR(n) columns of Paimon tables created by Doris keep their length instead of becoming STRING. - Does this need documentation: No Co-authored-by: Fangyuan Deng <982092332@qq.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
run buildall |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Request changes: two verified character-handling defects remain. Persisting bounded VARCHAR now exposes a native/JNI mismatch for historical narrowing, and persisting true CHAR needs write-side blank padding before key extraction and storage. Both are inline below; there are no existing blocking comments to carry forward.
Static review of both changed files and their CREATE/CTAS/ADD/MODIFY, recursive type, schema evolution, and read/write consumers. No builds or tests were run, as required by this review environment. The author's reported test count is not reviewer-executed evidence.
Critical checkpoint conclusions:
- Goal and correctness: declared bounds and DATETIME-v1 seconds precision are mapped correctly locally; end-to-end character behavior still needs the two inline fixes. The new unit assertions verify descriptors and library casts, not the affected Doris read/write paths.
- Scope, clarity, and reuse: focused two-file change; CREATE/ADD/MODIFY and nested types reuse the same mapper.
- Concurrency and locking: stateless conversion; no new threads, mutable shared state, locks, lock-held I/O, or lock-order changes.
- Lifecycle and initialization: only small schema descriptors are constructed; no new resource ownership, caches, worker lifetimes, or static initialization dependency.
- Configuration: no configuration or dynamic-update mechanism added.
- Compatibility: existing remote schemas remain authoritative, and an unchanged MODIFY retains the remote type. No SPI/function symbol or wire-format change. Newly created/explicitly altered character schemas expose the inline interoperability problems.
- Thrift/protobuf IDs: no definitions changed; branch field-ID comparison is inapplicable.
- Parallel paths: CREATE/CTAS and ADD/MODIFY share this mapping; ARRAY/MAP/STRUCT recurse and preserve existing nullability/comments. Native/JNI read parity and canonical CHAR writes require fixes. STRING, DATETIMEV2, and TIMESTAMPTZ behavior is unchanged.
- Conditions and errors: uppercase normalization makes the v1/v2 timestamp discriminator correct; valid DDL validates and recursively defaults character lengths before encoding them as precision. Unsupported types still fail; no new swallowed error or speculative fallback.
- Test coverage: scalar bounds/kinds, timestamp precisions, historical library casts, ARRAY/MAP bounds, and existing STRUCT/nullability/unsupported cases inspected. End-to-end ALTER/read parity and short CHAR write/key cases are missing and should cover the inline issues.
- Test results: new expected values agree with Paimon 1.4.2 source; no golden outputs changed. Assertions were inspected, not executed.
- Observability: no new distributed operation; existing DDL error/log context remains sufficient. The silent value/key defects need correctness fixes and regression coverage.
- Transactions and persistence: unchanged Paimon catalog create/alter persists the schema. Doris EditLog/replay/failover and transaction commit protocols are untouched.
- Writes, atomicity, and crashes: atomic schema/insert commit and cleanup paths are unchanged, but CHAR values need normalization before key/bucket extraction; no new leak or deadlock mechanism identified.
- FE/BE propagation: no new variable or protocol field; existing remote-schema transport carries bounds to JNI, while native character conversion loses their semantic effect.
- Performance: constant-time scalar construction and existing linear schema recursion; no additional I/O or row-path work introduced.
- Other concerns and user focus: no additional user focus was supplied. Invalid/missing-length and timestamp-compatibility risks were dismissed with upstream/downstream evidence. No remaining unresolved candidate or static coverage gap after the final sweep; runtime execution remains outside this review's scope.
Review converged after 2 rounds: both assigned agents returned NO_NEW_VALUABLE_FINDINGS in the latest round, both changed files and applicable checkpoints are covered, and all candidates/risk focuses have explicit verified or dismissed dispositions.
Review converged after 2 round(s).
| // DDL must retain bounds: widening to STRING changes historical narrowing casts. | ||
| return new CharType(type.getPrecision()); | ||
| case "VARCHAR": | ||
| return new VarCharType(type.getPrecision()); |
There was a problem hiding this comment.
[P1] Apply narrowing casts on the native Paimon read path
This mapping is also used by MODIFY COLUMN. For an append-only Parquet table t(s STRING) containing abcdef, ALTER TABLE t MODIFY COLUMN s VARCHAR(3) now persists the bound, but the default native read still returns abcdef: PaimonScanPlanProvider routes raw files to native without checking character casts, the v1 string converter is ConsistentConverter, and v2 treats string types as equal regardless of length. Paimon's historical cast instead returns abc (also with force_jni_scanner=true). Previously this ALTER mapped to VARCHAR(MAX), so the change exposes inconsistent results after Doris DDL. Please apply Paimon character casts in native reads or route affected splits through JNI, and cover ALTER/read parity with an end-to-end test; the new direct CastExecutors assertion bypasses this path.
There was a problem hiding this comment.
The difference is real, but it is how Doris's native readers already treat the CHAR/VARCHAR bounds of external tables, not something this PR adds. They apply the table's length only when truncate_char_or_varchar_columns is on: the format_v2 TableReader handles exactly this case (a table VARCHAR(n) over a wider or STRING file column, _should_truncate_char_or_varchar_column), and Hive and Iceberg tables behave the same. Master already reads Paimon VARCHAR(n) as VARCHAR(n), so a column narrowed by Spark or Flink gives the same native/JNI difference today, and with the session variable on the native read returns abc. This PR only makes Doris DDL store the declared type, as branch-4.1 and branch-4.2 do. Making native reads follow Paimon's casts by default would change those existing tables as well, so I'd keep it out of this PR. I updated the description, which said older values are truncated without mentioning the native default.
| return new DoubleType(); | ||
| case "CHAR": | ||
| // DDL must retain bounds: widening to STRING changes historical narrowing casts. | ||
| return new CharType(type.getPrecision()); |
There was a problem hiding this comment.
[P2] Normalize CHAR values before writing the new CHAR schema
Returning CharType(n) also changes newly Doris-created tables from VARCHAR(MAX) to real Paimon CHAR, but the Doris sink does not implement Paimon's CHAR padding. With k CHAR(3) NOT NULL as a primary key, inserting 'a' passes the one-character string through BindSink, Arrow, and PaimonWriteSchema.tableRow; the SDK extracts/stores that key without casting. Paimon's CHAR cast produces 'a ', and its CHAR comparator compares the raw strings, so another writer using the typed CHAR value creates a separate key instead of updating the Doris-written record. Please normalize CHAR values before key/bucket extraction and storage (including nested CHAR), and add a write/cast parity test with a short value. The former VARCHAR(MAX) schema did not require this padding.
There was a problem hiding this comment.
Confirmed: neither write backend pads CHAR. PaimonWriteSchema.tableRow (JNI) and the FFI backend pass the string through as Doris holds it. That is already the case on master for CHAR(n) columns of tables created by Spark or Flink, which Doris reads as CHAR(n) and writes to unpadded, and branch-4.1/4.2 behave the same with this mapping. The fix belongs in the write path (both backends, nested fields included), where it also covers those existing tables; mapping CHAR(n) to a wider type here would only hide it for tables created by Doris. I'll do the padding as a separate change, and the description now states the limitation.
TPC-H: Total hot run time: 29128 ms |
TPC-DS: Total hot run time: 152623 ms |
ClickBench: Total hot run time: 24.71 s |
FE UT Coverage ReportIncrement line coverage `` 🎉 |
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: None
Related PR: #66227, #68825
Problem Summary:
In short. When Doris creates or alters a Paimon table, master drops the declared length of CHAR(n) and VARCHAR(n) columns (they all become VARCHAR(2147483647)) and gives DATETIME a precision of 6. Paimon relies on these bounds when it reads files written under an older schema, so a table created or altered through Doris does not behave like the schema the user declared. This PR keeps the bounds, as branch-4.1 #66227 does.
Background.
PaimonTypeMapping.toPaimonType(fe-connector-paimon) turns the column types ofCREATE TABLE,ADD COLUMNandMODIFY COLUMNinto the Paimon schema. Paimon stores that schema with every data file. When it reads a file written under an older schema, it casts each column from the file's type to the current type, so the current type decides what a reader sees: a VARCHAR(3) column truncates older longer values, and a TIMESTAMP(3) column reads a numeric string such as42as milliseconds. The mapping was ported from fe-core'sDorisToPaimonTypeVisitorwhen Paimon moved into the connector plugin; branch-4.1 #66227 later changed that visitor to keep the bounds, and the change never reached master. #68825 (the master port of the rest of #66227) updated this method's Javadoc to describe the new mapping, but not the code.The problem, and what it cost.
MODIFY COLUMN c VARCHAR(3)narrows the column, and Paimon truncates older values when it reads them; on master it widens the column to the maximum length instead.The same applies inside ARRAY, MAP and STRUCT, and to
CREATE TABLE ... AS SELECT: query columns that Doris' CTAS rules keep as VARCHAR(n) (columns of internal tables, and expressions as VARCHAR(65533) by default) keep that length instead of becoming STRING. Only Doris DDL uses this mapping:ADD COLUMNandMODIFY COLUMNon a table created by Spark are affected, reading a table is not.How this PR fixes it.
toPaimonTypemaps CHAR(n) toCharType(n), VARCHAR(n) toVarCharType(n)(CREATE TABLE already carries the length inConnectorType.precision), STRING toVarCharType(MAX_LENGTH), DATETIME toTimestampType(0)and DATETIMEV2(p) toTimestampType(p). A column declared as plainvarcharorcharis normalized to VARCHAR(65533) or CHAR(1) byColumnDefinitionbefore it reaches the connector, so the length is always set. The Javadoc is the same text as in #68825.Two existing behaviors now also apply to these tables, as they already do on master to CHAR(n) and VARCHAR(n) columns of Paimon tables created by Spark or Flink. Doris's native Parquet/ORC readers apply the declared length to older values only when
truncate_char_or_varchar_columnsis on (the same as for Hive and Iceberg tables); JNI reads always apply Paimon's cast. Doris's Paimon writers do not pad CHAR(n) values to their length, while Paimon's own cast does. Both are left to separate changes.Results.
PaimonTypeMappingToPaimonTestnow checks the bounds and precisions, the nested types, and Paimon's own casts on the mapped types: from VARCHAR(10) to the mapped VARCHAR(3),abcdefbecomesabc; from STRING to the mapped DATETIMEV2(3),42becomes1970-01-01T00:00:00.042. With master's mapping all four of these cases fail. All fe-connector-paimon unit tests (726) pass. No existing regression suite creates Paimon tables with CHAR(n), VARCHAR(n) or DATETIME v1 columns through Doris; thetest_paimon_rust_reader_eq_for_nullsuite in #68825 narrows a column to VARCHAR(3) and needs this change.Original author: @pzhdfy (#66227)
Release note
Paimon tables created or altered by Doris keep the declared length of CHAR(n) and VARCHAR(n) columns instead of turning them into STRING.
Check List (For Author)
Test
PaimonTypeMappingToPaimonTest: 4 cases fail with master's mapping and pass with this PR; all fe-connector-paimon tests (726) pass.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)