Repository navigation
[Fix][Connector-V2][Doris] Declare LARGEINT as DECIMAL(39, 0) - #12787
zhengxiang378928908-code wants to merge 5 commits into
Conversation
DanielLeens
left a comment
There was a problem hiding this comment.
Good job, and thanks a lot for tackling Doris LARGEINT precision, which is a real pain point for non-Doris sinks. The type-conversion work, the value-level tests and the Doris to MySQL end-to-end case are carefully thought out. I did find one blocker that CI on your fork already proves, plus a few follow-ups, so please see below.
What Problem Does This PR Solve?
User pain point: Doris LARGEINT is a signed 128-bit integer (up to 39 digits), but the Doris source declared it as DECIMAL(20, 0). A non-Doris sink that auto-creates its table from the schema therefore gets a column that is too narrow for values over 20 digits.
Fix approach: Declare scalar LARGEINT, ARRAY<largeint> elements and map key/value LARGEINT as DECIMAL(39, 0). For Doris to Doris, keep the native LARGEINT (scalar, array, map key/value) on the sink side through the source type, because DECIMAL(39, 0) exceeds the DECIMALV3 limit of 38. Fix the Doris docs mapping table and add unit tests plus a Doris to MySQL E2E.
One-sentence summary: Widen the SeaTunnel type for Doris LARGEINT from DECIMAL(20, 0) to DECIMAL(39, 0) and preserve LARGEINT on Doris sinks.
Before / after example:
Doris column: f_largeint LARGEINT
Before: SeaTunnel DECIMAL(20, 0) -> MySQL auto-create DECIMAL(20,0) (values with 21+ digits do not fit)
After: SeaTunnel DECIMAL(39, 0) -> MySQL auto-create DECIMAL(39,0) (full range fits)
-> Spark engine: job fails at schema conversion (see Issue 1)
1. Code Change Review
1.1 Core Logic Analysis
What changed
AbstractDorisTypeConverter.sampleTypeConverter(AbstractDorisTypeConverter.java:189-194):LARGEINTnow maps tonew DecimalType(DORIS_LARGEINT_PRECISION, 0)withcolumnLength = 39(was20).DorisTypeConverterV2.convertArray(DorisTypeConverterV2.java:172-174):ARRAY<largeint>elements becomeDECIMAL(39, 0).AbstractDorisTypeConverter.reconvert,ARRAYbranch (AbstractDorisTypeConverter.java:401-408): when the source type equalsARRAY<largeint>, emitARRAY<LARGEINT>instead ofARRAY<DECIMALV3(p, s)>.DorisTypeConverterV2.reconvertMapand the newmapLargeintSourceTypes: pass the LARGEINT source type of a map key/value down so it is kept asLARGEINT.- Docs: the source mapping row changes from
STRINGtoDECIMAL(39, 0)(en and zh). Tests and one new E2E are added.
Before / after of the core mapping:
// before
decimalType = new DecimalType(20, 0);
builder.columnLength(20L);
// after
decimalType = new DecimalType(DORIS_LARGEINT_PRECISION, 0); // 39
builder.columnLength((long) DORIS_LARGEINT_PRECISION);Key Findings
- The normal user path (any Doris source with a
LARGEINTcolumn) reaches the changed logic throughDorisTypeConverterV2.convert -> sampleTypeConverter. So the change is not an edge-path fix, it changes the schema every LARGEINT user sees. - The Doris sink side is handled well:
LARGEINTis preserved through the source type, and the DDL forARRAYandMAPalso keepsLARGEINT. DecimalType(39, 0)is not representable on the Spark engine. CI on the PR author's fork showsAnalysisException: decimal can only support precision up to 38thrown fromTypeConverterUtils.convert(Issue 1). This is a blocker.- The change alters the schema contract for every existing Doris LARGEINT job (type, and the auto-created column type on downstream sinks), but
incompatible-changes.mdis not updated (Issue 2). - Special-casing relies on an exact string match of the source type (
ARRAY<largeint>), which is brittle (Issue 4).
Runtime path (text flow)
Doris FE/BE metadata (COLUMN_TYPE "largeint" / "array<largeint>" / "map<..>")
-> DorisCatalog/DorisSource builds BasicTypeDefine
-> DorisTypeConverterV2.convert(): LARGEINT -> DecimalType(39, 0), sourceType = original column type
-> CatalogTable schema (now DECIMAL(39,0))
|-- Zeta engine: rows carry BigDecimal, arrow Utf8 -> BigDecimal (unchanged read path)
|-- Spark engine: TypeConverterUtils.convert(DecimalType 39) -> new spark DecimalType(39, 0)
| -> AnalysisException: "decimal can only support precision up to 38" [job fails at readSchema]
|-- Doris sink (auto-create): reconvert(): sourceType == LARGEINT -> "LARGEINT" (kept)
`-- other JDBC/etc sinks (auto-create): reconvert(DECIMAL(39,0)) -> sink dialect decides (MySQL OK; 38-max sinks must clamp or fail)
In-depth correctness analysis
The Zeta path is correct: the value is read as BigDecimal from the Arrow Utf8 vector, and the unit test DorisLargeintReadTest.testReadLargeintFromArrowUtf8 exercises that with real extrema. The converter changes are internally consistent between V1 and V2.
The Spark path is not. TypeConverterUtils.convert (seatunnel-translation-spark-common, TypeConverterUtils.java:109-111) builds new org.apache.spark.sql.types.DecimalType(precision, scale) directly from the SeaTunnel decimal, and Spark rejects precision above 38. The Doris source schema is converted before any row is read, so every Spark job that reads a table with a LARGEINT column (including DorisIT, DorisMultiReadIT and the new DorisLargeintToMysqlIT on the Spark containers) now fails at BatchSourceReader.readSchema. Previously DECIMAL(20, 0) worked on Spark.
1.2 Compatibility Impact
Partially incompatible. Evidence:
- Zeta jobs keep running, but the declared schema of every Doris LARGEINT column changes from
DECIMAL(20, 0)toDECIMAL(39, 0). Auto-created downstream tables get a different column type, and downstream sinks whose maximum decimal precision is 38 (this is an inference from typical dialect limits, for example Oracle, SQL Server, Hive/Paimon/Iceberg style catalogs; please verify per sink) will either clamp with a warning or fail at table creation. Before this PR such jobs worked for values under 20 digits. - Spark engine: previously working Doris LARGEINT jobs now fail (proved by CI, see Issue 1).
- Doris to Doris: new tables created from
ARRAY<largeint>/MAP<..., largeint>now use nativeLARGEINTinstead ofDECIMALV3(20, 0); that is a behavior change for auto-created tables but is a correctness improvement and existing tables are untouched. - The docs mapping row previously said
STRING, which did not match the code (DECIMAL(20, 0)); correcting it is good.
1.3 Performance / Side-Effect Analysis
No hot-path change: the conversion happens once per schema. BigDecimal with 39 digits is stored the same way as before. No new threads, caches or I/O in production code.
1.4 Error Handling and Logging
Issue 1: DECIMAL(39, 0) breaks every Doris source job on the Spark engine (CI-proven)
- Location:
AbstractDorisTypeConverter.java:189-194(root cause trigger),seatunnel-translation/seatunnel-translation-spark/seatunnel-translation-spark-common/src/main/java/org/apache/seatunnel/translation/spark/utils/TypeConverterUtils.java:109-111(where it throws). - Problem description: The failing job on the author's fork (
Run / doris-connector-it (17, ubuntu-latest), 10 failures:DorisIT6,DorisMultiReadIT2,DorisLargeintToMysqlIT2, all on the Spark containers) showsAnalysisException: decimal can only support precision up to 38fromDecimalType.<init>called byTypeConverterUtils.convert(TypeConverterUtils.java:111)viaMultiTableManager.getTableSchemaandBatchSourceReader.readSchema. The failures are only in the Spark container cases (container indices 6 and 7 of each test template); the other container cases of those tests did not fail. The same failures occur in the(11, ubuntu-latest)job. - Potential risk: Existing Doris to anything jobs on Spark that read a LARGEINT column stop working after upgrade, including previously passing E2E coverage. This is a regression of already-covered behavior.
- Best improvement: Decide the type contract first, since Spark (and likely other 38-limited consumers) cannot represent 39 digits. Options: (a) map LARGEINT to
STRINGplus keep theLARGEINTsource type, so Doris sinks stay lossless and other sinks get a varchar (this matches what the docs said before this PR); (b) cap atDECIMAL(38, 0)and document that values beyond 38 digits are not supported on that path; (c) keepDECIMAL(39, 0)only if the Spark translation gets an explicit, tested handling. Whichever is chosen must keep theDorisIT/DorisMultiReadITSpark containers green. Please also discuss the choice on the linked issue (#12753) since it is a user-visible contract. - Severity: High
- Raised by another reviewer: No
Issue 2: Schema contract change is not recorded as an incompatible change
- Location:
docs/en/introduction/concepts/incompatible-changes.md(not touched by the PR);docs/en/connectors/source/Doris.md:51and the zh counterpart. - Problem description: The declared type of every Doris LARGEINT column changes (
DECIMAL(20, 0)toDECIMAL(39, 0), or whatever Issue 1 settles on), which changes auto-created downstream column types. The project guideline requires such changes to be listed inincompatible-changes.mdwith migration guidance. - Potential risk: Users upgrading will see changed DDL on downstream tables or new failures on 38-limited sinks without any notice.
- Best improvement: Add an entry with the old and new mapping, which engines and sinks are affected, and a workaround (for example an explicit
cast/sqltransform toDECIMAL(38, 0)orSTRING). - Severity: Medium
- Raised by another reviewer: No
Issue 3: Downstream sinks with a 38-digit decimal ceiling are not evaluated
- Location: change in
AbstractDorisTypeConverter.java:191; E2EDorisLargeintToMysqlIT.javaonly covers MySQL. - Problem description: The new E2E proves MySQL (max 65 digits). Nothing shows what happens for sinks whose maximum decimal precision is 38 when they auto-create from
DECIMAL(39, 0). - Potential risk: Silent narrowing with a warning (data overflow for values with 39 digits) or a DDL failure, depending on the dialect.
- Best improvement: Spot-check the main sink dialects (at least one 38-max JDBC dialect) and document the behavior, or choose the contract in Issue 1 so that the question disappears.
- Severity: Medium
- Raised by another reviewer: No
Issue 4: Exact-string source type match is brittle
- Location:
AbstractDorisTypeConverter.java:402-404(equalsIgnoreCase(DORIS_LARGEINT_ARRAY)whereDORIS_LARGEINT_ARRAY = "ARRAY<largeint>"),DorisTypeConverterV2.mapLargeintSourceTypes. - Problem description: The Doris-preserving branch only fires when the source type string is exactly
ARRAY<largeint>(case-insensitive). A different spacing or a type likearray<largeint(40)>would fall through toARRAY<DECIMALV3(39, 0)>, which Doris rejects at DDL time. - Potential risk: Doris to Doris auto-create fails for those variants.
- Best improvement: Normalize (strip whitespace, parse the element type) or reuse
extractArrayTypeand compare the element type toLARGEINT. - Severity: Low
- Raised by another reviewer: No
2. Code Quality Assessment
2.1 Coding Standards
Apache license headers are present on the new Java and conf files. The new constant has a clear Javadoc and the new helper has a good explanatory comment. DorisLargeintToMysqlIT follows existing IT scaffolding (AbstractDorisIT, TestContainerExtension).
2.2 Test Coverage and Test Stability
- Unit tests: updated converter tests for scalar, array and map;
testReconvertMapWithLargeintcovers key, value and unchanged mapping;DorisLargeintReadTest.testReadLargeintFromArrowUtf8reads real extrema throughArrowToSeatunnelRowReader. These would fail if the mapping were reverted to precision 20, which is good.testLargeintTypeHoldsWholeRangeis partly a restatement of the constant, but it does guard the range. - Gap: no test exercises the Spark translation with the new type, which is exactly where the regression is. The existing Spark containers in
DorisITcaught it, so the E2E coverage is valuable; do not weaken it. - E2E stability rating for
DorisLargeintToMysqlIT: Risk present. It fails on the Spark containers today (testLargeintToMysqlAutoCreate,executeJobexit code 1 asserted inDorisLargeintToMysqlIT). Also in the same CI runDorisITshows stream-load "No route to host" style failures in other cases, so the Doris container tests are sensitive to environment; this PR should not add more coupling than needed (it starts a second container, MySQL, on a fixed network aliasmysql_largeint_e2e).
2.3 Documentation Updates
The source mapping row is corrected in both languages, which is a nice catch. Missing: incompatible-changes.md entry (Issue 2) and a note that Doris to non-Doris auto-create produces DECIMAL(39, 0) (or whichever type is finally chosen). The sink doc mapping for LARGEINT/DECIMAL could mention the preserved LARGEINT behavior for Doris to Doris.
3. Architectural Soundness
3.1 Elegance
Long-term direction is sound: keep the exact source type for lossless Doris to Doris and give non-Doris sinks a wide-enough type. The current form is a precise fix for the stated problem on Zeta, but it is not complete for the engines SeaTunnel supports (Issue 1).
3.2 Maintainability
Constants and comments are clear. The string-based special cases (ARRAY<largeint>, map source type parsing) add some parsing logic that should be centralized (Issue 4).
3.3 Extensibility
Passing the original source type down through reconvertMap is a reasonable hook for other Doris-specific types in the future.
3.4 Historical-Version Compatibility
Partially incompatible as described in 1.2. Old jobs on Zeta keep running but see a different schema; old jobs on Spark break. Existing Doris tables are not altered.
4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | DECIMAL(39, 0) breaks Doris LARGEINT source on Spark (CI-proven) | AbstractDorisTypeConverter.java:189-194; TypeConverterUtils.java:109-111 |
High |
| 2 | Schema contract change not listed in incompatible-changes.md | docs/en/introduction/concepts/incompatible-changes.md |
Medium |
| 3 | Sinks with a 38-digit decimal ceiling not evaluated | AbstractDorisTypeConverter.java:191; DorisLargeintToMysqlIT.java |
Medium |
| 4 | Exact-string source type match is brittle | AbstractDorisTypeConverter.java:402-404 |
Low |
5. Merge Recommendation
Conclusion: Not recommended for merge
- Blockers - must be fixed
- Issue 1: the Spark containers of
DorisIT,DorisMultiReadITand the newDorisLargeintToMysqlITfail withdecimal can only support precision up to 38; CI on the head commit is red for this real, PR-caused reason (failing jobdoris-connector-it). - Issue 2: record the schema contract change in
incompatible-changes.mdonce the final type is decided.
- Issue 1: the Spark containers of
- Recommended fixes - non-blocking
- Issue 3: confirm behavior on at least one 38-max sink.
- Issue 4: make the
ARRAY<largeint>detection tolerant.
Overall assessment: the intent and the Doris-side handling are good and the tests are meaningful, but the chosen target type cannot be represented by the Spark engine, which turns previously green Spark E2E cases red. A better alternative is to keep the source type LARGEINT for Doris sinks and expose a type that every supported engine can hold (for example STRING, or a documented DECIMAL(38, 0) cap) to everything else. Happy to re-review once the type contract is settled, and thanks again for the thorough work.
|
@SEZ9 @DanielLeens Both items are pushed in 521c27f:
|
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround and for pinging me. I re-reviewed the latest head (521c27f) against my previous round on ddb814e.
What Problem Does This PR Solve?
User pain point: Doris LARGEINT (signed 128-bit, up to 39 digits) was declared as DECIMAL(20, 0), so any sink that auto-creates its table from the schema gets a column that is too narrow (#12753).
Fix approach: Declare scalar LARGEINT, ARRAY<largeint> elements and map key/value LARGEINT as DECIMAL(39, 0), and keep the native LARGEINT on Doris sinks through the source type.
One-sentence summary: Widen the SeaTunnel type of Doris LARGEINT to DECIMAL(39, 0) while keeping Doris to Doris lossless.
1. Code Change Review
1.1 Core Logic Analysis
What changed since my last review (5 files, no change to the Zeta read path):
AbstractDorisTypeConverter.java: newisLargeintType/isLargeintArrayTypehelpers normalize case, whitespace and an optional display width (largeint(40)), andreconvertforARRAYnow usesisLargeintArrayType(column.getSourceType())instead of the exact-stringequalsIgnoreCase.DorisTypeConverterV2.java:convertArrayandmapLargeintSourceTypesuse the same helper, so array and map variants are matched on the element type.incompatible-changes.md(en and zh): new entry for theDECIMAL(20, 0)toDECIMAL(39, 0)change with impact and aCASTworkaround.DorisTypeConvertorV2Test: new cases forarray<largeint(40)>,ARRAY< LARGEINT >andmap<largeint(40),double>. They would turn red if the helper went back to an exact match.
Status of my earlier items:
- Issue 2 (incompatible-changes entry): closed. The Oracle clamp to
NUMBER(38,0)was confirmed earlier on the #12753 thread; the SQL Server statement in the entry is the author's claim that I did not verify. - Issue 3 (38-digit sinks): closed for this PR. The behavior is now documented rather than tested, which fits the scope since each sink converter owns its own precision limit.
- Issue 4 (brittle string match): closed for arrays and maps. One small leftover, non-blocking: the scalar branch of
reconvert(AbstractDorisTypeConverter.java:341-343) still usesequalsIgnoreCase(DORIS_LARGEINT), so it is not covered by the new tolerant helper. I could not confirm what the catalog returns as the scalar column type, so please treat this as a consistency suggestion only. - Issue 1 (Spark): not closed, see below.
Issue 1 (carried over, still open): DECIMAL(39, 0) fails every Doris source job on Spark 2.4 and 3.3
- Location:
AbstractDorisTypeConverter.java:220-225triggers it; it throws inseatunnel-translation-spark-commonTypeConverterUtils.convert. - Evidence on this head: the fork run for 521c27f reports the same 10 failures as before in
doris-connector-it(JDK 11 and 17), all on the Spark containers:DorisIT(testDoris,testDorisDirectToBe,testNoSchemaDoris),DorisMultiReadIT.testDorisMultiReadandDorisLargeintToMysqlIT.testLargeintToMysqlAutoCreate, each withAnalysisException: decimal can only support precision up to 38. This is real and caused by the type change, not CI noise, and it is the only red job. - Why it stays a blocker: previously working Spark jobs would fail at schema time, before any row is read. As agreed with @SEZ9 and in my comment on #12753, the fix belongs in an explicit Spark policy tracked in #12814, not in a silent clamp, a hidden fallback or disabled Spark cases. The PR description also says this should not merge before that policy lands, and I agree.
- Raised by another reviewer: yes, @SEZ9 on #12753 ("Spark policy issue: it stays the blocker for merging the PR"). +1 to that; I traced the same path and the schema conversion is the first place that throws.
1.2 Compatibility Impact
Partially incompatible. Zeta and Flink jobs keep running with a different declared schema (documented now). Spark jobs reading a Doris LARGEINT column break until the #12814 policy is implemented. Existing downstream tables are not altered. Please add the Spark-visible part to the incompatible-changes.md entry together with the #12814 change, as you already plan to.
1.3 Performance / Side-Effect Analysis
Schema-time only; the new helpers run once per column on a short string. No effect on the per-row path.
1.4 Error Handling and Logging
No new exception paths. Issue 1 above is the only item.
2. Code Quality Assessment
2.1 Coding Standards
Helpers have clear Javadoc. Nothing to add.
2.2 Test Coverage and Test Stability
Unit coverage for scalar, array and map variants is meaningful. Stability rating: Stable for the new unit tests. DorisLargeintToMysqlIT is doing its job by keeping the Spark containers red; please keep it and the Spark cases in DorisIT and DorisMultiReadIT enabled.
2.3 Documentation Updates
en and zh incompatible-changes.md are consistent with each other and with the code. The Spark section is deferred to the #12814 change.
3. Architectural Soundness
3.1 Elegance of the Solution
Precise fix for the Doris source representation; the Spark limit is correctly kept out of this PR.
3.2 Maintainability
Centralizing the matching in isLargeintType is a good improvement over scattered string comparisons.
3.3 Extensibility
The source-type pass-through is a reasonable hook for other Doris-specific types.
3.4 Historical-Version Compatibility
Same as 1.2: partially incompatible, documented, with the Spark gap tracked in #12814.
4. Issue Summary
| Number | Issue | Location | Severity | Status |
|---|---|---|---|---|
| 1 | DECIMAL(39, 0) breaks Doris LARGEINT sources on Spark |
AbstractDorisTypeConverter.java:220-225, TypeConverterUtils.convert |
High | Open, depends on #12814 |
| 2 | Missing incompatible-changes entry | incompatible-changes.md |
Medium | Fixed |
| 3 | 38-digit sinks not evaluated | AbstractDorisTypeConverter.java |
Medium | Closed (documented) |
| 4 | Exact-string match | AbstractDorisTypeConverter.java, DorisTypeConverterV2.java |
Low | Fixed (scalar branch left, optional) |
5. Merge Recommendation
Conclusion: Not recommended for merge (yet)
- Blockers - must be fixed
- Recommended fixes - non-blocking
- Use the same tolerant matching in the scalar
reconvertbranch for consistency.
- Use the same tolerant matching in the scalar
Overall assessment: the Doris-side work is in good shape and I have no further code-side findings for this PR. Thanks again for the careful follow-up; I am happy to look at the #12814 proposal separately.
|
@DanielLeens Applied the optional suggestion in 9fad9b5: the scalar LARGEINT branch in |
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for applying the scalar-branch suggestion so quickly. I re-reviewed the current head (9fad9b5) against my previous round on 521c27f. The code-side work is in good shape; two gates remain, and one of them is new since my last round.
What Problem Does This PR Solve?
User pain point: Doris LARGEINT (signed 128-bit, up to 39 digits) was declared as DECIMAL(20, 0), so any sink that auto-creates its table from the schema gets a column that is too narrow for larger values (#12753).
Fix approach: Declare scalar LARGEINT, ARRAY<largeint> elements and map key/value LARGEINT as DECIMAL(39, 0), and keep the native LARGEINT on Doris sinks through the source type.
One-sentence summary: Widen the SeaTunnel type of Doris LARGEINT to DECIMAL(39, 0) while keeping Doris to Doris lossless.
Example:
Doris column: f_largeint LARGEINT
Before: DECIMAL(20, 0) -> MySQL auto-create DECIMAL(20,0) (21+ digit values do not fit)
After: DECIMAL(39, 0) -> MySQL auto-create DECIMAL(39,0) (full range fits)
Doris sink: source type LARGEINT is kept
Spark 2.4 / 3.3: schema conversion fails (Issue 1)
1. Code Change Review
1.1 Core Logic Analysis
Delta since my last review (521c27f to 9fad9b5, 2 files):
AbstractDorisTypeConverter.java(sampleReconvert,case DECIMAL, around line 340): the scalar check changed fromcolumn.getSourceType().equalsIgnoreCase(DORIS_LARGEINT)toisLargeintType(column.getSourceType()).isLargeintTypeis null-safe and strips whitespace, upper-cases, and accepts an optional display width, solargeint(40),LargeIntandLARGEINT (40)now map back toLARGEINTinstead of falling through toVARCHAR(200).DorisTypeConvertorV2Test.testReconvertDecimal: loops over five spellings and asserts bothcolumnTypeanddataTypeequalLARGEINT. Reverting the branch to an exact match would turn this test red, so it has failure capability.
My optional suggestion from the last round is closed. The scalar, array and map paths now all use the same helper.
Key findings
- The normal path (any Doris source with a
LARGEINTcolumn) reaches this logic throughDorisTypeConverterV2.convert -> sampleTypeConverter, so this is the main path, not an edge-path fix. - The Zeta read path is unchanged and correct (Arrow Utf8 to
BigDecimal, covered byDorisLargeintReadTest). - The delta does not affect the Spark gap described in Issue 1.
Runtime path
Doris metadata (largeint / array<largeint> / map<..>)
-> DorisTypeConverterV2.convert(): LARGEINT -> DecimalType(39, 0), sourceType kept
-> CatalogTable schema DECIMAL(39,0)
|-- Zeta / Flink : BigDecimal rows, OK
|-- Spark 2.4/3.3: TypeConverterUtils.convert -> spark DecimalType(39,0)
| -> AnalysisException "decimal can only support precision up to 38"
| (at readSchema, before any row is read)
`-- Doris sink : reconvert() -> isLargeintType(sourceType) -> LARGEINT
1.2 Compatibility Impact
Partially incompatible. Zeta and Flink jobs keep running with a different declared schema, which is now documented in incompatible-changes.md (en and zh). Spark jobs reading a Doris LARGEINT column fail at schema time until the Spark policy in #12814 is implemented. Existing downstream tables are not altered.
1.3 Performance / Side-Effect Analysis
Schema-time only. The helpers run once per column on a short string (one regex match plus a replaceAll). No effect on the per-row path, no new threads, locks, retries or I/O.
1.4 Error Handling and Logging
Issue 1 (carried over, still open): DECIMAL(39, 0) fails every Doris source job on Spark 2.4 and 3.3
- Location:
AbstractDorisTypeConverter.java(LARGEINTtoDecimalType(DORIS_LARGEINT_PRECISION, 0)); it throws inseatunnel-translation-spark-commonTypeConverterUtils.convert. - Problem description: The fork run for this head (https://github.com/zhengxiang378928908-code/seatunnel/actions/runs/38994918275) has
doris-connector-it (11, ubuntu-latest)and(17, ubuntu-latest)red. In the (17) job log the failing tests areDorisIT.testDoris,DorisIT.testDorisDirectToBe,DorisIT.testNoSchemaDoris,DorisMultiReadIT.testDorisMultiReadandDorisLargeintToMysqlIT.testLargeintToMysqlAutoCreate, each listed twice (the two Spark containers), withAnalysisException: decimal can only support precision up to 38. The same set failed on the earlier heads, so this is PR-caused and not CI noise. - Potential risk: previously working Spark jobs over Doris tables with a
LARGEINTcolumn break after upgrade, even when values fit in 20 digits. - Best improvement: land the explicit Spark policy tracked in #12814 (not a silent clamp, hidden fallback or disabled Spark cases), then re-run
doris-connector-itso the Spark containers are green together with this change. The PR description already says it should not merge before that. - Severity: High
- Raised by another reviewer: Yes, @SEZ9 on #12753 (Spark policy as the merge blocker). +1; the PR body and my earlier rounds say the same.
Issue 2: PR has merge conflicts with dev
- Location: PR branch versus
dev(GitHubmergeable_stateisdirty; compare status isdiverged, 3 ahead and 28 behind). - Problem description: the branch cannot currently be merged cleanly. This is a separate blocker from Issue 1 and was not present in my earlier rounds.
- Potential risk: the Doris files and docs touched here (
incompatible-changes.mdin particular is edited by many PRs) may have changed ondev; the reviewed diff may not match what would be merged. - Best improvement: please rebase or merge
dev, resolve the conflicts, and re-run the Doris E2E on the resolved head. - Severity: Medium (merge gate)
- Raised by another reviewer: No
No new code-side findings in this round.
2. Code Quality Assessment
2.1 Coding Standards
License headers are present on new files, the helpers have clear Javadoc, and the scalar, array and map paths now share one matching helper. Nothing to add.
2.2 Test Coverage and Test Stability
- Unit tests (
DorisTypeConvertorV1Test,DorisTypeConvertorV2Test,DorisLargeintReadTest): meaningful, they assert exact output types and values, and the new loop intestReconvertDecimalguards the scalar spelling variants. Stability: Stable (pure in-memory, no sleeps, ports or shared static state). - E2E (
DorisLargeintToMysqlIT,DorisIT,DorisMultiReadIT): stability rating Risk present. They start Doris and MySQL containers (a fixed network alias for MySQL), and are red on the Spark containers today for the reason in Issue 1. Please keep them and the Spark cases enabled; they are exactly what catches this regression.
2.3 Documentation Updates
incompatible-changes.md and the Doris source mapping are updated in en and zh and are consistent with the code. The Spark section is deferred to the #12814 change, as the author stated. No docs gap beyond that.
3. Architectural Soundness
3.1 Elegance
Precise fix for the Doris source representation; the Spark engine limit is correctly kept out of this PR and tracked in #12814.
3.2 Maintainability
Centralized matching in isLargeintType / isLargeintArrayType is easier to maintain than scattered string comparisons.
3.3 Extensibility
Passing the original source type down through reconvert and reconvertMap is a reasonable hook for other Doris-specific types.
3.4 Historical-Version Compatibility
Partially incompatible as in 1.2: documented for Zeta and Flink, open for Spark until #12814 lands. No serialization, checkpoint or option changes.
4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | DECIMAL(39, 0) breaks Doris LARGEINT sources on Spark (CI-proven, depends on #12814) |
AbstractDorisTypeConverter.java, TypeConverterUtils.convert |
High |
| 2 | Merge conflicts with dev |
PR branch vs dev |
Medium |
5. Merge Recommendation
Conclusion: Not recommended for merge
- Blockers - must be fixed
- Issue 1: implement the Spark policy from #12814 and get
doris-connector-itgreen on the Spark containers. - Issue 2: resolve the merge conflicts with
devand re-run CI on the resolved head.
- Issue 1: implement the Spark policy from #12814 and get
- Recommended fixes - non-blocking
- None. The earlier optional scalar-branch suggestion is done.
Overall assessment: the Doris-side change and its tests are solid and I have no further code-side findings. The remaining gates are the Spark policy (an engine-level decision outside this connector) and the conflict resolution. Thanks again for the careful follow-up.
9fad9b5 to
ce7690a
Compare
DanielLeens
left a comment
There was a problem hiding this comment.
Reviewed current head ce7690a2d1bc against dev (14 files).
The fix correctly maps Doris signed 128-bit LARGEINT to DECIMAL(39,0) rather than the old 20-digit schema. I traced scalar, ARRAY<LARGEINT>, and MAP conversion/reconversion: source-type-aware reconstruction preserves LARGEINT where DecimalV3 cannot represent precision 39, while ordinary decimal behavior is left unchanged. Type normalization intentionally tolerates case, whitespace, and display widths.
The compatibility impact is explicitly documented in both languages, with migration guidance. Unit tests cover the complete signed range via Arrow UTF-8, scalar/array/map metadata, and round trips; the new Doris-to-MySQL E2E asserts the auto-created precision and extreme values. I found no source-level blocker.
CI: the current Build check is failing, so it remains a merge gate even though I found no diff-level cause from the available result.
Conclusion: code is ready after the CI gate is green; a maintainer with repository permission can then apply final approval.
|
Thanks @DanielLeens for the review. The Spark implementation for #12814 is now open as #12862. The Testing both together showed one more Doris issue: the shared Doris test tables have
I'm running the combined CI on my fork (this PR + #12862) and will post the result here. |
|
Combined CI result (this PR at 73f5246 + #12862, fork run 39139304609):
For comparison, this PR alone (39139300360) fails only in the Spark containers with |
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for the update, @zhengxiang378928908-code, and for the detailed notes on the Spark side, the follow-up PR #12862 and the combined fork CI run. I re-reviewed the latest head (73f52467). Since my last review (ce7690a2, "no source-level blocker") only the Doris E2E module changed (AbstractDorisIT, DorisIT, DorisMultiReadIT and nine spark/*.conf files), so I focused on those and re-verified the connector change.
What Problem Does This PR Solve?
Doris LARGEINT (signed 128-bit, up to 39 digits) was declared as DECIMAL(20, 0), so non-Doris sinks (for example an auto-created MySQL table) fail on values longer than 20 digits (#12753). The PR declares scalar LARGEINT, ARRAY<largeint> elements and map key/value LARGEINT as DECIMAL(39, 0), and keeps the Doris to Doris round trip as native LARGEINT through the source type.
1. Code Change Review
1.1 Core logic
Connector change (unchanged since my last review, re-verified):
// AbstractDorisTypeConverter.convert
case DORIS_LARGEINT: decimalType = new DecimalType(DORIS_LARGEINT_PRECISION /* 39 */, 0); // was (20, 0)
// reconvert
case DECIMAL: if (isLargeintType(column.getSourceType())) { columnType = LARGEINT; break; } // was equalsIgnoreCase
case ARRAY: if (isLargeintArrayType(column.getSourceType())) { ARRAY<LARGEINT> }
// DorisTypeConverterV2.reconvertMap: mapLargeintSourceTypes(sourceType) passes LARGEINT down for key/valueRuntime path: Doris source (DorisTypeConverterV2.convert) -> DecimalType(39, 0) in the CatalogTable -> reader decodes Arrow UTF-8 into BigDecimal (DorisLargeintReadTest) -> sink. For a Doris sink, reconvert sees DecimalType(39,0) with source type LARGEINT and writes native LARGEINT; for other sinks the 39-digit decimal is used by their own catalog.
Changes since my last review (E2E only):
DorisIT/DorisMultiReadITpass the container to the checks. On Spark,readColumns(...)leaves out three columns (MAP_SMALLINT_LARGEINT,MAP_LARGEINT_DOUBLE,F_ARRAY_STRING_LARGEINT) and the jobs run from the newspark/*.confvariants (the same config with a shorterdoris.read.field, I diffedspark/doris_source_and_sink.confagainst the original). On Zeta and FlinkreadColumnsreturns the full list, so those engines still compare every column.- Sink checks now compare column types by name instead of the old paired-cursor loop. This is stricter, not weaker: it asserts that the sink column names equal the expected list, and the old
if (sinkResultSet.next())guard that could silently skip rows is gone. The expectations that changed (decimalv3(20, 0)->largeint) follow the intended behavior. - New Spark-only
DorisIT.testNestedLargeintFailsOnSparkasserts exit code != 0 andSPARK-TRANSLATION-01plus the table and column path for the three nested cases.
1.2 Compatibility
Partially incompatible, and recorded in incompatible-changes.md (en/zh) with migration guidance: the source schema of a Doris LARGEINT column changes from DECIMAL(20,0) to DECIMAL(39,0), so auto-created sink tables get wider columns, and sinks with a 38-digit maximum clamp with a warning. Existing sink tables are not altered. The Spark effect is intentionally not documented yet (see Issue 2).
1.3 Performance / side effects
Type mapping only; no hot-path change. The type matching uses a precompiled-free String.matches/replaceAll on schema-time strings, executed once per column, which is negligible.
1.4 Error handling / logging and issues
Issue 1: The PR cannot pass CI or merge on its own; it depends on #12862
- Location:
DorisIT.java(testNestedLargeintFailsOnSpark,spark/*.confjobs) and thedoris-connector-itSpark containers ([6]Spark 2.4,[7]Spark 3.3) - Problem description: On the current head the Apache-side
Buildis red. The fork run for this exact head (39139300360) fails only indoris-connector-it(JDK 11 and 17) withAnalysisException: decimal can only support precision up to 38on the Spark containers; Code style, dead links and unit tests pass. The new Spark-only test also asserts an error code and message (SPARK-TRANSLATION-01, "Column...of table...has type DECIMAL(39, 0)") that exist only in #12862, so it cannot pass againstdev. Your combined run (this PR + #12862) showsdoris-connector-itgreen on JDK 11 and 17. - Potential risk: Merging this PR first would make every Doris table with a
LARGEINTcolumn fail at Spark schema translation, including tables that worked before (up to 20 digits). - Best improvement: Keep the explicit "merge after #12862" ordering: land #12862 first, then update this branch and get a green
Build. Alternatively fold the two into one PR. Please also add "Depends on #12862" to the PR description header so maintainers do not merge it early. - Severity: Medium
- Raised by another reviewer: No (you flagged it in the PR description; I am confirming it with the CI evidence)
Issue 2: Spark behavior change is not in incompatible-changes.md
- Location:
docs/en/introduction/concepts/incompatible-changes.mdand the zh counterpart (the new Doris section) - Problem description: Together with #12862 the Spark outcome becomes: top-level
LARGEINTgoes through the string carrier, andARRAY<LARGEINT>orMAPwithLARGEINTnow fails at job start withSPARK-TRANSLATION-01(before this change those columns worked asDECIMAL(20,0)). You noted you will add this once the policy is settled. - Potential risk: Spark users with such columns see their jobs fail after upgrade without a migration note.
- Best improvement: Add the Spark bullet (cause, error code, the
doris.read.fieldworkaround you already use in the E2E) in en and zh, in whichever of the two PRs lands last. - Severity: Medium
- Raised by another reviewer: No
Issue 3: Nine near-duplicate Spark job configs and a cross-PR message coupling
- Location:
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-doris-e2e/src/test/resources/spark/*.conf;DorisIT.assertNestedLargeintRejected - Problem description: Each Spark config is a full copy of the base job differing only in
doris.read.field(and a header comment), so future edits to the base jobs must be mirrored by hand. The negative test also matches the exact error text produced by #12862. - Potential risk: The copies can drift, and a wording change in #12862 would silently break this test.
- Best improvement: Optional: keep the copies but mention the pairing in the comments, or reuse the base configs through a small template/
readColumns-driven read field. Match on the error code plus the column path rather than the full sentence. - Severity: Low
- Raised by another reviewer: No
2. Code Quality Assessment
2.1 Coding standards
License headers present in new files; new constants, helpers and the E2E helpers carry Javadoc that explains why (DecimalV3 limit, Spark 38-digit limit). Comment completeness is good.
2.2 Test coverage and stability
Strong at unit level: DorisTypeConvertorV1/V2Test (scalar, array, map, case/whitespace/display-width spellings), DorisLargeintReadTest (full signed range through Arrow UTF-8), and DorisLargeintToMysqlIT (auto-created DECIMAL(39,0) with min/max values). The existing E2E assertions were not weakened for Zeta and Flink; for Spark three columns are excluded on purpose and covered by the negative test. Stability rating: Risk present - the Spark lanes depend on an unmerged PR (Issue 1).
2.3 Docs
docs/en and docs/zh Doris source pages and the incompatible-changes entries are updated; Spark note pending (Issue 2).
3. Architectural Soundness
The fix is confined to the Doris connector's type converter and does not touch the shared Spark translator, as agreed in #12753/#12814. Keeping the Spark policy in a separate PR is the right boundary; the cost is the merge ordering above.
4. Issue Summary
| # | Issue | Location | Severity |
|---|---|---|---|
| 1 | Depends on #12862 for Spark lanes; Build red until then | DorisIT, spark/*.conf |
Medium |
| 2 | Spark behavior change not in migration notes | incompatible-changes.md (en/zh) |
Medium |
| 3 | Duplicated Spark configs, message coupling | spark/*.conf, DorisIT |
Low |
5. Merge Recommendation
CI state of head 73f52467: Apache-side Build is failing. The failing jobs are doris-connector-it (JDK 11 and 17) on the Spark 2.4 and 3.3 containers with decimal can only support precision up to 38, which is caused by this PR's DECIMAL(39, 0) meeting the not-yet-merged #12862 - it is a dependency effect, not a flaky job. The branch has no conflicts (17 commits behind dev, not needed for this failure).
Conclusion: Ready to merge after fixes
-
Blockers
- Land #12862 first (or combine), then a green
Buildon this PR. - Issue 2: the Spark migration note, in whichever PR lands last.
- Land #12862 first (or combine), then a green
-
Non-blocking
- Issue 3.
I found no source-level problem in the connector change itself. I did not run the code; this review is based on the full diff, the call chain and the fork CI logs. Thank you for the careful, well-documented work!
Review follow-up for apache#12787. - incompatible-changes (en/zh): on Spark, top-level and ROW-nested LARGEINT is carried as text and restored to DECIMAL(39, 0); LARGEINT inside ARRAY/MAP now fails at job start with SPARK-TRANSLATION-01. Migration: leave those columns out with doris.read.field, or use Zeta or Flink. - DorisIT matches the error code, column path and table instead of the full message text of apache#12862. - spark/*.conf say they differ from the base job only in doris.read.field and must be kept in sync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks, @DanielLeens. Pushed ab885d5:
The previous combined run (39139304609) finished all green after re-running the timed-out job. A new combined run with both new heads is in progress (39193334501); I'll post the result. |
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks a lot for the quick follow-up, and for adding the "Depends on #12862" header, the Spark note and the tightened negative test. I re-reviewed the latest head (ab885d5cd) against my previous round on 73f52467c.
What Problem Does This PR Solve?
User pain point: Doris LARGEINT (signed 128-bit, up to 39 digits) was declared as DECIMAL(20, 0), so a non-Doris sink that auto-creates its table from the schema gets a column too narrow for values longer than 20 digits (#12753).
Fix approach: Declare scalar LARGEINT, ARRAY<largeint> elements and map key/value LARGEINT as DECIMAL(39, 0), and keep native LARGEINT on Doris sinks through the source type.
One-sentence summary: Widen the SeaTunnel type of Doris LARGEINT to DECIMAL(39, 0) while keeping Doris to Doris lossless.
1. Code Change Review
1.1 Core Logic Analysis
Delta since 73f52467c (1 commit, docs plus E2E only; no production Java change):
incompatible-changes.md(en and zh): new Spark bullet. Top-level andROW-nestedLARGEINTis carried through Spark as decimal text and restored toDECIMAL(39, 0);LARGEINTinsideARRAY/MAPnow fails at job start withSPARK-TRANSLATION-01. The migration guide adds thedoris.read.fieldworkaround and says an SQL transform cannot help. The zh text matches the en text.DorisIT.assertNestedLargeintRejected(DorisIT.javaaround line 283): the assertion no longer matches the full message sentence; it checks the error code, the quoted column path and the table path.
// before
output.contains(String.format("Column `%s` of table `%s` has type DECIMAL(39, 0)", columnPath, tablePath))
// after
output.contains("`" + columnPath + "`"); output.contains(tablePath);- The eight
spark/*.confheaders now say "Onlydoris.read.fielddiffers; keep everything else in sync".
Key findings
- The normal path (any Doris source with a
LARGEINTcolumn) reaches the changed logic throughDorisTypeConverterV2.convert -> sampleTypeConverter; this is the main path, not an edge-path fix. The connector code is unchanged since my approval-level read once7690a2d. - The negative test is a small, intentional loosening of message wording. The original intent (
SPARK-TRANSLATION-01plus table and column path, for.key,.value,.element) is still asserted, and the exit-code check is intact. Not a test-weakening: the removed part was coupling to another PR's sentence. This addresses my previous Issue 3. - The Spark behavior depends on #12862 (still open, head
36e16f44f,Buildnot green yet at the time of this review). This PR alone cannot pass the Spark containers.
Runtime path
Doris metadata -> DorisTypeConverterV2.convert(): LARGEINT -> DecimalType(39, 0), sourceType kept
|-- Zeta / Flink : BigDecimal rows, OK
|-- Spark 2.4/3.3: needs #12862 (string carrier). Without it TypeConverterUtils.convert throws
| "decimal can only support precision up to 38" at readSchema
`-- Doris sink : reconvert() sees DECIMAL + sourceType LARGEINT -> native LARGEINT
1.2 Compatibility Impact
Partially incompatible. The Doris source schema of LARGEINT changes from DECIMAL(20, 0) to DECIMAL(39, 0); sinks capped at 38 digits clamp with a warning, and on Spark ARRAY/MAP with LARGEINT now fail at job start. All of this is now recorded in incompatible-changes.md (en/zh) with migration guidance. Existing downstream tables are not altered.
1.3 Performance / Side-Effect Analysis
No new hot-path allocation; only a precision constant and string-normalization helpers in schema conversion (run once per table, not per row). No threads, locks or state added.
1.4 Error Handling and Logging
Previous Issue 2 (Spark note in incompatible-changes.md) and previous Issue 3 (message coupling and sync comments) are closed. One item remains.
Issue 1: Merge ordering and CI gate on #12862
- Location:
DorisIT,DorisMultiReadIT,spark/*.conf(Spark lanes ofdoris-connector-it) - Problem: On this head the Apache-side
Buildis red. The fork run for this exact head (39193306399, this PR alone) fails onlydoris-connector-it(JDK 11 and 17); I did not read the log lines this round, but the failing job set is identical to the earlier head where the cause wasdecimal can only support precision up to 38on the Spark containers. The combined run (this PR plus #12862,39193334501) was still queued when I looked, so there is no green result for this head yet. The earlier combined run on73f52467cwas green fordoris-connector-it. - Risk: Merging this PR before #12862 would make every Doris table with a
LARGEINTcolumn fail at Spark schema translation, including tables that worked before with up to 20 digits. - Best improvement: Land #12862 first (the "Depends on" header now says so), then sync this branch with dev and get a green
Build; please post the green combined run once it finishes. - Severity: Medium
- Raised by another reviewer: No
2. Code Quality Assessment
2.1 Coding Standards
New files carry license headers; new helpers and E2E helpers have Javadoc that explains why. No unrelated changes in the delta.
2.2 Test Coverage and Test Stability
Unit: DorisTypeConvertorV1Test/V2Test (scalar, array, map, spelling variants) and DorisLargeintReadTest (full signed range via Arrow UTF-8). E2E: DorisLargeintToMysqlIT plus the Spark-only testNestedLargeintFailsOnSpark. Existing assertions were not weakened for Zeta and Flink. Stability rating: Risk present, only because the Spark lanes depend on an unmerged PR (Issue 1).
2.3 Documentation Updates
docs/en and docs/zh Doris source pages and both incompatible-changes.md files are updated and consistent with the code.
3. Architectural Soundness
3.1 Elegance
Precise fix, confined to the Doris connector converter; the shared Spark translator is handled separately in #12862 as agreed in #12753/#12814.
3.2 Maintainability
Shared normalization helpers cover scalar, array and map paths. The Spark config copies are a manual-sync cost but are now labeled as such.
3.3 Extensibility
Other LARGEINT-like wide types can reuse the same source-type-aware reconvert pattern.
3.4 Historical-Version Compatibility
Covered in 1.2. Old jobs writing into existing tables keep working; auto-created tables get wider columns; the Spark ARRAY/MAP break is documented.
4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | Land #12862 first and get a green Build |
Spark lanes of doris-connector-it |
Medium |
5. Merge Recommendation
CI state of head ab885d5cd: Apache-side Build is failing; the fork run 39193306399 fails only doris-connector-it (JDK 11 and 17). This is the known dependency effect on #12862, not a flaky job, and nothing in this delta (docs plus a test-assertion edit) can fix or cause it. The branch has no conflicts (17 commits behind dev, not required for this failure).
Conclusion: Ready to merge after fixes
- Blockers (must fix)
- Merge #12862 first, then sync this branch and show a green
Build(including the Spark lanes).
- Merge #12862 first, then sync this branch and show a green
- Recommended fixes (non-blocking)
- None.
I found no source-level problem in the connector change. I did not run the code; this review is based on the full diff, the call chain and the fork CI job list. There is no maintainer approval yet, so a committer will need to approve once the CI gate is green. Thank you for the careful, well-documented work!
|
Combined CI on the current heads (this PR at This PR's own run (39193306399) still fails only |
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for posting the combined result, @zhengxiang378928908-code. I checked it and it updates the CI picture I gave in my last review (I wrote that the combined run was still queued).
- Fork run
39193334501is a merge commit (f709578) of this PR headab885d5cdand #12862 head36e16f44f. It finished with 83 jobs passed, 0 failed and 11 skipped, anddoris-connector-itpassed on JDK 11 and 17, including the Spark containers andtestNestedLargeintFailsOnSpark. - #12862 on its own head
36e16f44falso has a greenBuildon the Apache side, so the Spark change looks healthy. - The
Buildcheck on this PR (ab885d5cdalone) is still red, as expected: onlydoris-connector-itfails, on the Spark 38-digit limit that #12862 removes.
So the blocker from my last review is unchanged in substance, but now has evidence behind it: it is purely the merge order. #12862 has to land first (it is still open and waiting for a committer review), then this branch needs a sync with dev and a green Build of its own. I am not changing my code assessment: I found no source-level issue in the connector change, and once the Build on this PR is green I am happy to approve from the technical side, with a committer doing the final approval. Thank you for the thorough verification work!
ab885d5 to
dcaa9f2
Compare
Review follow-up for apache#12787. - incompatible-changes (en/zh): on Spark, top-level and ROW-nested LARGEINT is carried as text and restored to DECIMAL(39, 0); LARGEINT inside ARRAY/MAP now fails at job start with SPARK-TRANSLATION-01. Migration: leave those columns out with doris.read.field, or use Zeta or Flink. - DorisIT matches the error code, column path and table instead of the full message text of apache#12862. - spark/*.conf say they differ from the base job only in doris.read.field and must be kept in sync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks @zhengxiang378928908-code for rebasing this onto the latest dev. I re-reviewed the new head dcaa9f2e8 against my last round on ab885d5cd. The new head is a pure rebase: git range-diff shows the same five commits with identical content, and the only textual difference is the surrounding context of the incompatible-changes.md entries (en/zh), which now sit after the JSON array/map entry that landed in dev. No production Java, test or config line changed. My code assessment is therefore unchanged, and the only open item is still the merge order with #12862.
What Problem Does This PR Solve?
User pain point: Doris LARGEINT (signed 128-bit, up to 39 digits) was declared as DECIMAL(20, 0), so any non-Doris sink that auto-creates its table gets a column too narrow for values with more than 20 digits (#12753).
Fix approach: Declare scalar LARGEINT, ARRAY<LARGEINT> elements and LARGEINT map keys/values as DECIMAL(39, 0), and keep native LARGEINT on Doris sinks by passing the source type through reconvert().
One-sentence summary: Widen the SeaTunnel type of Doris LARGEINT to DECIMAL(39, 0) while keeping Doris to Doris lossless.
1. Code Change Review
1.1 Core Logic Analysis
Changed production code (unchanged since my last round):
AbstractDorisTypeConverter.java:98DORIS_LARGEINT_PRECISION = 39;:106isLargeintType(case, whitespace and display width tolerant);:355scalarreconvertkeepsLARGEINTwhen the source type isLARGEINT;:447ARRAY<LARGEINT>is kept asARRAY<LARGEINT>instead ofDECIMALV3(39, 0)elements.DorisTypeConverterV2.java:171array elementLARGEINT->DecimalArrayType(DecimalType(39, 0));:258/:294mapLargeintSourceTypespassesLARGEINTsource types to the map key/value columns so a Doris sink keepsLARGEINT.
// before
decimalType = new DecimalType(20, 0);
// after
decimalType = new DecimalType(DORIS_LARGEINT_PRECISION, 0); // 39Key findings
- The normal path reaches the change: every Doris source table with a
LARGEINTcolumn goes throughDorisTypeConverterV2.convert. This is a main-path fix, not an edge-path fix. - The rebase did not touch any of the files
devchanged in between in a conflicting way;compare dev...dcaa9f2e8isahead_by=5, behind_by=0. - #12862 is still open, and its head has moved from
36e16f44fto7b9d94255("Keep wide-decimal checks off the per-row path"). The combined green fork run you posted (39193334501) was built against the old #12862 head, so it no longer describes the current pair.
Runtime path
Doris metadata -> DorisTypeConverterV2.convert(): LARGEINT -> DecimalType(39, 0), sourceType kept
|-- Zeta / Flink : BigDecimal rows, OK
|-- Spark 2.4/3.3: needs #12862 (string carrier); without it Spark rejects
| "decimal can only support precision up to 38" while building the read schema
`-- Doris sink : reconvert() sees DECIMAL + sourceType LARGEINT -> native LARGEINT
1.2 Compatibility Impact
Partially incompatible, intentionally and documented in both incompatible-changes.md files: the source schema type changes from DECIMAL(20, 0) to DECIMAL(39, 0), sinks capped at 38 digits clamp with a warning, and on Spark LARGEINT inside ARRAY/MAP fails at job start once #12862 is in. Existing downstream tables are not altered, and there is no checkpoint or serialization format change.
1.3 Performance / Side-Effect Analysis
Only schema-conversion helpers (once per table, not per row). No new threads, locks, buffers or state.
1.4 Error Handling and Logging
No new source-level issue. The only remaining item is the merge-order gate below.
Issue 1: Merge ordering and CI gate on #12862 (carried over)
- Location: Spark lanes of
doris-connector-it(DorisLargeintToMysqlIT,DorisMultiReadIT,DorisIT,spark/*.conf) - Problem: On this exact head, fork run
40073961674failsdoris-connector-iton JDK 11 and 17. I read the log:DorisLargeintToMysqlITandDorisMultiReadITfail only on the two Spark containers ([6]and[7]) withAnalysisException: decimal can only support precision up to 38, which is exactly the effect #12862 removes. The same run also failsunit-test (11)inSeaTunnelClientTest.testSetJobIdDuplicate(seatunnel-engineclient), which this PR does not touch and which I consider unrelated. The Apache-side status rollup for this head is currently empty, so there is noBuildresult attached yet. - Risk: Merging this PR before #12862 makes every Doris table with a
LARGEINTcolumn fail at Spark schema translation, including tables that worked before with up to 20 digits. - Best improvement: Land #12862 first (the "Depends on" header already says so), then sync this branch and get a green
Buildon its own head. Until then, a fresh combined fork run against the current #12862 head7b9d94255would confirm the pair still works together after that PR's latest change. - Severity: Medium
- Raised by another reviewer: No
2. Code Quality Assessment
2.1 Coding Standards
New helpers and E2E helpers carry Javadoc that explains why; new files have license headers. No unrelated changes.
2.2 Test Coverage and Test Stability
Unit: DorisTypeConvertorV1Test / DorisTypeConvertorV2Test (scalar, array, map, spelling variants) and DorisLargeintReadTest (full signed range through Arrow). E2E: DorisLargeintToMysqlIT plus the Spark-only DorisIT.testNestedLargeintFailsOnSpark (DorisIT.java:257). These tests would fail if the precision went back to 20 or the source-type pass-through were removed. Existing Zeta and Flink assertions are not weakened. Stability rating: Risk present, only because the Spark lanes depend on an unmerged PR (Issue 1).
2.3 Documentation Updates
docs/en and docs/zh Doris source pages and both incompatible-changes.md entries match the code and each other.
3. Architectural Soundness
3.1 Elegance
Precise fix, confined to the Doris converter; the shared Spark translation is handled separately in #12862.
3.2 Maintainability
Shared normalization helpers cover scalar, array and map. The spark/*.conf copies are a manual-sync cost, but their headers say so.
3.3 Extensibility
The source-type-aware reconvert pattern can be reused for other wide native types.
3.4 Historical-Version Compatibility
Covered in 1.2: jobs writing into existing tables keep working; auto-created tables get wider columns; the Spark ARRAY/MAP break is documented with a migration path.
4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | Land #12862 first and get a green Build on this head |
Spark lanes of doris-connector-it |
Medium |
5. Merge Recommendation
Conclusion: Ready to merge after fixes
- Blockers (must fix)
- #12862 must be merged first; then this branch needs a green
Build(including the Spark containers) on its own head.
- #12862 must be merged first; then this branch needs a green
- Recommended fixes (non-blocking)
- Optionally re-run the combined fork build against the current #12862 head
7b9d94255, and re-run the unrelatedunit-test (11)job when you next push.
- Optionally re-run the combined fork build against the current #12862 head
Overall assessment: No source-level problem found; the connector change, tests and docs are in good shape, and the rebase is clean. I did not run the code; this review is based on the full diff, range-diff, the call chain and the fork CI logs. A committer still needs to approve once CI is green. Thank you for the patience on this dependency chain!
|
Thanks for the contribution. The CI fact cited in my previous conclusion has changed; this follow-up updates that gate on the unchanged reviewed head, without issuing another full source review. Gate snapshot (2026-10-28T15:54:49+00:00), head The blocking jobs are Run / unit-test (11, ubuntu-latest) Observed log evidence: Please inspect or rerun only the named failed job where a rerun is appropriate. The logs establish the failing stage, but do not alone establish whether the current PR caused it. No matching upstream cure has been proved by this follow-up; a base sync alone is not a verified resolution, so no unrelated module/dependency patch is requested. My previous source conclusion remains clear for the reviewed scope. CI is not being passed by inference, and this follow-up is not an effective approval. Separately, GitHub reports merge conflicts with My source blocker is cleared, but an older CHANGES_REQUESTED opinion may remain in the review summary after a COMMENT-only technical conclusion. With Daniel currently READ-only, a write-capable maintainer must separately reconcile the effective approval/dismissal gate; this clarification does not claim that action was performed. |
LARGEINT is a signed 128-bit integer, but the source schema declared it as DECIMAL(20, 0), so a non-Doris sink that creates its table from the schema gets a column too narrow for values over 20 digits. - Declare scalar LARGEINT and ARRAY<largeint> elements as DECIMAL(39, 0) - Doris to Doris keeps LARGEINT for scalar, array and map key/value columns, since DECIMAL(39, 0) exceeds the DECIMALV3 precision limit - Fix the source type mapping in the Doris docs - Add value-level tests for 21-digit values and both signed extrema, and a Doris to MySQL auto-create E2E regression Part of apache#12753 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDrmQ2Mn4vwVcU9ox48wpv
- Detect LARGEINT in array elements and map keys/values by normalizing the type (case, whitespace, optional display width) instead of matching the exact string ARRAY<largeint> - Record the LARGEINT DECIMAL(20, 0) -> DECIMAL(39, 0) schema change in incompatible-changes.md (en/zh) Part of apache#12753 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ray elements Reconverting a DECIMAL column back to Doris only recognized the exact source type LARGEINT, so spellings such as largeint(40) or ' LargeInt ' fell through to VARCHAR(200). Use isLargeintType, the same check the ARRAY<LARGEINT> branch already uses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LARGEINT is now DECIMAL(39, 0). On Spark, a wide decimal inside ARRAY or MAP fails the precheck from apache#12814, so the shared Doris jobs failed on Spark because the test tables have MAP<SMALLINT, LARGEINT>, MAP<LARGEINT, DOUBLE> and ARRAY<LARGEINT> columns. - Spark runs the same jobs from spark/*.conf, which leave out those three columns with doris.read.field. Top-level LARGEINT is still read and written on Spark. - Sink checks compare columns by name and select explicit column lists, so they work when the Spark sink table has fewer columns. - testNestedLargeintFailsOnSpark checks that each of the three columns fails with SPARK-TRANSLATION-01 and names the table and column path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-up for apache#12787. - incompatible-changes (en/zh): on Spark, top-level and ROW-nested LARGEINT is carried as text and restored to DECIMAL(39, 0); LARGEINT inside ARRAY/MAP now fails at job start with SPARK-TRANSLATION-01. Migration: leave those columns out with doris.read.field, or use Zeta or Flink. - DorisIT matches the error code, column path and table instead of the full message text of apache#12862. - spark/*.conf say they differ from the base job only in doris.read.field and must be kept in sync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dcaa9f2 to
e1ddfb5
Compare
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for the rebase and the detailed combined-run history. I re-read the entire latest 26-file diff and retraced source metadata, Arrow values, Doris auto-DDL and both Spark adapters. The connector implementation remains sound in that scope. The merge-order blocker remains: #12862 is still open, so this PR alone breaks Spark schema creation for LARGEINT. The latest failing jobs are identified below rather than carried over from an older run.
What Problem Does This PR Solve?
User pain point: Doris LARGEINT can contain 39 digits, but its old DECIMAL(20,0) schema makes a non-Doris auto-created sink column too narrow.
Fix approach: Use DECIMAL(39,0) for scalar/array/map LARGEINT and preserve native LARGEINT source types when converting back into Doris DDL.
One-sentence summary: A 21-digit value that cannot fit in a MySQL DECIMAL(20,0) column will fit in the newly auto-created DECIMAL(39,0); signed 128-bit endpoints are also covered.
Review report time: 2026-10-29T12:23:56.886543-04:00. Current head e1ddfb581dcc85db1584a14358c51a9d49ca5f57; base dev at 886a3ac6e59aad5e46755761cf9fb8ac2a6d3df9. Checks: Signal pull request update=SUCCESS; Build=FAILURE. Check references: Signal pull request update Build. Compare: diverged, ahead 5, behind 5; merge state blocked / BLOCKED.
1. Code Change Review
1.1 Core Logic Analysis
All 26 files were read. AbstractDorisTypeConverter.java:98,225-234 widens the scalar precision/length, and :352-358 preserves scalar LARGEINT via normalized source type. Its array branch at :442-452 preserves ARRAY. DorisTypeConverterV2.java:168-176 widens array elements; :255-311 passes only LARGEINT source hints into map key/value reconversion, leaving unrelated map type behavior intact. isLargeintType accepts case/whitespace/display-width variants. DorisValueReader.java:185-190 consumes Arrow rows using the produced logical schema, and ArrowToSeatunnelRowReader.java:203-210 parses text directly to BigDecimal without floating-point conversion. The E2E JDBC dependency is test-scoped and supports the new real MySQL auto-DDL check. Spark projection copies omit only the three unsupported nested columns and retain top-level LARGEINT.
Before:
new DecimalType(20, 0);
builder.columnLength(20L);
// Nested Doris LARGEINT reconverted as DECIMALV3(20,0)After:
new DecimalType(DORIS_LARGEINT_PRECISION, 0); // 39
builder.columnLength(39L);
// Preserve LARGEINT source hints in scalar, ARRAY and MAP Doris outputKey Findings
- Normal Doris source schema discovery reaches this fix for both converter versions.
- This is a schema-width correction; Arrow already preserves the actual text digits as BigDecimal.
- Doris-to-Doris output must retain native LARGEINT because Doris DECIMALV3 cannot represent precision 39.
- Spark standalone remains blocked until #12862 is merged; a green historical combined branch is not proof for this exact standalone head.
- Existing downstream tables are not altered, and sinks capped at 38 still cannot hold the full signed 128-bit range.
Complete runtime path
DorisSource metadata -> DorisTypeConverterFactory -> V1/V2.convert
-> LARGEINT logical DecimalType(39,0), sourceType retained
-> scan/BE Arrow -> DorisValueReader -> ArrowToSeatunnelRowReader -> BigDecimal
-> Zeta/Flink source/transform/sink retain logical decimal
-> Jdbc save mode -> MySQL reconvert -> DECIMAL(39,0) create -> exact values
-> Doris catalog save mode -> reconvert source hints -> native LARGEINT / ARRAY / MAP
Spark standalone -> TypeConverterUtils creates Spark DecimalType(39,0) -> AnalysisException
After #12862 enters base -> top-level/ROW string carrier; nested ARRAY/MAP rejected intentionally
-> documented doris.read.field projection omits only unsupported nested columns
The effects depend on the entry and preconditions above. Recovery, batching and connector lifecycle retain their existing contracts unless explicitly described here; source inspection is not runtime proof.
1.2 Compatibility Impact
Partially incompatible. The logical source schema changes from DECIMAL(20,0) to DECIMAL(39,0), affecting auto-created sink DDL and Spark collection acceptance. Both incompatible-changes pages document widening existing DECIMAL(20,0) columns, optional CAST back to a safe narrower schema, 38-digit sink limits, and source-side omission/Zeta/Flink for nested Spark values. Existing sink tables are not automatically altered. No option rename, API/SPI signature or checkpoint serialization format change is introduced.
1.3 Performance / Side-Effect Analysis
Schema normalization/regex work occurs per column, not per source row; row values continue using BigDecimal. There are no new production threads, locks, network requests, buffers, retries, idempotency policy or resource-release changes. The test-only MySQL container adds CI time/capacity but no runtime dependency. The read loop/resource behavior is otherwise inherited, not newly proven by this PR.
1.4 Error Handling and Logging
The converter does not swallow the Spark precision error or silently round to fit. The exact current-head Build run 40828535584 fails Run / doris-connector-it on JDK 11 and 17; job 123299398276 logs AnalysisException: decimal can only support precision up to 38. The previously mentioned unrelated unit-test failure is not in the current failed-job set. The current standalone source is missing the required Spark carrier, rather than needing an unrelated dependency patch.
Issue 1: Land the Spark wide-decimal prerequisite before this PR
- Location:
seatunnel-connectors-v2/connector-doris/src/main/java/org/apache/seatunnel/connectors/doris/datatype/AbstractDorisTypeConverter.java:225-234 - Problem description: The ordinary source schema now advertises precision 39, but the exact base does not contain WideDecimalUtils or the carrier implementation. Live prerequisite #12862 is OPEN, head 1ce4e26. Current doris-connector-it failures on both JDKs reproduce Spark schema precision rejection. This is the existing merge-order blocker, not an additional converter defect.
- Potential risk: Merging this head first regresses Spark jobs that previously accepted small LARGEINT values under the old 20-digit schema; they now fail before reading any row.
- Best improvement / root-cause and location assessment: Merge #12862 first, then have the author update this branch from the base containing that fix and run Build on the resulting exact head, including both Spark versions. A sync to the current base alone will not solve it while #12862 remains unmerged. A combined run is useful integration evidence but does not satisfy the standalone merge gate. Do not add unrelated source patches to chase the expected failure.
- Severity: High; blocking.
- Raised by another reviewer: No.
2. Code Quality Assessment
2.1 Coding Standards
The complete changed-file set, imports and removed lines were inspected. New normalization and map source-hint helpers explain their constraints; the precision constant documents signed-128-bit sizing. License headers are present on new files. The engine-specific config copies document the exact permitted projection difference. Preserve those copies deliberately when their base jobs change.
2.2 Test Coverage and Test Stability
Stability rating: Stable. DorisLargeintReadTest.java:52-116 uses fixed Arrow buffers/values and try-with-resources, with no sleeps, ports, clocks or shared mutable state. Converter tests assert exact scalar/array/map schemas and spelling variants. DorisLargeintToMysqlIT.java:90-141 drops the target per engine invocation, checks information_schema precision/scale and exact ordered values including null; JDBC resources and containers are closed. DorisIT.java:341-376 asserts specific Spark error code/path/table, so an arbitrary job failure cannot pass. Modified metadata comparisons use LinkedHashMap order, but DorisCatalogUtil.TABLE_SCHEMA_QUERY:67-71 explicitly orders by ORDINAL_POSITION, so this is not unspecified query ordering. Existing data-readiness polling remains. A deterministic missing prerequisite is a merge-order/CI blocker, not itself a flaky-test rating; this corrects my earlier conflation. No local tests ran.
Local verification results: exact-head git fetch and isolated review branch seatunnel-review-12787-20261029 created; full git diff 886a3ac6e59aad5e46755761cf9fb8ac2a6d3df9...e1ddfb581dcc85db1584a14358c51a9d49ca5f57 read (26 files); git diff --check passed; git show, rg and line-numbered source/caller/config/test reads used for the trace above. GitHub changed filenames are reconciled with this local diff before publication. No local compile, Maven verify, unit/IT/E2E, Docker, service or UI execution occurred, as required for this review task. No build/deployment artifact was reused; engine-ui was not built.
2.3 Documentation Updates
Doris source mapping and migration entries are matched in en/zh. They distinguish existing tables, 38-digit targets and Spark nested-type rejection and preserve source-side workarounds. The Spark capability text is conditional on landing #12862 first, as required by Issue 1.
3. Architectural Soundness
3.1 Elegance of the Solution
Precise fix.
3.2 Maintainability
Best change-location assessment: Widening the source schema at its Doris converter is the root-cause fix. Keep shared Spark transport in its separate prerequisite. A connector-specific cast to precision 38 would hide range loss and would be inferior to the coordinated carrier.
3.3 Extensibility
Keep existing connector identities and contracts. Reuse the current boundary and regression fixtures; no shared-framework redesign or unrelated dependency is needed for the behavior reviewed here.
3.4 Historical-Version Compatibility
The logical source schema changes from DECIMAL(20,0) to DECIMAL(39,0), affecting auto-created sink DDL and Spark collection acceptance. Both incompatible-changes pages document widening existing DECIMAL(20,0) columns, optional CAST back to a safe narrower schema, 38-digit sink limits, and source-side omission/Zeta/Flink for nested Spark values. Existing sink tables are not automatically altered. No option rename, API/SPI signature or checkpoint serialization format change is introduced. This review does not claim an executed upgrade/recovery validation.
4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | Land the Spark wide-decimal prerequisite before this PR | seatunnel-connectors-v2/connector-doris/src/main/java/org/apache/seatunnel/connectors/doris/datatype/AbstractDorisTypeConverter.java:225-234 |
High, blocking |
Completeness check: full diff and affected entry/call chain, failure/resource paths, compatibility, tests and stability, bilingual docs, configuration/registration and current CI/merge gates were inspected before enumerating these findings. No additional finding is reserved for a later round on this version.
Community review reconciliation: No non-Daniel review or inline technical finding was present in the collected history. The author has explicitly acknowledged #12862 merge order and provided historical combined builds; those replies are acknowledged here in one consolidated review. Earlier source findings remain closed. Historical Daniel requested changes may remain visible under READ permissions; this technical converter clearance does not clear the dependency or perform maintainer approval.
5. Merge Recommendation
Conclusion: Ready to merge after fixes
- Blockers — must be fixed: Issue 1: Land the Spark wide-decimal prerequisite before this PR; Required current-head CI and effective maintainer review/merge gates must be satisfied.
- Recommended fixes — non-blocking: No additional unrelated redesign is requested.
Overall assessment: Widening the source schema at its Doris converter is the root-cause fix. Keep shared Spark transport in its separate prerequisite. A connector-specific cast to precision 38 would hide range loss and would be inferior to the coordinated carrier. Current head e1ddfb581dcc85db1584a14358c51a9d49ca5f57; base dev at 886a3ac6e59aad5e46755761cf9fb8ac2a6d3df9. Checks: Signal pull request update=SUCCESS; Build=FAILURE. Check references: Signal pull request update Build. Compare: diverged, ahead 5, behind 5; merge state blocked / BLOCKED. Behind/diverged status alone is synchronization metadata and does not justify a sync-and-rerun instruction. Daniel has READ permission: this COMMENT is a technical conclusion, not an effective APPROVED event or dismissal of historical requested changes. A write-capable maintainer owns final approval/dismissal where required. Current failing checks: JDK 17 Doris IT and JDK 11 Doris IT. Upstream-fix check: #12862 is still OPEN and is absent from this base, so sync alone is not a cure yet. Wait for that fix to land, then sync and rebuild; do not rerun unchanged standalone Spark lanes expecting a different schema contract. Thanks again; happy to help with the focused next revision.
Purpose of this pull request
Fixes #12753.
AbstractDorisTypeConverter#convertdeclared DorisLARGEINT(128-bit signed integer, up to 39 digits) asDECIMAL(20, 0), so any non-Doris sink failed on values longer than 20 digits. Doris → Doris only worked because of thesourceType = largeintspecial case inreconvert().This PR declares scalar
LARGEINTand theARRAY<largeint>element type asDECIMAL(39, 0), and keeps the Doris → Doris round trip mapping back toLARGEINT.Does this PR introduce any user-facing change?
Yes. The source schema of a Doris
LARGEINTcolumn changes fromDECIMAL(20, 0)toDECIMAL(39, 0). Docs (en/zh) updated.Engine limit found: Spark cannot represent
DECIMAL(39, 0)As asked in #12753, describing it here instead of clamping:
seatunnel-translation-spark-common,TypeConverterUtils.convert(line 111), which buildsorg.apache.spark.sql.types.DecimalType(39, 0).org.apache.spark.sql.AnalysisException: decimal can only support precision up to 38SourceExecuteProcessor.execute→DataFrameReader.load→BatchSourceReader.readSchema→MultiTableManager.getTableSchema→TypeConverterUtils.parcel.LARGEINTcolumn fails on Spark, regardless of the actual values. Before this PR, the same tables worked on Spark as long as values fit in 20 digits.[6]Spark 2.4,[7]Spark 3.3):DorisLargeintToMysqlIT.testLargeintToMysqlAutoCreate(new)DorisIT.testDoris,DorisIT.testDorisDirectToBe,DorisIT.testNoSchemaDoris(existing, passed before this PR)DorisMultiReadIT.testDorisMultiRead(existing, passed before this PR)Spark policy is tracked separately in #12814. Per the discussion in #12753, this PR will not disable the Spark tests, clamp to 38 digits, or add a fallback to the shared Spark translator. It stays focused on the Doris source representation and its Zeta/Flink coverage, and should not be merged until the Spark policy in #12814 is agreed and implemented.
How was this patch tested?
DorisTypeConvertorV1Test,DorisTypeConvertorV2Test, newDorisLargeintReadTest.DorisLargeintToMysqlIT(DorisLARGEINT→ MySQLDECIMAL(39,0), including min/max 128-bit values); updatedDorisIT,DorisMultiReadIT. Passing on Zeta and Flink; failing on Spark as described above.Check list
release-note.🤖 Generated with Claude Code