[fix](paimon connector) Five independent fixes a sibling-connector read depends on - #66403
Merged
Merged
Conversation
Exporting them makes this executable the definition every later-loaded library binds to, so a JNI library carrying its own RocksDB runs half on ours. The fluss scanner bundles frocksdbjni, whose librocksdbjni.so defines 2576 rocksdb symbols under names identical to ours but was built against the pre-C++11 libstdc++ string ABI: objects laid out by one copy and used by the other yield a garbage length, an std::bad_alloc that escapes the JNI frame, and an aborted BE. Reading any fluss primary-key table with a kv snapshot killed the process, reproducibly. Scoped to the archive rather than dropping ENABLE_EXPORTS, because what needs the exports is native UDFs (runtime/user_function_cache.cpp dlopens them) and those use the Doris UDF ABI, which has nothing to do with RocksDB. Crash stacks do not need it either -- they are symbolized from debug info, which is why they name even anonymous-namespace functions. 61 rocksdb symbols remain exported: inline and template members the compiler emitted into Doris's own objects, which no archive exclusion can reach. 29 of those still share a name with the JNI library, but none appear in its relocation table -- it never resolves them at load time, so they cannot be interposed. The library also duplicates zstd, lz4, snappy, bzip2 and zlib symbols; those are C ABIs, stable and layout-free, and are left alone. Verified: the fluss primary-key suite passes with BE alive (it aborted before); all three fluss suites green; an internal table survives write, BE restart and read, which is the tablet metadata RocksDB itself round-tripping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016VCPjzhwMQuP7nTgdvGVbM
Connector.ownsHandle defaults to false, and this connector never overrode it. That was invisible while paimon was only ever a front-door catalog: the predicate exists so a GATEWAY connector can embed another as a sibling and route a foreign handle back to whoever made it, since the sibling's concrete handle type cannot be named across the plugin classloader split. The fluss connector reads a lake table by delegating to this one, so it asks that question about every handle it gets back — and got "not mine" about handles paimon had just produced. Every guard on the gateway side then falls through, and the first cast throws a ClassCastException naming the GATEWAY's handle type and two class loaders, with nothing to suggest the missing piece is a method here. Same one-liner the iceberg and hudi siblings behind the hms gateway already carry. No unit test could have caught this: a hand-written test double implements ownsHandle precisely because it has to, so the double is more capable than the real connector. It took an end-to-end read to surface. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016VCPjzhwMQuP7nTgdvGVbM
FileScannerV2 built its table reader once, from the first range, and reused it for every range after that. One scan node can be given ranges of more than one table format: a fluss union read plans the table's lake half through the paimon connector and its log half itself, and both arrive as ranges of the same scan. Whichever range came first then decided the reader for all of them, and the other format's ranges were handed to a reader that does not understand them. That does not fail cleanly — it fails as whatever that reader makes of a foreign range. Here it was paimon's, reporting an unsupported file format for a fluss range that carries no paimon parameters at all. Which ranges share a scanner is up to the engine's assignment, so the same query succeeded or failed by how the ranges happened to be dealt out, and changing the projected columns could flip it either way. The reader now follows the range's table format. The expression contexts are deliberately not rebuilt: they are per-scanner and format-independent, and _init_expr_ctxes is not idempotent. Verified by disabling the rebuild and rerunning the suites: only the union read fails, with exactly the original error, and the four fluss suites that do not mix formats stay green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016VCPjzhwMQuP7nTgdvGVbM
A scan range this connector plans is opaque about its origin: the JNI arm carries a serialized split and nothing else, the native arm a file path and a byte interval. That is fine while the only reader is BE, which just reads what it is handed. It stops being fine once another connector plans splits here on behalf of its own table. The fluss connector does exactly that: a fluss table tiered into paimon keeps a bucket-identical layout, and reading it means pairing the lake data of bucket b with the log tail of bucket b that has not been tiered yet. Nothing on the range says b. Parsing it out of the data-file path would work only on the native arm and only by depending on this connector's directory layout. So carry it: paimon.bucket = DataSplit.bucket(), on the native and JNI arms alike -- which BE reader a split lands on is a session-level escape hatch the sibling does not control, and it must not change what the sibling can learn. FE-only; populateRangeParams does not forward it, so BE sees nothing new. Two ranges deliberately do NOT carry it. The collapsed COUNT(*) range stands for the splits of every bucket, so any single number on it would be a lie. A non-DataSplit system split has no bucket at all. A consumer that needs the binding must fail loud on an absent bucket rather than read it as "no state for this bucket" -- that reading turns a broken contract into duplicated rows. The fixture is two-bucket on purpose: with one bucket every range reads "0" and a hard-coded constant passes. Four mutations checked red -- constant bucket on the native arm, no bucket on the JNI arm, a bucket on the system split, a bucket on the count range. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016VCPjzhwMQuP7nTgdvGVbM
A connector whose BE-side reader merges or suppresses rows by key needs that key read whether or not the query selected it. Doris already keeps those columns for its own aggregate and merge-on-read unique-key tables -- preserveExtraStorageKeySlots, four lines above where the scan's slots are pruned -- for exactly that reason. A plugin connector had no way to say the same thing, and BE cannot read a column the plan never asked for. So ask it: getMustReadColumns, answered per scan, empty by default, so nothing changes for a connector that needs only what the query projects. The answer arrives during plan translation, after the scan node is initialized and before splits are planned, and widens the scan's tuple only -- the project above it already has its own output tuple, so the column is read and then dropped rather than returned. The question goes through the same memoized provider that will plan the splits, because the two have to come from one decision: a connector that answers "no extra columns" here and then plans a read that needs them leaves BE looking for a column that is not in the projection. A name that matches no slot fails the query and says which name, rather than being skipped -- skipping turns a disagreement about the table into silently wrong rows. Checked red by six mutations: dropping the branch, skipping unknown names, stopping after the first match, dropping the null answer guard, resolving a fresh provider to ask, and a non-empty SPI default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016VCPjzhwMQuP7nTgdvGVbM
morningman
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
August 4, 2026 00:54
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
4 tasks
Contributor
Author
|
run buildall |
924060929
approved these changes
Aug 4, 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. |
Contributor
TPC-H: Total hot run time: 28461 ms |
Contributor
TPC-DS: Total hot run time: 169721 ms |
Contributor
ClickBench: Total hot run time: 23.85 s |
morrySnow
changed the title
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
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
3 participants
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?
Issue Number: close #xxx
Related PR: #66399
Problem Summary:
Five independent fixes, none of them in one connector's own code. They were found while building the fluss catalog (#66399), which is where each one's symptom first showed up — but every one of them is a Doris bug or a Doris gap that exists without fluss, so they are proposed on their own, ahead of and separately from that connector. #66399 will be rebased on top of this and shrink by exactly these five commits.
They are unrelated to each other; there is one commit per fix and each can be reviewed alone.
1.
[fix](be) Stop exporting the statically linked RocksDB symbolsbe/src/service/CMakeLists.txt— one line, plus why.doris_besetsENABLE_EXPORTS, so the 4840 rocksdb symbols it links statically are exported into the global dynamic symbol table. The executable is the highest-priority definition for everything loaded after it, so any JNI library that carries its own RocksDB has its internal calls resolved into doris_be's copy instead — 2576 symbols with byte-identical mangled names.That would be survivable if the two agreed on layout. They do not: such libraries are commonly built against the pre-C++11 libstdc++ string ABI (
...C1ERKSs) while doris_be is built against the new one (...RKNSt7__cxx1112basic_stringE). An object constructed with one layout and used by functions compiled for the other yields a garbage length, anstd::bad_allocthat escapes through the JNI frame, and an aborted BE process.The fix hides that one archive from the dynamic symbol table, so such a library binds to its own copy. It is scoped to the archive rather than dropping
ENABLE_EXPORTS, because what actually needs the exports is native UDFs (runtime/user_function_cache.cppdlopens them) and those use the Doris UDF ABI, which has nothing to do with RocksDB. Crash stacks do not need it either — they are symbolized from debug info, which is why they can name even anonymous-namespace functions.Verified by symbol table rather than by argument: after the change 61 rocksdb symbols remain exported (compiler-instantiated inline/template members that landed in Doris's own objects, which an archive-level exclusion cannot reach). 29 of those share a name with a JNI library's, but
readelf -rshows none of them in that library's relocation table, so it never looks them up. The zstd/lz4/snappy/bzip2/zlib duplicates are left alone deliberately: those are C ABIs, stable and layout-free, unlike RocksDB's C++ objects.2.
[fix](be) Pick the table reader per scan range, not per scan nodebe/src/exec/scan/file_scanner_v2.{h,cpp}+ unit test._open_implbuilds one_table_readerfrom the first scan range;_prepare_next_splitthen reuses it for every range that follows, and never revisits the choice. The reader is format-specific, so a scan node holding ranges of two differenttable_format_types hands the second kind to the first kind's reader.That does not fail cleanly. It fails as whatever the wrong reader makes of a foreign range — e.g. paimon's reader reporting an unsupported file format for a range that carries no paimon parameters at all. And which ranges end up in the same scanner is the engine's assignment, so the same query succeeds or fails depending on how the ranges happened to be dealt out, and changing the projection can change the outcome.
The fix records the format the reader was built for and rebuilds when a range disagrees. The expression contexts are deliberately not rebuilt: they are per-scanner and format-independent, and
_init_expr_ctxesis not idempotent.A scan node mixing formats is what a connector reading a table as "a lake plus the log written after it" produces — its lake half planned by a sibling connector, its own half by itself — but nothing in the scanner assumes that, and the fix is a general one.
New unit test
TheTableReaderIsRebuiltWhenARangeChangesTableFormat: same format reuses the reader, a different format replaces it, and the formats really do map to different reader types (otherwise the first two assertions would hold for a scanner that never rebuilt anything). Reverting the comparison to the pre-fix behaviour turns it red.3.
[fix](paimon) Claim the table handles this connector producesfe/fe-connector/fe-connector-paimon+ unit tests.Connector.ownsHandledefaults tofalse. The iceberg and hudi connectors override it — they are already used as siblings behind the hms gateway — but paimon never did. Any gateway connector that embeds paimon therefore asks "is this handle yours?" about a handle paimon itself produced and is told no, so every one of the gateway's type guards fails open and the first cast throwsClassCastException.One method, same implementation as the two siblings that already have it.
4.
[feat](paimon) Say which bucket a scan range came fromfe/fe-connector/fe-connector-paimon+ unit tests.Adds
paimon.bucket=DataSplit.bucket()to the scan range properties, so a connector that plans paimon splits on behalf of its own table can line them up with its own per-bucket state.FE-only:
populateRangeParamsdoes not forward it, so BE is unaffected. Set on everyDataSplit-backed range, native and JNI alike, so which reader BE ends up using cannot change what a caller can learn about the split. Deliberately not set on the collapsedCOUNT(*)range (it stands for splits from several buckets, so any single number would be a lie) nor on a non-DataSplitsystem split (there is no bucket). Consumers are expected to fail loud when it is absent on a range they meant to bind, since treating that as "no state for this bucket" is a wrong-results bug rather than a degradation.5.
[feat](connector) Let a connector name the columns its reader must readfe/fe-connector/fe-connector-api+fe/fe-core+ unit tests. The only engine-side change here.A connector whose BE-side reader merges, suppresses or otherwise identifies rows by key needs those key columns to be READ, whether or not the query selected them. Today the plugin scan's tuple is pruned to the projection, so the reader is handed a scan without the column it needs.
This is not a new mechanism. Doris does exactly this for its own aggregate and merge-on-read unique-key tables:
PhysicalPlanTranslator.preserveExtraStorageKeySlotskeeps the key slots and ships them asextra_key_column_slot_ids, because BE merges by key regardless of what was selected. The new branch sits beside that one, before the sameremoveIf, and only widens the scan's tuple — the project above it was already given its own output tuple, so a preserved column is read and then dropped and never reaches the query's output.Three names:
ConnectorScanPlanProvider.getMustReadColumns(session, handle)— defaults to an empty set, so every existing connector prunes exactly as beforePluginDrivenScanNode.mustReadColumnsFromConnector()— same memoized provider the rest of planning uses, with the plugin classloader pinnedPhysicalPlanTranslator.preserveConnectorMustReadSlots()A returned name that matches no slot fails the query loud rather than being skipped: it means the connector and the engine disagree about the table, and reading on would hand the connector's reader a scan missing a column it said it needs — silently wrong rows, not an error.
Release note
None
Check List (For Author)
Test
Unit tests, all 0 skipped:
fe-connector-api113,fe-connector-paimon511 (1 pre-existing skip),fe-coreneighbourhood 153 (PluginDrivenScanNode*,PhysicalPlanTranslator*,CountStarSmallestSlotTest, which starts a real FE and exercises the OLAP pruning path this change sits next to). BE:FileScannerV2*:FileScannerTest*26 andPaimon*:*Iceberg*:*EqualityDelete*227.Every new behaviour was mutation-tested — the change inverted, rebuilt, and the test required to go red. For 注释英文拼写错误 #2 that is the reader-rebuild comparison; for Support bulk loading from S3 compatible distributed storage #3, 居然是Java还是ant #4 and 修改为maven就好了 #5 the mutations are listed in the individual commit messages.
能公开一些公开数据集上的性能测试数据吗? #1 cannot be covered by a unit test — it is a link-time property. It was verified against a real BE: the failure reproduced twice before the change (BE abort with
Java_org_rocksdb_RocksDB_openROnly→ColumnFamilyDescriptor::ColumnFamilyDescriptoron the stack), the same Java code passed in a plain JVM, and after a full relink the same workload runs and BE's own tablet metadata survives a restart. Residual exported symbols were checked withnm -Dandreadelf -ras described above.Behavior changed:
Does this need documentation?