Repository navigation
[refactor](jdbc) Unify JDBC scanning into FileQueryScanNode/JniReader framework - #61141
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 27758 ms |
TPC-DS: Total hot run time: 153274 ms |
morningman
force-pushed
the
wt-new-jni-refactor
branch
from
March 9, 2026 17:13
9409202 to
a246700
Compare
morningman
force-pushed
the
wt-new-jni-refactor
branch
from
March 9, 2026 17:14
a246700 to
8c2975a
Compare
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 27653 ms |
TPC-DS: Total hot run time: 153313 ms |
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 27581 ms |
TPC-DS: Total hot run time: 153276 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review: JDBC Data Source Refactoring
Reviewed this PR that refactors JDBC scanning/writing to use the unified FileScanner/JniReader framework. The architecture is well-designed — splitting JniConnector into JniReader (lifecycle) + JniDataBridge (stateless data exchange), and introducing the JdbcTypeHandler strategy pattern are good abstractions.
However, I found several issues, including one critical data loss bug. See inline comments for details.
Critical Checkpoints (per code-review skill)
| Checkpoint | Conclusion |
|---|---|
| Goal accomplished? | Partially. The scan path refactoring looks correct, but the write path has a critical transaction commit bug that causes silent data loss. |
| Modification focused? | Mostly yes, though 74 files / 5000+ lines is a large changeset. The TypeHandler strategy pattern extraction adds complexity but is justified. |
| Concurrency | No new concurrency issues introduced. JniReader lifecycle is single-threaded per scanner instance. |
| Lifecycle management | _file_slot_descs stored as reference is fragile but currently safe (documented in comment). |
| Incompatible changes / rolling upgrade | JdbcScanNode now extends FileQueryScanNode. Thrift field jdbc_params added as optional (field 11) in TTableFormatFileDesc — backward compatible. Old FE → new BE path should work since old path still exists. |
| Parallel code paths | The internal_service.cpp test_jdbc_connection path was updated consistently. |
| Test coverage | PR claims "Previous test can cover this change" with no new tests for a 5000+ line refactor. This is concerning — the transaction commit bug would have been caught by any transactional write test. |
| Observability | No new metrics or logging added for the new JNI-based JDBC path. The old path had some timing counters. |
| Data writes | CRITICAL: Transactional writes are broken — see inline comment #1. |
| FE-BE variable passing | jdbc_driver_checksum is missing from the scanner params path — see inline comment #5. |
morningman
force-pushed
the
wt-new-jni-refactor
branch
from
March 13, 2026 06:37
b8d722f to
836de99
Compare
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 26867 ms |
TPC-DS: Total hot run time: 168660 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
zhangstar333
approved these changes
Mar 16, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
CalvinKirs
approved these changes
Mar 16, 2026
morningman
added a commit
to morningman/doris
that referenced
this pull request
Mar 25, 2026
… framework (apache#61141) Refactor the JDBC data source scanning architecture to integrate with the unified FileScanner/JniReader framework, replacing the standalone ExternalScanNode-based JDBC scan path. The JDBC scan path was independently implemented with its own operator (JDBCScanLocalState), scanner (JdbcScanner), and JNI connector (JniConnector → BaseJdbcExecutor hierarchy), while other JNI-based data sources (Paimon, Hudi, MaxCompute, TrinoConnector) already use the unified FileScanner → JniReader → JniScanner path. This caused code duplication, maintenance burden, and architectural inconsistency. 1. BE: Split JniConnector into two focused classes: - JniReader: JNI lifecycle management (open/read/close), base class for all JNI readers (PaimonJniReader, HudiJniReader, JdbcJniReader, etc.) - JniDataBridge: Stateless data exchange between C++ Blocks and Java shared memory via JNI 2. Java: Introduce Strategy Pattern for database-specific type handling: - JdbcTypeHandler interface with DefaultTypeHandler base implementation - Per-database handlers: MySQLTypeHandler, OracleTypeHandler, PostgreSQLTypeHandler, ClickHouseTypeHandler, SQLServerTypeHandler, DB2TypeHandler, SapHanaTypeHandler, TrinoTypeHandler, GbaseTypeHandler - JdbcTypeHandlerFactory for handler selection - JdbcJniScanner (extends JniScanner) for read path - JdbcJniWriter (extends JniWriter) for write path - Old BaseJdbcExecutor subclasses marked @deprecated but preserved 3. FE: Migrate JdbcScanNode from ExternalScanNode to FileQueryScanNode: - JdbcScanNode now extends FileQueryScanNode - Introduces JdbcSplit (extends FileSplit) to carry JDBC params - Uses TFileScanRange with FORMAT_JNI and table_format_type="jdbc" - Adds jdbc_params field to TTableFormatFileDesc in Thrift - Adapts PhysicalPlanTranslator.visitPhysicalJdbcScan() accordingly 4. BE: Add JdbcJniReader and integrate into FileScanner: - JdbcJniReader handles special types (bitmap, HLL, quantile_state, JSONB) via string-based intermediary and CAST - FileScanner._get_next_reader() adds "jdbc" table_format_type branch - JdbcUtils utility for JDBC driver URL resolution - Existing JdbcScanner preserved as transitional (deprecated) All predicate push-down logic (createJdbcFilters, getJdbcQueryStr, etc.) is preserved from the original JdbcScanNode implementation.
morningman
added a commit
to morningman/doris
that referenced
this pull request
Mar 25, 2026
… framework (apache#61141) Refactor the JDBC data source scanning architecture to integrate with the unified FileScanner/JniReader framework, replacing the standalone ExternalScanNode-based JDBC scan path. The JDBC scan path was independently implemented with its own operator (JDBCScanLocalState), scanner (JdbcScanner), and JNI connector (JniConnector → BaseJdbcExecutor hierarchy), while other JNI-based data sources (Paimon, Hudi, MaxCompute, TrinoConnector) already use the unified FileScanner → JniReader → JniScanner path. This caused code duplication, maintenance burden, and architectural inconsistency. 1. BE: Split JniConnector into two focused classes: - JniReader: JNI lifecycle management (open/read/close), base class for all JNI readers (PaimonJniReader, HudiJniReader, JdbcJniReader, etc.) - JniDataBridge: Stateless data exchange between C++ Blocks and Java shared memory via JNI 2. Java: Introduce Strategy Pattern for database-specific type handling: - JdbcTypeHandler interface with DefaultTypeHandler base implementation - Per-database handlers: MySQLTypeHandler, OracleTypeHandler, PostgreSQLTypeHandler, ClickHouseTypeHandler, SQLServerTypeHandler, DB2TypeHandler, SapHanaTypeHandler, TrinoTypeHandler, GbaseTypeHandler - JdbcTypeHandlerFactory for handler selection - JdbcJniScanner (extends JniScanner) for read path - JdbcJniWriter (extends JniWriter) for write path - Old BaseJdbcExecutor subclasses marked @deprecated but preserved 3. FE: Migrate JdbcScanNode from ExternalScanNode to FileQueryScanNode: - JdbcScanNode now extends FileQueryScanNode - Introduces JdbcSplit (extends FileSplit) to carry JDBC params - Uses TFileScanRange with FORMAT_JNI and table_format_type="jdbc" - Adds jdbc_params field to TTableFormatFileDesc in Thrift - Adapts PhysicalPlanTranslator.visitPhysicalJdbcScan() accordingly 4. BE: Add JdbcJniReader and integrate into FileScanner: - JdbcJniReader handles special types (bitmap, HLL, quantile_state, JSONB) via string-based intermediary and CAST - FileScanner._get_next_reader() adds "jdbc" table_format_type branch - JdbcUtils utility for JDBC driver URL resolution - Existing JdbcScanner preserved as transitional (deprecated) All predicate push-down logic (createJdbcFilters, getJdbcQueryStr, etc.) is preserved from the original JdbcScanNode implementation.
4 of 12 tasks
morningman
added a commit
to morningman/doris
that referenced
this pull request
Oct 31, 2026
… in the suite comment ### What problem does this PR solve? Issue Number: None Related PR: apache#61141, apache#69601 Problem Summary: The header comment of test_tvf_avro_format_rejected said that apache#69601 removed the BE Avro reader. On master the reader was removed by apache#61141; apache#69601 made FE reject format=avro during analysis and added a BE-side guard. Correct the PR number in the comment. Comment only. ### Release note None ### Check List (For Author) - Test: No need to test (comment-only change); the suite still passes the offline Groovy parse check - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
morningman
added a commit
that referenced
this pull request
Oct 31, 2026
…69849) ### What problem does this PR solve? Issue Number: None Related PR: #69601, #64699 Problem Summary: **In short.** #69601 made FE reject `format=avro` while it parses file format properties. This PR adds end-to-end coverage for that: a regression suite that runs `s3()` and `hdfs()` with `format=avro` and checks that they fail during analysis, and a unit test for the two table-function constructors. Test only, no code change. **Background.** Master no longer reads standalone Avro files: #61141 removed the BE reader, #65733 the scanner module, and #69601 made `FileFormatProperties.createFileFormatProperties` throw `Avro file reads are no longer supported`. Before #69601, FE accepted the format and the query failed later in BE with `Not supported file format in fetch table schema: 12`. branch-4.2 still reads Avro through the JNI scanner (#64699 fixed a crash in it), so users who upgrade from 4.x with Avro table functions or loads get this error. **What was missing.** #69601's unit test calls `createFileFormatProperties` directly. Nothing checks that a table function reaches that check before it validates storage properties or lists files. If a constructor validated the location first, `format=avro` against an unreachable location would report a storage error instead, and no test would notice. **What this PR adds.** - `external_table_p0/tvf/test_tvf_avro_format_rejected`: `s3()` and `hdfs()` with `format` set to `avro` and to `AVRO`, against a made-up bucket, made-up credentials and an HDFS address nothing listens on. Each call must fail with `Avro file reads are no longer supported`. - `UnsupportedExternalReadsTest#testAvroTableFunctionsAreRejectedBeforeStorageAccess`: constructs `S3TableValuedFunction` and `HdfsTableValuedFunction` with `format=avro` / `AVRO` and checks the same message. **Results.** On master `2a4191c1dc9` with this change, all four statements of the suite fail during analysis with `Can not build s3(): Avro file reads are no longer supported` / `Can not build hdfs(): ...`, and the unit test passes. Before #69601 the same `s3()` call would have gone on to validate and list the made-up location, so the suite pins the order as well as the message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Refactor the JDBC data source scanning architecture to integrate with the
unified FileScanner/JniReader framework, replacing the standalone
ExternalScanNode-based JDBC scan path.
Motivation
The JDBC scan path was independently implemented with its own operator
(JDBCScanLocalState), scanner (JdbcScanner), and JNI connector (JniConnector
→ BaseJdbcExecutor hierarchy), while other JNI-based data sources (Paimon,
Hudi, MaxCompute, TrinoConnector) already use the unified FileScanner →
JniReader → JniScanner path. This caused code duplication, maintenance burden,
and architectural inconsistency.
Changes
BE: Split JniConnector into two focused classes:
all JNI readers (PaimonJniReader, HudiJniReader, JdbcJniReader, etc.)
shared memory via JNI
Java: Introduce Strategy Pattern for database-specific type handling:
PostgreSQLTypeHandler, ClickHouseTypeHandler, SQLServerTypeHandler,
DB2TypeHandler, SapHanaTypeHandler, TrinoTypeHandler, GbaseTypeHandler
FE: Migrate JdbcScanNode from ExternalScanNode to FileQueryScanNode:
BE: Add JdbcJniReader and integrate into FileScanner:
via string-based intermediary and CAST
All predicate push-down logic (createJdbcFilters, getJdbcQueryStr, etc.)
is preserved from the original JdbcScanNode implementation.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)