refactor(reader): Move the reader factory out of fb/ (#1069) - #1069
Open
HuamengJiang wants to merge 1 commit into
Open
refactor(reader): Move the reader factory out of fb/ (#1069)#1069HuamengJiang wants to merge 1 commit into
HuamengJiang wants to merge 1 commit into
Conversation
meta-cla
Bot
added
the
CLA Signed
|
@HuamengJiang has exported this pull request. If you are a Meta employee, you can view the originating Diff in D114807828. |
meta-codesync
Bot
changed the title
HuamengJiang
force-pushed
the
export-D114807828
branch
from
August 6, 2026 19:16
70f6a10 to
a7c7f3c
Compare
HuamengJiang
added a commit
to HuamengJiang/nimble-1
that referenced
this pull request
Aug 6, 2026
…or#1069) Summary: Gets the NIMBLE reader's registration surface out of `fb/`, so that internal and open-source builds register the same factory instead of each having their own entry point. Before this, `NimbleReaderFactory` -- and with it `registerNimbleReaderFactory()`, which is what Prism, Gluten, Axel, Axiom, and a long tail of tools call -- lived in `dwio/nimble/velox/reader/fb/NimbleReader.h`. Nothing about the factory is internal: it holds a `SelectiveNimbleReaderFactory`, reads one flag, and picks a reader. It was stuck in `fb/` by a single edge, namely that it constructs the batch reader, which does depend on internal-only code (`//dwio/api:velox-util`). So the edge is inverted rather than the factory being kept hostage to it: - `dwio/nimble/velox/reader/NimbleReaderFactory.{h,cpp}` is new, OSS-clean, and holds the factory, the dispatch, and the register/unregister pair. - The batch branch now goes through `detail::createBatchReader()`, declared in that header and defined once per build flavour: `fb/NimbleReader.cpp` for Buck, `BatchReaderOSS.cpp` for CMake. This is the same ODR-based split already used for `detail::defaultMetadata()` (`VeloxWriterDefaultMetadataOSS.cpp`) and `detail::initHook()` (`SelectiveNimbleReaderInitHookOSS.cpp`). - `fb/NimbleReader.h` is deleted. With the factory gone it declared nothing, since the batch `NimbleReader` and `NimbleRowReader` live in an anonymous namespace in the `.cpp`. - `fb/` keeps what is genuinely internal: the batch reader, the Iceberg field-id projection helpers, and `FieldIdResolver`. In the open-source build the hook returns nullptr and the factory raises a clear error naming the session property, rather than failing to link. That path is unreachable in practice: `FileConfig::kSelectiveNimbleReaderEnabledSession` and `QueryConfig::kSelectiveNimbleReaderEnabled` both default to true, so every engine-driven read already takes the selective branch. The 28 `#include` sites move to the new header. They have to change in this commit rather than a follow-up, because deleting the old header breaks them immediately. On the CMake side, `nimble_velox_reader_factory` is a new library and `add_subdirectory(reader)` is ordered after `selective`, which it links. Behavior is unchanged on both branches; this is placement and linkage only. Note for reviewers: `mrs/ranklake/table_service/src/storage/table:nimble_index_tablet` is one of the 28 consumers and does not build. That is pre-existing on trunk -- it constructs `ReaderOptions{pool, &dataIoStats, &metadataIoStats}` against a class that only has a one-argument constructor -- and I verified it fails identically with this change shelved. Its include is updated for consistency, but the target is left otherwise untouched. Reviewed By: xiaoxmeng Differential Revision: D114807828
HuamengJiang
force-pushed
the
export-D114807828
branch
from
August 6, 2026 23:51
a7c7f3c to
fc35dd8
Compare
HuamengJiang
added a commit
to HuamengJiang/nimble-1
that referenced
this pull request
Aug 6, 2026
…or#1069) Summary: Gets the NIMBLE reader's registration surface out of `fb/`, so that internal and open-source builds register the same factory instead of each having their own entry point. Before this, `NimbleReaderFactory` -- and with it `registerNimbleReaderFactory()`, which is what Prism, Gluten, Axel, Axiom, and a long tail of tools call -- lived in `dwio/nimble/velox/reader/fb/NimbleReader.h`. Nothing about the factory is internal: it holds a `SelectiveNimbleReaderFactory`, reads one flag, and picks a reader. It was stuck in `fb/` by a single edge, namely that it constructs the batch reader, which does depend on internal-only code (`//dwio/api:velox-util`). So the edge is inverted rather than the factory being kept hostage to it: - `dwio/nimble/velox/reader/NimbleReaderFactory.{h,cpp}` is new, OSS-clean, and holds the factory, the dispatch, and the register/unregister pair. - The batch branch now goes through `detail::createBatchReader()`, declared in that header and defined once per build flavour: `fb/NimbleReader.cpp` for Buck, `BatchReaderOSS.cpp` for CMake. This is the same ODR-based split already used for `detail::defaultMetadata()` (`VeloxWriterDefaultMetadataOSS.cpp`) and `detail::initHook()` (`SelectiveNimbleReaderInitHookOSS.cpp`). - `fb/NimbleReader.h` is deleted. With the factory gone it declared nothing, since the batch `NimbleReader` and `NimbleRowReader` live in an anonymous namespace in the `.cpp`. - `fb/` keeps what is genuinely internal: the batch reader, the Iceberg field-id projection helpers, and `FieldIdResolver`. In the open-source build the hook returns nullptr and the factory raises a clear error naming the session property, rather than failing to link. That path is unreachable in practice: `FileConfig::kSelectiveNimbleReaderEnabledSession` and `QueryConfig::kSelectiveNimbleReaderEnabled` both default to true, so every engine-driven read already takes the selective branch. The 28 `#include` sites move to the new header. They have to change in this commit rather than a follow-up, because deleting the old header breaks them immediately. On the CMake side, `nimble_velox_reader_factory` is a new library and `add_subdirectory(reader)` is ordered after `selective`, which it links. Behavior is unchanged on both branches; this is placement and linkage only. Note for reviewers: `mrs/ranklake/table_service/src/storage/table:nimble_index_tablet` is one of the 28 consumers and does not build. That is pre-existing on trunk -- it constructs `ReaderOptions{pool, &dataIoStats, &metadataIoStats}` against a class that only has a one-argument constructor -- and I verified it fails identically with this change shelved. Its include is updated for consistency, but the target is left otherwise untouched. Reviewed By: xiaoxmeng Differential Revision: D114807828
HuamengJiang
added a commit
to HuamengJiang/nimble-1
that referenced
this pull request
Aug 7, 2026
…or#1069) Summary: Gets the NIMBLE reader's registration surface out of `fb/`, so that internal and open-source builds register the same factory instead of each having their own entry point. Before this, `NimbleReaderFactory` -- and with it `registerNimbleReaderFactory()`, which is what Prism, Gluten, Axel, Axiom, and a long tail of tools call -- lived in `dwio/nimble/velox/reader/fb/NimbleReader.h`. Nothing about the factory is internal: it holds a `SelectiveNimbleReaderFactory`, reads one flag, and picks a reader. It was stuck in `fb/` by a single edge, namely that it constructs the batch reader, which does depend on internal-only code (`//dwio/api:velox-util`). So the edge is inverted rather than the factory being kept hostage to it: - `dwio/nimble/velox/reader/NimbleReaderFactory.{h,cpp}` is new, OSS-clean, and holds the factory, the dispatch, and the register/unregister pair. - The batch branch now goes through `detail::createBatchReader()`, declared in that header and defined once per build flavour: `fb/NimbleReader.cpp` for Buck, `BatchReaderOSS.cpp` for CMake. This is the same ODR-based split already used for `detail::defaultMetadata()` (`VeloxWriterDefaultMetadataOSS.cpp`) and `detail::initHook()` (`SelectiveNimbleReaderInitHookOSS.cpp`). - `fb/NimbleReader.h` is deleted. With the factory gone it declared nothing, since the batch `NimbleReader` and `NimbleRowReader` live in an anonymous namespace in the `.cpp`. - `fb/` keeps what is genuinely internal: the batch reader, the Iceberg field-id projection helpers, and `FieldIdResolver`. In the open-source build the hook returns nullptr and the factory raises a clear error naming the session property, rather than failing to link. That path is unreachable in practice: `FileConfig::kSelectiveNimbleReaderEnabledSession` and `QueryConfig::kSelectiveNimbleReaderEnabled` both default to true, so every engine-driven read already takes the selective branch. The 28 `#include` sites move to the new header. They have to change in this commit rather than a follow-up, because deleting the old header breaks them immediately. On the CMake side, `nimble_velox_reader_factory` is a new library and `add_subdirectory(reader)` is ordered after `selective`, which it links. Behavior is unchanged on both branches; this is placement and linkage only. Note for reviewers: `mrs/ranklake/table_service/src/storage/table:nimble_index_tablet` is one of the 28 consumers and does not build. That is pre-existing on trunk -- it constructs `ReaderOptions{pool, &dataIoStats, &metadataIoStats}` against a class that only has a one-argument constructor -- and I verified it fails identically with this change shelved. Its include is updated for consistency, but the target is left otherwise untouched. Reviewed By: xiaoxmeng Differential Revision: D114807828
HuamengJiang
force-pushed
the
export-D114807828
branch
from
August 7, 2026 02:32
fc35dd8 to
e2dbca9
Compare
…or#1069) Summary: Gets the NIMBLE reader's registration surface out of `fb/`, so that internal and open-source builds register the same factory instead of each having their own entry point. Before this, `NimbleReaderFactory` -- and with it `registerNimbleReaderFactory()`, which is what Prism, Gluten, Axel, Axiom, and a long tail of tools call -- lived in `dwio/nimble/velox/reader/fb/NimbleReader.h`. Nothing about the factory is internal: it holds a `SelectiveNimbleReaderFactory`, reads one flag, and picks a reader. It was stuck in `fb/` by a single edge, namely that it constructs the batch reader, which does depend on internal-only code (`//dwio/api:velox-util`). So the edge is inverted rather than the factory being kept hostage to it: - `dwio/nimble/velox/reader/NimbleReaderFactory.{h,cpp}` is new, OSS-clean, and holds the factory, the dispatch, and the register/unregister pair. - The batch branch now goes through `detail::createBatchReader()`, declared in that header and defined once per build flavour: `fb/NimbleReader.cpp` for Buck, `BatchReaderOSS.cpp` for CMake. This is the same ODR-based split already used for `detail::defaultMetadata()` (`VeloxWriterDefaultMetadataOSS.cpp`) and `detail::initHook()` (`SelectiveNimbleReaderInitHookOSS.cpp`). - `fb/NimbleReader.h` is deleted. With the factory gone it declared nothing, since the batch `NimbleReader` and `NimbleRowReader` live in an anonymous namespace in the `.cpp`. - `fb/` keeps what is genuinely internal: the batch reader, the Iceberg field-id projection helpers, and `FieldIdResolver`. In the open-source build the hook returns nullptr and the factory raises a clear error naming the session property, rather than failing to link. That path is unreachable in practice: `FileConfig::kSelectiveNimbleReaderEnabledSession` and `QueryConfig::kSelectiveNimbleReaderEnabled` both default to true, so every engine-driven read already takes the selective branch. The 28 `#include` sites move to the new header. They have to change in this commit rather than a follow-up, because deleting the old header breaks them immediately. On the CMake side, `nimble_velox_reader_factory` is a new library and `add_subdirectory(reader)` is ordered after `selective`, which it links. Behavior is unchanged on both branches; this is placement and linkage only. Note for reviewers: `mrs/ranklake/table_service/src/storage/table:nimble_index_tablet` is one of the 28 consumers and does not build. That is pre-existing on trunk -- it constructs `ReaderOptions{pool, &dataIoStats, &metadataIoStats}` against a class that only has a one-argument constructor -- and I verified it fails identically with this change shelved. Its include is updated for consistency, but the target is left otherwise untouched. Reviewed By: xiaoxmeng Differential Revision: D114807828
HuamengJiang
force-pushed
the
export-D114807828
branch
from
August 7, 2026 08:17
e2dbca9 to
725a505
Compare
HuamengJiang
added a commit
to HuamengJiang/velox-1
that referenced
this pull request
Aug 25, 2026
…or#18677) Summary: X-link: facebookincubator/nimble#1069 Gets the NIMBLE reader's registration surface out of `fb/`, so that internal and open-source builds register the same factory instead of each having their own entry point. Before this, `NimbleReaderFactory` -- and with it `registerNimbleReaderFactory()`, which is what Prism, Gluten, Axel, Axiom, and a long tail of tools call -- lived in `dwio/nimble/velox/reader/fb/NimbleReader.h`. Nothing about the factory is internal: it holds a `SelectiveNimbleReaderFactory`, reads one flag, and picks a reader. It was stuck in `fb/` by a single edge, namely that it constructs the batch reader, which does depend on internal-only code (`//dwio/api:velox-util`). So the edge is inverted rather than the factory being kept hostage to it: - `dwio/nimble/velox/reader/NimbleReaderFactory.{h,cpp}` is new, OSS-clean, and holds the factory, the dispatch, and the register/unregister pair. - The batch branch now goes through `detail::createBatchReader()`, declared in that header and defined once per build flavour: `fb/NimbleReader.cpp` for Buck, `BatchReaderOSS.cpp` for CMake. This is the same ODR-based split already used for `detail::defaultMetadata()` (`VeloxWriterDefaultMetadataOSS.cpp`) and `detail::initHook()` (`SelectiveNimbleReaderInitHookOSS.cpp`). - `fb/NimbleReader.h` is deleted. With the factory gone it declared nothing, since the batch `NimbleReader` and `NimbleRowReader` live in an anonymous namespace in the `.cpp`. - `fb/` keeps what is genuinely internal: the batch reader, the Iceberg field-id projection helpers, and `FieldIdResolver`. In the open-source build the hook returns nullptr and the factory raises a clear error naming the session property, rather than failing to link. That path is unreachable in practice: `FileConfig::kSelectiveNimbleReaderEnabledSession` and `QueryConfig::kSelectiveNimbleReaderEnabled` both default to true, so every engine-driven read already takes the selective branch. The 28 `#include` sites move to the new header. They have to change in this commit rather than a follow-up, because deleting the old header breaks them immediately. On the CMake side, `nimble_velox_reader_factory` is a new library and `add_subdirectory(reader)` is ordered after `selective`, which it links. Behavior is unchanged on both branches; this is placement and linkage only. Note for reviewers: `mrs/ranklake/table_service/src/storage/table:nimble_index_tablet` is one of the 28 consumers and does not build. That is pre-existing on trunk -- it constructs `ReaderOptions{pool, &dataIoStats, &metadataIoStats}` against a class that only has a one-argument constructor -- and I verified it fails identically with this change shelved. Its include is updated for consistency, but the target is left otherwise untouched. Reviewed By: xiaoxmeng Differential Revision: D114807828
HuamengJiang
added a commit
to HuamengJiang/velox-1
that referenced
this pull request
Aug 25, 2026
…or#18677) Summary: X-link: facebookincubator/nimble#1069 Gets the NIMBLE reader's registration surface out of `fb/`, so that internal and open-source builds register the same factory instead of each having their own entry point. Before this, `NimbleReaderFactory` -- and with it `registerNimbleReaderFactory()`, which is what Prism, Gluten, Axel, Axiom, and a long tail of tools call -- lived in `dwio/nimble/velox/reader/fb/NimbleReader.h`. Nothing about the factory is internal: it holds a `SelectiveNimbleReaderFactory`, reads one flag, and picks a reader. It was stuck in `fb/` by a single edge, namely that it constructs the batch reader, which does depend on internal-only code (`//dwio/api:velox-util`). So the edge is inverted rather than the factory being kept hostage to it: - `dwio/nimble/velox/reader/NimbleReaderFactory.{h,cpp}` is new, OSS-clean, and holds the factory, the dispatch, and the register/unregister pair. - The batch branch now goes through `detail::createBatchReader()`, declared in that header and defined once per build flavour: `fb/NimbleReader.cpp` for Buck, `BatchReaderOSS.cpp` for CMake. This is the same ODR-based split already used for `detail::defaultMetadata()` (`VeloxWriterDefaultMetadataOSS.cpp`) and `detail::initHook()` (`SelectiveNimbleReaderInitHookOSS.cpp`). - `fb/NimbleReader.h` is deleted. With the factory gone it declared nothing, since the batch `NimbleReader` and `NimbleRowReader` live in an anonymous namespace in the `.cpp`. - `fb/` keeps what is genuinely internal: the batch reader, the Iceberg field-id projection helpers, and `FieldIdResolver`. In the open-source build the hook returns nullptr and the factory raises a clear error naming the session property, rather than failing to link. That path is unreachable in practice: `FileConfig::kSelectiveNimbleReaderEnabledSession` and `QueryConfig::kSelectiveNimbleReaderEnabled` both default to true, so every engine-driven read already takes the selective branch. The 28 `#include` sites move to the new header. They have to change in this commit rather than a follow-up, because deleting the old header breaks them immediately. On the CMake side, `nimble_velox_reader_factory` is a new library and `add_subdirectory(reader)` is ordered after `selective`, which it links. The new `NimbleReaderFactory.h` is listed in the `add_library()` source list because velox's `check-header-ownership` pre-commit hook requires every OSS-exported `.h` to be named by some `CMakeLists.txt` target. Behavior of the reader path is unchanged on both branches; this is placement and linkage only. Note for reviewers -- one unrelated build fix is folded in, at #ranklake's target. `mrs/ranklake/table_service/src/storage/table:nimble_index_tablet` is one of the consumers of the moved header, and it did not build on trunk. The cause is unrelated to this refactor: `829e31d97d31` ("[velox]refactor: Add IO stats setters to ReaderOptions and use pool-only constructor", May 9 2026) removed the multi-argument `ReaderOptions` constructor, and `d78602ee9fb1` ("[velox] Refactor ReaderOptions IoStatistics from raw pointers to shared_ptr", May 15 2026) moved the stats to `shared_ptr`, but this call site was never ported and has been broken for roughly three months. Verified pre-existing: the failing statement is byte-identical on master. It went unnoticed because the target is orphaned -- `tablet_factory` wires up `json_tablet`, `sst_tablet` and `base_tablet` but not `nimble_index_tablet`, and `buck2 uquery "rdeps(fbcode//mrs/..., ...)"` returns nothing, so nothing depends on it and it has no tests. The port is mechanical: ``` - facebook::velox::io::IoStatistics dataIoStats; - facebook::velox::io::IoStatistics metadataIoStats; - facebook::velox::dwio::common::ReaderOptions readerOpts{ - pool.get(), &dataIoStats, &metadataIoStats}; + auto dataIoStats = std::make_shared<facebook::velox::io::IoStatistics>(); + auto metadataIoStats = std::make_shared<facebook::velox::io::IoStatistics>(); + facebook::velox::dwio::common::ReaderOptions readerOpts{pool.get()}; + readerOpts.setDataIoStats(dataIoStats).setMetadataIoStats(metadataIoStats); ``` This also closes a latent dangling-pointer bug: the old form handed raw pointers to two stack locals into `readerOpts`, which feeds `nimbleReader_`, a member that outlives the function. The `shared_ptr` form keeps them alive, which is what the velox change was for. Once the file compiled, CLANGTIDY could run on it for the first time and flagged a pre-existing `unused parameter 'blobData'` in the `mergeTabletBlobData` stub; that parameter name is now commented out. Verification there is compile-only, since the target has no tests and no dependents. Reviewed By: xiaoxmeng Differential Revision: D114807828
HuamengJiang
added a commit
to HuamengJiang/velox-1
that referenced
this pull request
Aug 25, 2026
…or#18677) Summary: X-link: facebookincubator/nimble#1069 Gets the NIMBLE reader's registration surface out of `fb/`, so that internal and open-source builds register the same factory instead of each having their own entry point. Before this, `NimbleReaderFactory` -- and with it `registerNimbleReaderFactory()`, which is what Prism, Gluten, Axel, Axiom, and a long tail of tools call -- lived in `dwio/nimble/velox/reader/fb/NimbleReader.h`. Nothing about the factory is internal: it holds a `SelectiveNimbleReaderFactory`, reads one flag, and picks a reader. It was stuck in `fb/` by a single edge, namely that it constructs the batch reader, which does depend on internal-only code (`//dwio/api:velox-util`). So the edge is inverted rather than the factory being kept hostage to it: - `dwio/nimble/velox/reader/NimbleReaderFactory.{h,cpp}` is new, OSS-clean, and holds the factory, the dispatch, and the register/unregister pair. - The batch branch now goes through `detail::createBatchReader()`, declared in that header and defined once per build flavour: `fb/NimbleReader.cpp` for Buck, `BatchReaderOSS.cpp` for CMake. This is the same ODR-based split already used for `detail::defaultMetadata()` (`VeloxWriterDefaultMetadataOSS.cpp`) and `detail::initHook()` (`SelectiveNimbleReaderInitHookOSS.cpp`). - `fb/NimbleReader.h` is deleted. With the factory gone it declared nothing, since the batch `NimbleReader` and `NimbleRowReader` live in an anonymous namespace in the `.cpp`. - `fb/` keeps what is genuinely internal: the batch reader, the Iceberg field-id projection helpers, and `FieldIdResolver`. In the open-source build the hook returns nullptr and the factory raises a clear error naming the session property, rather than failing to link. That path is unreachable in practice: `FileConfig::kSelectiveNimbleReaderEnabledSession` and `QueryConfig::kSelectiveNimbleReaderEnabled` both default to true, so every engine-driven read already takes the selective branch. The 28 `#include` sites move to the new header. They have to change in this commit rather than a follow-up, because deleting the old header breaks them immediately. On the CMake side, `nimble_velox_reader_factory` is a new library and `add_subdirectory(reader)` is ordered after `selective`, which it links. The new `NimbleReaderFactory.h` is listed in the `add_library()` source list because velox's `check-header-ownership` pre-commit hook requires every OSS-exported `.h` to be named by some `CMakeLists.txt` target. Behavior of the reader path is unchanged on both branches; this is placement and linkage only. Note for reviewers -- one unrelated build fix is folded in, at #ranklake's target. `mrs/ranklake/table_service/src/storage/table:nimble_index_tablet` is one of the consumers of the moved header, and it did not build on trunk. The cause is unrelated to this refactor: `829e31d97d31` ("[velox]refactor: Add IO stats setters to ReaderOptions and use pool-only constructor", May 9 2026) removed the multi-argument `ReaderOptions` constructor, and `d78602ee9fb1` ("[velox] Refactor ReaderOptions IoStatistics from raw pointers to shared_ptr", May 15 2026) moved the stats to `shared_ptr`, but this call site was never ported and has been broken for roughly three months. Verified pre-existing: the failing statement is byte-identical on master. It went unnoticed because the target is orphaned -- `tablet_factory` wires up `json_tablet`, `sst_tablet` and `base_tablet` but not `nimble_index_tablet`, and `buck2 uquery "rdeps(fbcode//mrs/..., ...)"` returns nothing, so nothing depends on it and it has no tests. The port is mechanical: ``` - facebook::velox::io::IoStatistics dataIoStats; - facebook::velox::io::IoStatistics metadataIoStats; - facebook::velox::dwio::common::ReaderOptions readerOpts{ - pool.get(), &dataIoStats, &metadataIoStats}; + auto dataIoStats = std::make_shared<facebook::velox::io::IoStatistics>(); + auto metadataIoStats = std::make_shared<facebook::velox::io::IoStatistics>(); + facebook::velox::dwio::common::ReaderOptions readerOpts{pool.get()}; + readerOpts.setDataIoStats(dataIoStats).setMetadataIoStats(metadataIoStats); ``` This also closes a latent dangling-pointer bug: the old form handed raw pointers to two stack locals into `readerOpts`, which feeds `nimbleReader_`, a member that outlives the function. The `shared_ptr` form keeps them alive, which is what the velox change was for. Once the file compiled, CLANGTIDY could run on it for the first time and flagged a pre-existing `unused parameter 'blobData'` in the `mergeTabletBlobData` stub; that parameter name is now commented out. Verification there is compile-only, since the target has no tests and no dependents. Reviewed By: xiaoxmeng Differential Revision: D114807828
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
1 participant
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.
Summary:
Gets the NIMBLE reader's registration surface out of
fb/, so that internal and open-source builds register the same factory instead of each having their own entry point.Before this,
NimbleReaderFactory-- and with itregisterNimbleReaderFactory(), which is what Prism, Gluten, Axel, Axiom, and a long tail of tools call -- lived indwio/nimble/velox/reader/fb/NimbleReader.h. Nothing about the factory is internal: it holds aSelectiveNimbleReaderFactory, reads one flag, and picks a reader. It was stuck infb/by a single edge, namely that it constructs the batch reader, which does depend on internal-only code (//dwio/api:velox-util).So the edge is inverted rather than the factory being kept hostage to it:
dwio/nimble/velox/reader/NimbleReaderFactory.{h,cpp}is new, OSS-clean, and holds the factory, the dispatch, and the register/unregister pair.detail::createBatchReader(), declared in that header and defined once per build flavour:fb/NimbleReader.cppfor Buck,BatchReaderOSS.cppfor CMake. This is the same ODR-based split already used fordetail::defaultMetadata()(VeloxWriterDefaultMetadataOSS.cpp) anddetail::initHook()(SelectiveNimbleReaderInitHookOSS.cpp).fb/NimbleReader.his deleted. With the factory gone it declared nothing, since the batchNimbleReaderandNimbleRowReaderlive in an anonymous namespace in the.cpp.fb/keeps what is genuinely internal: the batch reader, the Iceberg field-id projection helpers, andFieldIdResolver.In the open-source build the hook returns nullptr and the factory raises a clear error naming the session property, rather than failing to link. That path is unreachable in practice:
FileConfig::kSelectiveNimbleReaderEnabledSessionandQueryConfig::kSelectiveNimbleReaderEnabledboth default to true, so every engine-driven read already takes the selective branch.The 28
#includesites move to the new header. They have to change in this commit rather than a follow-up, because deleting the old header breaks them immediately.On the CMake side,
nimble_velox_reader_factoryis a new library andadd_subdirectory(reader)is ordered afterselective, which it links.Behavior is unchanged on both branches; this is placement and linkage only.
Note for reviewers:
mrs/ranklake/table_service/src/storage/table:nimble_index_tabletis one of the 28 consumers and does not build. That is pre-existing on trunk -- it constructsReaderOptions{pool, &dataIoStats, &metadataIoStats}against a class that only has a one-argument constructor -- and I verified it fails identically with this change shelved. Its include is updated for consistency, but the target is left otherwise untouched.Reviewed By: xiaoxmeng
Differential Revision: D114807828