Skip to content

[Fix][Connector-V2][Doris] Declare LARGEINT as DECIMAL(39, 0) - #12787

Open
zhengxiang378928908-code wants to merge 5 commits into
apache:devfrom
zhengxiang378928908-code:fix/doris-largeint-decimal39
Open

zhengxiang378928908-code wants to merge 5 commits into
apache:devfrom
zhengxiang378928908-code:fix/doris-largeint-decimal39

Conversation

@zhengxiang378928908-code

@zhengxiang378928908-code zhengxiang378928908-code commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Depends on #12862. Merge #12862 first, then this PR. On its own this PR fails on the Spark containers (decimal can only support precision up to 38).

Purpose of this pull request

Fixes #12753.

AbstractDorisTypeConverter#convert declared Doris LARGEINT (128-bit signed integer, up to 39 digits) as DECIMAL(20, 0), so any non-Doris sink failed on values longer than 20 digits. Doris → Doris only worked because of the sourceType = largeint special case in reconvert().

This PR declares scalar LARGEINT and the ARRAY<largeint> element type as DECIMAL(39, 0), and keeps the Doris → Doris round trip mapping back to LARGEINT.

Does this PR introduce any user-facing change?

Yes. The source schema of a Doris LARGEINT column changes from DECIMAL(20, 0) to DECIMAL(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:

  • Component: seatunnel-translation-spark-common, TypeConverterUtils.convert (line 111), which builds org.apache.spark.sql.types.DecimalType(39, 0).
  • Error: org.apache.spark.sql.AnalysisException: decimal can only support precision up to 38
  • Stage: source schema translation, before any row is read: SourceExecuteProcessor.execute → DataFrameReader.load → BatchSourceReader.readSchema → MultiTableManager.getTableSchema → TypeConverterUtils.parcel.
  • Scope: Spark 2.4 and Spark 3.3 only. Zeta and Flink pass. Because the failure is at schema time, any Doris table that has a LARGEINT column 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.
  • CI evidence (fork run https://github.com/zhengxiang378928908-code/seatunnel/actions/runs/38510021504), same result on JDK 11 and 17; every failure is a Spark container ([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?

  • Unit: DorisTypeConvertorV1Test, DorisTypeConvertorV2Test, new DorisLargeintReadTest.
  • E2E: new DorisLargeintToMysqlIT (Doris LARGEINT → MySQL DECIMAL(39,0), including min/max 128-bit values); updated DorisIT, DorisMultiReadIT. Passing on Zeta and Flink; failing on Spark as described above.

Check list

🤖 Generated with Claude Code

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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): LARGEINT now maps to new DecimalType(DORIS_LARGEINT_PRECISION, 0) with columnLength = 39 (was 20).
  • DorisTypeConverterV2.convertArray (DorisTypeConverterV2.java:172-174): ARRAY<largeint> elements become DECIMAL(39, 0).
  • AbstractDorisTypeConverter.reconvert, ARRAY branch (AbstractDorisTypeConverter.java:401-408): when the source type equals ARRAY<largeint>, emit ARRAY<LARGEINT> instead of ARRAY<DECIMALV3(p, s)>.
  • DorisTypeConverterV2.reconvertMap and the new mapLargeintSourceTypes: pass the LARGEINT source type of a map key/value down so it is kept as LARGEINT.
  • Docs: the source mapping row changes from STRING to DECIMAL(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 LARGEINT column) reaches the changed logic through DorisTypeConverterV2.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: LARGEINT is preserved through the source type, and the DDL for ARRAY and MAP also keeps LARGEINT.
  • DecimalType(39, 0) is not representable on the Spark engine. CI on the PR author's fork shows AnalysisException: decimal can only support precision up to 38 thrown from TypeConverterUtils.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.md is 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) to DECIMAL(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 native LARGEINT instead of DECIMALV3(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: DorisIT 6, DorisMultiReadIT 2, DorisLargeintToMysqlIT 2, all on the Spark containers) shows AnalysisException: decimal can only support precision up to 38 from DecimalType.<init> called by TypeConverterUtils.convert(TypeConverterUtils.java:111) via MultiTableManager.getTableSchema and BatchSourceReader.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 STRING plus keep the LARGEINT source type, so Doris sinks stay lossless and other sinks get a varchar (this matches what the docs said before this PR); (b) cap at DECIMAL(38, 0) and document that values beyond 38 digits are not supported on that path; (c) keep DECIMAL(39, 0) only if the Spark translation gets an explicit, tested handling. Whichever is chosen must keep the DorisIT/DorisMultiReadIT Spark 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:51 and the zh counterpart.
  • Problem description: The declared type of every Doris LARGEINT column changes (DECIMAL(20, 0) to DECIMAL(39, 0), or whatever Issue 1 settles on), which changes auto-created downstream column types. The project guideline requires such changes to be listed in incompatible-changes.md with 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/sql transform to DECIMAL(38, 0) or STRING).
  • 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; E2E DorisLargeintToMysqlIT.java only 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) where DORIS_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 like array<largeint(40)> would fall through to ARRAY<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 extractArrayType and compare the element type to LARGEINT.
  • 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; testReconvertMapWithLargeint covers key, value and unchanged mapping; DorisLargeintReadTest.testReadLargeintFromArrowUtf8 reads real extrema through ArrowToSeatunnelRowReader. These would fail if the mapping were reverted to precision 20, which is good. testLargeintTypeHoldsWholeRange is 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 DorisIT caught 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, executeJob exit code 1 asserted in DorisLargeintToMysqlIT). Also in the same CI run DorisIT shows 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 alias mysql_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

  1. Blockers - must be fixed
    • Issue 1: the Spark containers of DorisIT, DorisMultiReadIT and the new DorisLargeintToMysqlIT fail with decimal can only support precision up to 38; CI on the head commit is red for this real, PR-caused reason (failing job doris-connector-it).
    • Issue 2: record the schema contract change in incompatible-changes.md once the final type is decided.
  2. 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.

