[Improve][Connector-V2] Add support for BFILE and INTERVAL data types in Oracle connector - #11825
[Improve][Connector-V2] Add support for BFILE and INTERVAL data types in Oracle connector#11825corgy-w wants to merge 2 commits into
Conversation
… in Oracle connector
DanielLeens
left a comment
There was a problem hiding this comment.
What Problem Does This PR Solve?
The PR title claims to add Oracle BFILE and INTERVAL data-type support to the JDBC connector. Looking at the actual diff:
- INTERVAL is genuinely new:
OracleTypeConverter.convert()previously had nocaseforINTERVAL DAY TO SECOND/INTERVAL YEAR TO MONTH, so any Oracle table containing an interval column would hit thedefaultbranch and throwCommonError.convertToSeaTunnelTypeError, failing the whole job at schema-resolution time. This PR adds a case that strips the precision qualifiers (INTERVAL DAY(2) TO SECOND(6)→INTERVAL DAY TO SECOND) and maps the column toSTRING_TYPE. - BFILE is not new at the type-mapping layer:
OracleTypeConverter.java:232-235(case ORACLE_BFILE: builder.dataType(PrimitiveByteArrayType.INSTANCE); ...) is pre-existing, untouched code ondev— it is context in the diff, not an added line. What this PR actually adds for BFILE is a new column in the E2E fixture table plus special-case handling in the test'sinsertTestData()to always insertNULLfor that column (BFILE values can't be set via generic JDBCsetObject/setBytes— they requireBFILENAME('DIR','file')server-side). So in practice this PR adds E2E scaffolding for BFILE, not new runtime support, and (see Issue 3) that scaffolding never actually exercises a real BFILE value.
1. Code Change Review
1.1 Core Logic Analysis
OracleTypeConverter.java:127-132 strips (...) precision qualifiers only when oracleType.startsWith(ORACLE_INTERVAL), so the regex is properly scoped and cannot affect any other type's dispatch (e.g. NUMBER(10,2), whose precision/scale come from typeDefine.getPrecision()/getScale(), not string parsing). After stripping, "INTERVAL DAY(2) TO SECOND(6)" → "INTERVAL DAY TO SECOND" which correctly matches ORACLE_INTERVAL_DAY, and similarly for ORACLE_INTERVAL_YEAR. This part is sound and minimal.
The real question — does the normal read/write path for a table containing these columns actually reach the new code and produce correct data — is where I found problems (Issues 1-3 below). OracleTypeConverter only defines the catalog/schema mapping; I traced the runtime value path:
- Read:
AbstractJdbcRowConverter.toInternal()(Oracle does not override this) dispatchesBYTES→JdbcFieldTypeUtils.getBytes(rs, idx)→ a plainresultSet.getBytes(columnIndex). There is no Oracle-specific or BFILE-specific override anywhere inconnector-jdbc(verified via a full case-insensitive grep of the module — the only other BFILE references are unrelated DDL-builder/DM-database code). - Write:
OracleJdbcRowConverter.setValueToStatementByDataType()(OracleJdbcRowConverter.java:142-149) only special-casesORACLE_BLOBforBYTES; every otherBYTESsource type, includingBFILE, falls through tostatement.setBytes(...).
See Issue 3 for why this matters for BFILE specifically.
1.2 Compatibility Impact
Fully compatible. This is a purely additive change:
INTERVALcolumns previously caused a hardconvertToSeaTunnelTypeErrorfor any Oracle catalog read; now they resolve toSTRING_TYPEinstead of erroring. No existing (previously-working) type mapping's behavior, precision, or default is altered.BFILE's catalog mapping is untouched.- No
Optionrenamed/removed, no default value changed, no SPI contract touched.
The only thing to flag under compatibility is expectation-setting: a user who previously got a clear "unsupported type" error for INTERVAL columns will now get the job proceeding, but (per Issue 2) whether round-tripping actual interval values works end-to-end is not demonstrated by the tests as written.
1.3 Performance / Side-Effect Analysis
Negligible. The new regex strip only runs once per column during catalog/schema resolution (not on the per-row hot path), and only for columns whose type name starts with INTERVAL. No new buffering, streaming, threading, or locking is introduced. BFILE handling, if it were implemented (see Issue 3), would need to be mindful of LOB-locator streaming cost, but as it stands no such code exists yet to evaluate.
1.4 Error Handling and Logging
No new logging was added, consistent with the sibling STRING-producing cases (most of them don't log either), so this isn't a regression in style. However, see Issue 6: mutating oracleType in-place before the switch means that if some interval-shaped-but-unmatched type ever reaches the default branch, the thrown CommonError.convertToSeaTunnelTypeError will report the stripped type name rather than the original raw type reported by the driver, which slightly hurts diagnosability.
Issue 1: defaultCompare field list is inconsistent with 3 of the 4 job configs it verifies — guaranteed test failure
- Location:
seatunnel-e2e/.../connector-jdbc-e2e-part-1/src/test/java/.../JdbcOracleIT.java:139-161(staticfieldNames, now 20 columns incl.BFILE_COL/INTERVAL_COL) vs..../src/test/resources/jdbc_oracle_source_to_sink_use_select1.conf,..._select2.conf,..._select3.conf(SELECT/INSERT column lists not updated by this PR, still the old 18-column list). - Problem:
testJdbcDb()(AbstractJdbcIT.java:394-413) loops over all 4 files inCONFIG_FILE, runs each job, and callscheckResult→defaultCompare(executeKey, fieldNames, "INTEGER_COL")(JdbcOracleIT.java:276-278), which always uses the full, now-20-columnfieldNamesarray regardless of which config just ran.insertTestData()populates the source table once, up front, with realINTERVAL_COLvalues (Duration.ofHours(25)per row,JdbcOracleIT.java:314-315) for all 20000 rows, independent of which config will later run. Forselect1/2/3.conf, whose SELECT/INSERT statements were not extended to includeINTERVAL_COL, the job never writes that column into the sink table, so it staysNULLthere while the source has a real, non-null value in every row.defaultCompare'sAssertions.assertArrayEquals(sourceResult, sinkResult, ...)(AbstractJdbcIT.java:650-651) will then fail for those 3 test iterations. (BFILE_COLhappens to be NULL on both sides so it doesn't trip this, butINTERVAL_COLwill.) I confirmed this is a regression introduced by this PR and not a pre-existing test quirk: before this PR,fieldNameswas 18 columns andselect1/2/3.conf's query lists were exactly those same 18 columns — i.e. the invariant "compare-field-list ⊆ what each job variant actually writes" held. This PR breaks that invariant by extending onlyfieldNamesand the main config, not the three sibling configs. - Potential risk:
testJdbcDbwill fail (not flaky — deterministic, every row) for theselect1/select2/select3config-file iterations once the Oracle E2E job actually runs in CI. - Best improvement: Either add
BFILE_COL/INTERVAL_COLto the SELECT/INSERT lists in all three sibling.conffiles (mirroring the main config's update), or keep a separate, narrowerfieldNamessubset for the comparison used by those config variants. - Severity: High (blocking — will fail CI for this test class once it runs).
- Raised by another reviewer: No.
Issue 2: Unverified PreparedStatement.setObject(..., Duration) support for INTERVAL DAY TO SECOND on the ojdbc8 12.2.0.1 driver used by this test
- Location:
JdbcOracleIT.java:372-399(insertTestData()),JdbcOracleIT.java:314-315(Duration.ofHours(25)used as theINTERVAL_COLinsert value),JdbcOracleIT.java:281-283(driverUrl()pinsojdbc8-12.2.0.1). - Problem:
java.time.Duration→ OracleINTERVAL DAY TO SECONDis not part of the standard JDBC 4.2 object-mapping table; Oracle's own extension type for this isoracle.sql.INTERVALDS. Whether the (relatively old)ojdbc8 12.2.0.1driver accepts a rawDurationvia the genericPreparedStatement.setObject(int, Object)overload for anINTERVAL DAY TO SECONDtarget column is not something I can confirm from source alone, and I did not find any prior use of this pattern elsewhere in the codebase to corroborate it. - Potential risk: If unsupported,
insertTestData()'scatch (Exception exception)(JdbcOracleIT.java:395-398) converts it into aSeaTunnelRuntimeException, which is thrown out of@BeforeAll startUp()— every@Test/@TestTemplatemethod inJdbcOracleITwould then fail at setup, not just the interval-specific assertions. - Best improvement: Please confirm (ideally via the queued CI run, or a quick local check against the same driver version) that this insert actually succeeds, and consider inserting a driver-native representation (e.g. an interval literal string via
TO_DSINTERVAL(...)in SQL, ororacle.sql.INTERVALDS) ifDurationturns out to be unsupported. - Severity: High (blocking-if-confirmed; flagged pending the still-queued CI run since I cannot execute tests locally per review policy).
- Raised by another reviewer: No.
Issue 3: "BFILE support" is unimplemented and untested for any real (non-null) value — read path will most likely throw, not materialize file bytes
- Location:
OracleTypeConverter.java:232-235(pre-existingORACLE_BFILE → PrimitiveByteArrayType, unchanged by this PR);AbstractJdbcRowConverter.java:126-128(genericBYTESread →JdbcFieldTypeUtils.getBytes→ plainresultSet.getBytes(columnIndex), no Oracle/BFILE override exists anywhere in the module);OracleJdbcRowConverter.java:142-149(write path only special-casesORACLE_BLOB, notORACLE_BFILE);JdbcOracleIT.java:382-385(insertTestData()explicitlysetNull(index+1, OracleTypes.BFILE)for every row, never a real locator). - Problem:
BFILEis a locator to a file on the database server's OS filesystem (via a OracleDIRECTORYobject + filename), not LOB payload stored in the row — reading it requires driver-specific handling (cast tooracle.sql.BFILE, thenopenFile()/getBinaryStream()), analogous to how this same class already special-casesBLOBon the write side. Nothing inconnector-jdbcdoes this for reads; the genericresultSet.getBytes()call this PR relies on will most likely throw aSQLExceptionfor a real, non-null BFILE column rather than transparently return the referenced file's bytes (driver behavior here is known to be inconsistent/version-dependent, but "silently returns correct bytes" is not the expected outcome). Because the E2E test only ever insertsNULLforBFILE_COL, this PR's own test suite provides zero evidence that reading an actual BFILE value works — and the sink write path (falls through tostatement.setBytes()for any non-BLOBBYTESsource, includingBFILE) would itself fail against a real BFILE target column, since BFILE is read-only via JDBC. - Potential risk: Any user who takes the PR's claim of "BFILE support" at face value and points a real (non-null) BFILE column at this connector will very likely hit an unhandled
SQLExceptionon read, or a confusing low-level JDBC error on write if BFILE is ever a sink target — with no connector-level guard or clear error message either way. - Best improvement: Either (a) implement real BFILE materialization on read (cast to
oracle.sql.BFILE, open/stream the referenced file, respectingfileExists()) and add explicit, clear-error handling for BFILE as a sink/write target (since it's fundamentally not writable via JDBC), backed by an E2E case that actually populates a non-null BFILE locator viaBFILENAME(...); or (b) if full support isn't in scope for this PR, scale back the PR description/title to accurately describe what's delivered (NULL passthrough only) so users don't rely on unverified behavior. - Severity: High.
- Raised by another reviewer: No.
2. Code Quality Assessment
2.1 Coding Standards
The production change in OracleTypeConverter.java is small, well-scoped, and follows the existing file's conventions (constant naming, switch-based dispatch, PhysicalColumnBuilder usage). The inline comment explaining the precision-qualifier stripping is genuinely useful and explains a non-obvious "why." License headers are present on modified files (no new files added). No System.out.println, no wildcard imports, no obviously risky patterns.
Issue 4: No unit test added for the new INTERVAL conversion logic
- Location:
seatunnel-connectors-v2/connector-jdbc/src/test/java/.../oracle/OracleTypeConverterTest.java— this 1000+ line file has a dedicated test method per supported type (testConvertInteger,testConvertNumber,testConvertFloat,testConvertChar,testConvertBytes,testConvertBlobAsByte,testConvertDatetime, etc.), but notestConvertInterval/equivalent was added for this PR's actual code change. - Problem: The only new production logic in this PR — the precision-qualifier-stripping regex and the new
switchcases — has zero direct unit coverage. Nothing pins down, e.g., that"INTERVAL DAY(2) TO SECOND(6)","interval day to second"(lowercase, pre-toUpperCase()), or"INTERVAL YEAR(4) TO MONTH"all resolve correctly, or that the untouchedcolumnLengthstaysnullas intended. - Potential risk: A future refactor of this method (there's a lot of type-dispatch code nearby) could silently break interval handling with nothing to catch it at the unit level — only the (currently broken, see Issue 1) E2E test would have a chance of noticing, and per Issue 1 it wouldn't even get that far cleanly.
- Best improvement: Add
testConvertIntervalcovering both interval subtypes, with and without precision qualifiers, matching the style of the existing test methods. - Severity: Medium.
- Raised by another reviewer: No.
Issue 5: INTERVAL → STRING_TYPE mapping omits columnLength, unlike every sibling STRING-producing case
- Location:
OracleTypeConverter.java:271-275. - Problem:
ORACLE_CHAR/ORACLE_VARCHAR/ORACLE_NCHAR/ORACLE_ROWID/ORACLE_XML/ORACLE_LONG/ORACLE_CLOB/ORACLE_NCLOBall callbuilder.columnLength(...)explicitly; the newINTERVALcases don't, leaving itnull. - Potential risk: Low —
reconvert()'sSTRINGbranch already treatsnull/<=0column length as "use defaultVARCHAR2(4000)" (OracleTypeConverter.java:384-388), which is a safe fallback for interval-literal-length strings. Purely a minor inconsistency, not a functional bug. - Best improvement: Set an explicit, small
columnLength(interval literals are well under 100 chars) for clarity/consistency with the rest of the file. - Severity: Low.
- Raised by another reviewer: No.
Issue 6: In-place mutation of oracleType before dispatch degrades the default-branch error message for unmatched interval-like types
- Location:
OracleTypeConverter.java:127-132(mutation),OracleTypeConverter.java:276-278(defaultbranch uses the same, now-mutated,oracleTypevariable in the thrown error). - Problem: If a future/exotic interval-shaped type name doesn't match either
ORACLE_INTERVAL_YEAR/ORACLE_INTERVAL_DAYafter stripping, theCommonError.convertToSeaTunnelTypeErrormessage will show the stripped string, not the original raw type name the driver reported — making the failure harder to diagnose. - Potential risk: Minor operability/debuggability issue only.
- Best improvement: Keep the original
typeDefine.getDataType()value around separately for the error message, or only shadow the variable within theifblock's local scope. - Severity: Low.
- Raised by another reviewer: No.
2.2 Test Coverage and Test Stability
UT/E2E test code was changed (JdbcOracleIT.java, jdbc_oracle_source_to_sink.conf), so per review policy I'm giving this a mandatory stability rating:
High risk. Issue 1 is, by static analysis of the diff and the shared AbstractJdbcIT test-loop code, a deterministic failure for 3 of the 4 config-file iterations that testJdbcDb runs (assuming insertTestData() itself succeeds — see Issue 2, which is a second, independent way this test class can fail). This isn't a flaky/environmental concern; it's a logical mismatch introduced directly by this diff between what fieldNames now claims should match and what 3 of 4 job configs actually write.
2.3 Documentation Updates
docs/en/connectors/source/Oracle.md and docs/zh/connectors/source/Oracle.md were both updated identically to add INTERVAL to the STRING row, matching the new OracleTypeConverter behavior — good, and consistent between English/Chinese. I checked docs/en/connectors/sink/Oracle.md (unchanged by this PR) and confirmed that's correct: reconvert() wasn't touched, so a SeaTunnel STRING field still only ever maps back to VARCHAR2/CLOB on write, never to an Oracle INTERVAL type — there's nothing to document there for this PR's scope.
3. Architectural Soundness
3.1 Elegance of the Solution
The core OracleTypeConverter change itself is a clean, minimal, correctly-scoped addition that follows the file's existing patterns. The problems in this review are concentrated in the E2E test scaffolding around it, not the core type-mapping logic.
3.2 Maintainability
Fine for the production code. The test-side issues (Issues 1, 2) make the test suite harder to trust/maintain going forward if merged as-is, since a passing/failing signal for this test class would no longer reliably indicate whether interval/BFILE handling actually works.
3.3 Extensibility
The startsWith(ORACLE_INTERVAL) + regex-strip pattern is a reasonable, contained way to normalize driver-reported precision-qualified type names, and could be reused if more Oracle types with embedded parenthesized qualifiers need similar handling later.
3.4 Historical-Version Compatibility
No SPI, serialization, checkpoint/savepoint, or config-option compatibility concerns — this only affects schema-resolution-time type dispatch for a previously-unsupported (hard-erroring) Oracle type, and an already-existing (unchanged) mapping for another. Nothing here touches restore/upgrade paths.
4. Issue Summary
| # | Issue | Severity | Blocking |
|---|---|---|---|
| 1 | select1/2/3.conf not updated to match extended fieldNames, causing guaranteed defaultCompare mismatch on INTERVAL_COL |
High | Yes |
| 2 | Unverified Duration → INTERVAL DAY TO SECOND setObject() support on ojdbc8 12.2.0.1 |
High | Yes (pending CI confirmation) |
| 3 | Real BFILE read/write materialization unimplemented and untested; only NULL passthrough is exercised | High | Yes |
| 4 | No unit test for new INTERVAL conversion logic in OracleTypeConverterTest |
Medium | Recommended |
| 5 | Missing columnLength for INTERVAL → STRING_TYPE mapping |
Low | No |
| 6 | Mutated oracleType degrades default-branch error message |
Low | No |
5. Merge Recommendation
Conclusion: Ready to merge after fixes
-
Blockers — must be fixed
- Issue 1: Update
jdbc_oracle_source_to_sink_use_select1.conf,..._select2.conf,..._select3.conf(SELECT and INSERT column lists) to includeBFILE_COL/INTERVAL_COL, consistent with the main config and the extendedfieldNamesarray — otherwisetestJdbcDbwill fail deterministically on 3 of its 4 iterations. - Issue 2: Confirm (via the still-queued fork CI run, or otherwise) that
PreparedStatement.setObject(idx, Duration)actually succeeds against anINTERVAL DAY TO SECONDcolumn onojdbc8 12.2.0.1; if not, switch to a driver-supported representation for the insert. - Issue 3: Either implement real (non-null) BFILE read materialization with a corresponding E2E case that populates an actual locator via
BFILENAME(...), plus explicit handling/clear errors for BFILE as a write target, or narrow the PR's scope/description so "BFILE support" isn't implied beyond NULL passthrough.
- Issue 1: Update
-
Recommended fixes — non-blocking
- Issue 4: Add
testConvertIntervaltoOracleTypeConverterTest.javacovering both interval subtypes, with/without precision qualifiers. - Issue 5: Set an explicit
columnLengthfor theINTERVAL→STRING_TYPEmapping. - Issue 6: Preserve the original (unstripped) type name for the
default-branch error message.
- Issue 4: Add
Thanks for working on filling this Oracle type-mapping gap — the core OracleTypeConverter addition for INTERVAL is a solid, well-scoped piece of work. The main thing holding this back right now is that the E2E test as written doesn't actually validate the round trip it appears to (Issue 1 in particular is a mechanical, deterministic problem that's straightforward to fix), and the BFILE side of the PR doesn't yet exercise a real, non-null value. Happy to take another look once these are addressed.
CI note: as of this review, the fork's Build workflow run (corgy-w/seatunnel, branch corgy/dev-oracle-bfile-interval, run 31931931035) has been queued for 4+ hours without starting, and the corresponding apache-side check is still QUEUED. This review is based entirely on static source analysis; please re-check CI once it completes, since I'd expect Issue 1 (and possibly Issue 2) to surface there directly.
Purpose of this pull request
Add support for Oracle
INTERVALdata types (INTERVAL YEAR TO MONTH,INTERVAL DAY TO SECOND) in the JDBC Oracle connector, and add E2E test coverage for bothBFILEandINTERVALcolumns.This is a re-work of the previously closed PR #9389, rebased on the latest
dev:BFILEtype converter support was already merged intodev(via [Fix][Connector-V2] Fix OceanBase Oracle create unsupported data type #9383), so only theINTERVALmapping is added here.INTERVAL DAY(2) TO SECOND(6)), so the type name is normalized before matching.Does this PR introduce any user-facing change?
Yes. Oracle
INTERVALcolumns are now read asSTRINGinstead of failing withconvertToSeaTunnelTypeError. The Oracle source connector data type mapping docs (en/zh) are updated accordingly.How was this patch tested?
JdbcOracleITE2E test:BFILE_COLandINTERVAL_COLadded to source/sink tables, test data and the source-to-sink job config (jdbc_oracle_source_to_sink.conf).BFILEcolumns are inserted asNULL(a BFILE locator cannot be set viasetObject),INTERVALvalues are written viajava.time.Duration.spotless:applyclean,./mvnw -DskipTests install -pl seatunnel-connectors-v2/connector-jdbc,seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-1 -ambuilds successfully (the E2E test itself requires Docker, will run in CI).Check list
New License Guide
incompatible-changes.mdto describe the incompatibility caused by this PR.