morningman opened a new pull request, #66403:
URL: https://github.com/apache/doris/pull/66403
### 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 symbols`
`be/src/service/CMakeLists.txt` — one line, plus why.
`doris_be` sets `ENABLE_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, an
`std::bad_alloc` that 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.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 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 -r` shows
**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.
⚠️ This changes BE's link behaviour, so it wants a full relink and a BE
regression run — tablet metadata itself lives in RocksDB.
#### 2. `[fix](be) Pick the table reader per scan range, not per scan node`
`be/src/exec/scan/file_scanner_v2.{h,cpp}` + unit test.
`_open_impl` builds one `_table_reader` from the **first** scan range;
`_prepare_next_split` then 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 different `table_format_type`s 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_ctxes` is 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 produces`
`fe/fe-connector/fe-connector-paimon` + unit tests.
`Connector.ownsHandle` defaults to `false`. 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 throws
`ClassCastException`.
One method, same implementation as the two siblings that already have it.
#### 4. `[feat](paimon) Say which bucket a scan range came from`
`fe/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: `populateRangeParams` does not forward it, so **BE is unaffected**.
Set on every `DataSplit`-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 collapsed `COUNT(*)` range (it stands for
splits from several buckets, so any single number would be a lie) nor on a
non-`DataSplit` system 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
read`
`fe/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.preserveExtraStorageKeySlots` keeps the key slots and
ships them as `extra_key_column_slot_ids`, because BE merges by key regardless
of what was selected. The new branch sits beside that one, before the same
`removeIf`, 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:
- SPI `ConnectorScanPlanProvider.getMustReadColumns(session, handle)` —
**defaults to an empty set**, so every existing connector prunes exactly as
before
- `PluginDrivenScanNode.mustReadColumnsFromConnector()` — same memoized
provider the rest of planning uses, with the plugin classloader pinned
- `PhysicalPlanTranslator.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
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
Unit tests, all 0 skipped: `fe-connector-api` 113, `fe-connector-paimon`
511 (1 pre-existing skip), `fe-core` neighbourhood 153
(`PluginDrivenScanNode*`, `PhysicalPlanTranslator*`,
`CountStarSmallestSlotTest`, which starts a real FE and exercises the OLAP
pruning path this change sits next to). BE: `FileScannerV2*:FileScannerTest*`
26 and `Paimon*:*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 #3, #4 and #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::ColumnFamilyDescriptor` on 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 with `nm -D` and `readelf -r` as described above.
- Behavior changed:
- [x] No. <!-- #1 removes symbols that were never meant to be part of
BE's interface; #5's SPI method defaults to empty; #4 is FE-only and not
forwarded to BE. #2 changes behaviour only for a scan node that mixes table
formats, which today has no correct outcome. -->
- Does this need documentation?
- [x] No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