@zhengxiang378928908-code

Copy link
Copy Markdown
Contributor Author

@SEZ9 @DanielLeens Both items are pushed in 521c27f:

  • incompatible-changes.md (en/zh): records the DECIMAL(20, 0) → DECIMAL(39, 0) change, its effect on auto-created tables (MySQL DECIMAL(39,0); 38-max sinks such as Oracle and SQL Server clamp to DECIMAL(38,0) with a warning), Doris to Doris now keeping LARGEINT in arrays and maps, and a CAST workaround. The Spark behavior is left out until the policy in [Discussion][Translation][Spark] Explicit policy for DECIMAL precision > 38 on the Spark engine #12814 is settled, and I'll add it then.
  • ARRAY<LARGEINT> matching: array elements and map keys/values are now detected by normalizing the type (case, whitespace, optional display width such as largeint(40)) instead of an exact string match, with tests for those variants.

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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: new isLargeintType / isLargeintArrayType helpers normalize case, whitespace and an optional display width (largeint(40)), and reconvert for ARRAY now uses isLargeintArrayType(column.getSourceType()) instead of the exact-string equalsIgnoreCase.
  • DorisTypeConverterV2.java: convertArray and mapLargeintSourceTypes use the same helper, so array and map variants are matched on the element type.
  • incompatible-changes.md (en and zh): new entry for the DECIMAL(20, 0) to DECIMAL(39, 0) change with impact and a CAST workaround.
  • DorisTypeConvertorV2Test: new cases for array<largeint(40)>, ARRAY< LARGEINT > and map<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 uses equalsIgnoreCase(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-225 triggers it; it throws in seatunnel-translation-spark-common TypeConverterUtils.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.testDorisMultiRead and DorisLargeintToMysqlIT.testLargeintToMysqlAutoCreate, each with AnalysisException: 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)

  1. Blockers - must be fixed
    • Issue 1: land the explicit Spark policy from #12814 (with row write/read and nested-type paths covered, as @SEZ9 asked), then re-run doris-connector-it so the Spark containers are green together with this change.
  2. Recommended fixes - non-blocking
    • Use the same tolerant matching in the scalar reconvert branch for consistency.

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.

@zhengxiang378928908-code

Copy link
Copy Markdown
Contributor Author

@DanielLeens Applied the optional suggestion in 9fad9b5: the scalar LARGEINT branch in AbstractDorisTypeConverter.reconvert now uses isLargeintType, the same as the array branch. Before this, a source type such as largeint(40) or LargeInt fell through to VARCHAR(200); testReconvertDecimal now covers those spellings. The Spark containers are still expected to fail until #12814 lands.

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 from column.getSourceType().equalsIgnoreCase(DORIS_LARGEINT) to isLargeintType(column.getSourceType()). isLargeintType is null-safe and strips whitespace, upper-cases, and accepts an optional display width, so largeint(40), LargeInt and LARGEINT (40) now map back to LARGEINT instead of falling through to VARCHAR(200).
  • DorisTypeConvertorV2Test.testReconvertDecimal: loops over five spellings and asserts both columnType and dataType equal LARGEINT. 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 LARGEINT column) reaches this logic through DorisTypeConverterV2.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 by DorisLargeintReadTest).
  • 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 (LARGEINT to DecimalType(DORIS_LARGEINT_PRECISION, 0)); it throws in seatunnel-translation-spark-common TypeConverterUtils.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 are DorisIT.testDoris, DorisIT.testDorisDirectToBe, DorisIT.testNoSchemaDoris, DorisMultiReadIT.testDorisMultiRead and DorisLargeintToMysqlIT.testLargeintToMysqlAutoCreate, each listed twice (the two Spark containers), with AnalysisException: 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 LARGEINT column 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-it so 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 (GitHub mergeable_state is dirty; compare status is diverged, 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.md in particular is edited by many PRs) may have changed on dev; 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 in testReconvertDecimal guards 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

  1. Blockers - must be fixed
    • Issue 1: implement the Spark policy from #12814 and get doris-connector-it green on the Spark containers.
    • Issue 2: resolve the merge conflicts with dev and re-run CI on the resolved head.
  2. 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.

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@zhengxiang378928908-code

Copy link
Copy Markdown
Contributor Author

Thanks @DanielLeens for the review.

The Spark implementation for #12814 is now open as #12862. The Build check here stays red on the Spark containers until that lands, since this PR alone still hits Spark's 38-digit limit.

Testing both together showed one more Doris issue: the shared Doris test tables have MAP<SMALLINT, LARGEINT>, MAP<LARGEINT, DOUBLE> and ARRAY<LARGEINT> columns, which the agreed design rejects on Spark at job start. So DorisIT / DorisMultiReadIT would still fail on Spark by design. Pushed 73f5246 to keep the Doris Spark containers running:

  • On Spark the same jobs run from spark/*.conf, which leave out only those three columns with doris.read.field (the documented workaround). Top-level LARGEINT is still read and written on Spark.
  • Sink checks now compare columns by name and select explicit column lists, so they also work when the Spark sink table has fewer columns.
  • New Spark-only DorisIT.testNestedLargeintFailsOnSpark checks that each of the three columns fails with SPARK-TRANSLATION-01 and names the table and column path (.value, .key, .element).

I'm running the combined CI on my fork (this PR + #12862) and will post the result here.

@zhengxiang378928908-code

Copy link
Copy Markdown
Contributor Author

Combined CI result (this PR at 73f5246 + #12862, fork run 39139304609):

  • doris-connector-it passes on JDK 11 and 17, including the Spark 2.4/3.3 containers and testNestedLargeintFailsOnSpark.
  • 82 jobs passed. The only other job, all-connectors-it-6 (JDK 17), hit the 150-minute timeout and was cancelled; the same part passed on JDK 11. I've re-run it.

For comparison, this PR alone (39139300360) fails only in the Spark containers with decimal can only support precision up to 38, as expected until #12862 is merged.

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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/value

Runtime 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/DorisMultiReadIT pass 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 new spark/*.conf variants (the same config with a shorter doris.read.field, I diffed spark/doris_source_and_sink.conf against the original). On Zeta and Flink readColumns returns 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.testNestedLargeintFailsOnSpark asserts exit code != 0 and SPARK-TRANSLATION-01 plus 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/*.conf jobs) and the doris-connector-it Spark containers ([6] Spark 2.4, [7] Spark 3.3)
  • Problem description: On the current head the Apache-side Build is red. The fork run for this exact head (39139300360) fails only in doris-connector-it (JDK 11 and 17) with AnalysisException: decimal can only support precision up to 38 on 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 against dev. Your combined run (this PR + #12862) shows doris-connector-it green on JDK 11 and 17.
  • Potential risk: Merging this PR first would make every Doris table with a LARGEINT column 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.md and the zh counterpart (the new Doris section)
  • Problem description: Together with #12862 the Spark outcome becomes: top-level LARGEINT goes through the string carrier, and ARRAY<LARGEINT> or MAP with LARGEINT now fails at job start with SPARK-TRANSLATION-01 (before this change those columns worked as DECIMAL(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.field workaround 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

  1. Blockers

    • Land #12862 first (or combine), then a green Build on this PR.
    • Issue 2: the Spark migration note, in whichever PR lands last.
  2. 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!

zhengxiang378928908-code added a commit to zhengxiang378928908-code/seatunnel that referenced this pull request Oct 19, 2026
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>
@zhengxiang378928908-code

Copy link
Copy Markdown
Contributor Author

Thanks, @DanielLeens. Pushed ab885d5:

  • Issue 1: added "Depends on [Fix][Translation][Spark] Carry DECIMAL with precision above 38 through Spark #12862" at the top of the description. [Fix][Translation][Spark] Carry DECIMAL with precision above 38 through Spark #12862 should land first.
  • Issue 2: since this PR lands last, the Spark note is here, in incompatible-changes.md (en/zh): top-level and ROW-nested LARGEINT is carried as text on Spark and restored to DECIMAL(39, 0); LARGEINT inside ARRAY/MAP now fails at job start with SPARK-TRANSLATION-01 (it worked before as DECIMAL(20, 0)). Migration: leave those columns out with doris.read.field, or run on Zeta/Flink. An SQL transform can't help there, because Spark fails while building the source schema.
  • Issue 3: assertNestedLargeintRejected now checks only the error code, the column path and the table, not the message wording. Each spark/*.conf header now says it differs from its base job only in doris.read.field and must be kept in sync (I diffed all eight against their base jobs to confirm).

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 DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 and ROW-nested LARGEINT is carried through Spark as decimal text and restored to DECIMAL(39, 0); LARGEINT inside ARRAY/MAP now fails at job start with SPARK-TRANSLATION-01. The migration guide adds the doris.read.field workaround and says an SQL transform cannot help. The zh text matches the en text.
  • DorisIT.assertNestedLargeintRejected (DorisIT.java around 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/*.conf headers now say "Only doris.read.field differs; keep everything else in sync".

Key findings

  • The normal path (any Doris source with a LARGEINT column) reaches the changed logic through DorisTypeConverterV2.convert -> sampleTypeConverter; this is the main path, not an edge-path fix. The connector code is unchanged since my approval-level read on ce7690a2d.
  • The negative test is a small, intentional loosening of message wording. The original intent (SPARK-TRANSLATION-01 plus 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, Build not 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 of doris-connector-it)
  • Problem: On this head the Apache-side Build is red. The fork run for this exact head (39193306399, this PR alone) fails only doris-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 was decimal can only support precision up to 38 on 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 on 73f52467c was green for doris-connector-it.
  • Risk: Merging this PR before #12862 would make every Doris table with a LARGEINT column 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

  1. Blockers (must fix)
    • Merge #12862 first, then sync this branch and show a green Build (including the Spark lanes).
  2. 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!

@zhengxiang378928908-code

Copy link
Copy Markdown
Contributor Author

Combined CI on the current heads (this PR at ab885d5cd + #12862 at 36e16f44f, merge commit f709578bc): fork run 39193334501 is green, 83 jobs passed, 0 failed. That includes doris-connector-it on JDK 11 and 17 with the Spark 2.4/3.3 containers and testNestedLargeintFailsOnSpark.

This PR's own run (39193306399) still fails only doris-connector-it, on the Spark precision-38 limit, as expected until #12862 lands. After that I'll sync with dev and post a green Build.

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 39193334501 is a merge commit (f709578) of this PR head ab885d5cd and #12862 head 36e16f44f. It finished with 83 jobs passed, 0 failed and 11 skipped, and doris-connector-it passed on JDK 11 and 17, including the Spark containers and testNestedLargeintFailsOnSpark.
  • #12862 on its own head 36e16f44f also has a green Build on the Apache side, so the Spark change looks healthy.
  • The Build check on this PR (ab885d5cd alone) is still red, as expected: only doris-connector-it fails, 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!

zhengxiang378928908-code added a commit to zhengxiang378928908-code/seatunnel that referenced this pull request Oct 24, 2026
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 DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:98 DORIS_LARGEINT_PRECISION = 39; :106 isLargeintType (case, whitespace and display width tolerant); :355 scalar reconvert keeps LARGEINT when the source type is LARGEINT; :447 ARRAY<LARGEINT> is kept as ARRAY<LARGEINT> instead of DECIMALV3(39, 0) elements.
  • DorisTypeConverterV2.java:171 array element LARGEINT -> DecimalArrayType(DecimalType(39, 0)); :258 / :294 mapLargeintSourceTypes passes LARGEINT source types to the map key/value columns so a Doris sink keeps LARGEINT.
// before
decimalType = new DecimalType(20, 0);
// after
decimalType = new DecimalType(DORIS_LARGEINT_PRECISION, 0); // 39

Key findings

  • The normal path reaches the change: every Doris source table with a LARGEINT column goes through DorisTypeConverterV2.convert. This is a main-path fix, not an edge-path fix.
  • The rebase did not touch any of the files dev changed in between in a conflicting way; compare dev...dcaa9f2e8 is ahead_by=5, behind_by=0.
  • #12862 is still open, and its head has moved from 36e16f44f to 7b9d94255 ("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 40073961674 fails doris-connector-it on JDK 11 and 17. I read the log: DorisLargeintToMysqlIT and DorisMultiReadIT fail only on the two Spark containers ([6] and [7]) with AnalysisException: decimal can only support precision up to 38, which is exactly the effect #12862 removes. The same run also fails unit-test (11) in SeaTunnelClientTest.testSetJobIdDuplicate (seatunnel-engine client), 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 no Build result attached yet.
  • Risk: Merging this PR before #12862 makes every Doris table with a LARGEINT column 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 Build on its own head. Until then, a fresh combined fork run against the current #12862 head 7b9d94255 would 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

  1. Blockers (must fix)
    • #12862 must be merged first; then this branch needs a green Build (including the Spark containers) on its own head.
  2. Recommended fixes (non-blocking)
    • Optionally re-run the combined fork build against the current #12862 head 7b9d94255, and re-run the unrelated unit-test (11) job when you next push.

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!

@DanielLeens

Copy link
Copy Markdown
Contributor

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 dcaa9f2e82109f99df43b4d8953aaf8db740b296: Build is FAILURE.

The blocking jobs are Run / unit-test (11, ubuntu-latest) failure; Run / doris-connector-it (17, ubuntu-latest) failure; Run / doris-connector-it (11, ubuntu-latest) failure.

Observed log evidence: DorisIT.testNoSchemaDoris:248 expected: <0> but was: <1> DorisMultiReadIT.testDorisMultiRead:134 expected: <0> but was: <1>

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 dev. Please sync the latest base and resolve the conflicts first; the resulting new head needs checks and re-review. This conflict gate is independent of CI and existing source findings.

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>

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 output

Key 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

  1. 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.
  2. 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.

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [Connector-V2] [Doris] LARGEINT is declared as DECIMAL(20,0), so non-Doris sinks fail on values over 20 digits

2 participants