GH-38309: [C++] build filesystems as separate modules

[bkietz]

Rationale for this change

Each filesystem implementation carries unique and potentially heavy dependencies, so it'd be useful to build them separately. Furthermore, one typically doesn't need all of them at the same time and building separate modules would allow them to be dynamically loaded as necessary. Finally, defining this interface allows custom filesystem implementations to be supported seamlessly.

What changes are included in this PR?

An initial sketch of a registry, with documentation as if the registry were complete to illustrate intended usage.

Are these changes tested?

A toy module is added and a single unit test too.

Are there any user-facing changes?

Users would be able to add their own filesystem implementations to the registry

• Closes: [C++] Allow filesystems to be implemented in separate library (+ move remote filesystems out of libarrow into their own shared libraries) #38309

[pitrou]

Ok, so I think we should provide the building blocks for lazy loading of filesystem libraries when a given URI scheme is requested. Something like:

// filesystem.{h,cc}using FileSystemFactory = std::function<
Result<std::shared_ptr<FileSystem>>(
const Uri& uri, const io::IOContext& io_context, std::string* out_path)>;
using FileSystemLoader = std::function<Status(const std::string& scheme)>;
Status RegisterFileSystemFactory(std::vector<std::string> schemes,
FileSystemFactory factory);
Status RegisterFileSystemLoader(std::vector<std::string> schemes,
FileSystemLoader loader);
Result<FileSystemLoader> MakeSharedLibraryFileSystemLoader(
std::string library_path, std::string init_function) {
using InitFuncType = Status(*)();
auto loader = [=](const std::string& scheme) -> Status {
ARROW_ASSIGN_OR_RAISE(void* ptr, SharedLibrary::LookupSymbol(library_path, init_symbol));
returnreinterpret_cast<InitFuncType>(ptr)();
};
}
Status RegisterSharedLibraryFileSystemLoader(
std::vector<std::string> schemes,
std::string library_path, std::string init_function) {
ARROW_ASSIGN_OR_RAISE(auto loader,
MakeSharedLibraryFileSystemLoader(std::move(library_path), std::move(init_function)));
returnRegisterFileSystemLoader(std::move(schemes), std::move(loader));
}
// io_util.hclassSharedLibrary {
public:static Result<SharedLibrary> Open(const std::string& path);
Result<void*> LookupSymbol(const std::string& symbol);
static Result<void*> LookupSymbol(const std::string& path, const std::string& symbol) {
ARROW_ASSIGN_OR_RAISE(auto lib, Open(path));
return lib.LookupSymbol(symbol);
}
};

And then you can use it such as:

// libarrow_s3.so
Status RegisterS3FileSystem() {
returnRegisterFileSystemFactory({"s3"}, &MakeS3FileSystem);
}
// some init code somewhere
Status InitLoadableFileSystems() {
RETURN_NOT_OK(RegisterSharedLibraryFileSystemLoader(
{"s3"}, "libarrow_s3.so", "RegisterS3FileSystem");
// other filesystems here...returnStatus::OK();
}

[bkietz]

I think we should provide the building blocks for lazy loading of filesystem libraries

I don't think this presents much improvement over using direct factories. The main advantage I see with a FileSystemLoader approach is that initialization code can be run as part of loading the filesystem, including calls to EnsureS3Initialized() and dynamic loading of the library.

However EnsureS3Initialized() could equally be called by the factory, or triggered by dynamic loading at the same time as the factory is registered. As for dynamic loading itself: either the library contains one of the built-in filesystems or it is a custom implementation. For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered. For a custom filesystem libarrow.so is unaware of the name of a library containing support for custom:// uris, and can only automatically load if

• some naming convention is adopted so that we can load libarrow_fs_custom.so
• the name is passed explicitly to libarrow.so with SetLibraryNameForScheme(uri, libname) which is slightly more complicated than passing the name to LoadLibrary(libname)

[pitrou]

For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered.

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

[bkietz]

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

I agree, but again: customization will not be resident in libarrow.so, and will require instructions like "ensure X before calling FileSystemFromUri". That being the case, I don't think any instructions could be simpler than "ensure the library is loaded before calling FileSystemFromUri".

In the specific case of a python package which renames or moves libarrow_fs_s3.so, we disable autoloading and have pyarrow/__init__.py call cffi.FFI.dlopen() with the correct path.

[bkietz]

In light of the lack of an obvious approach, I think I'll defer support for autoloading for the moment. The main feature of interest is the registry anyway, and adding autoloading will not be more difficult after the registry is defined.

[kou]

It makes sense.

[kou]

+1

[kou]

Do we need std::move() here?

Suggested change

	std::move(scheme), factory, std::move(finalizer),
	std::move(scheme), std::move(factory), std::move(finalizer),

[bkietz]

We do not since the factory is currently a function pointer instead of a std::function. I guess that could be changed as well for consistency with finalizer

[kou]

Ah, OK. I missed the FileSystemFactory definition. (I thought that it's a normal class.)
Then should we remove std::move() for factory here https://github.com/apache/arrow/pull/39067/files#diff-e487db128ea7075182ace948c56f2992b370a10b40d0bceef696becab1c9dabfR808 ?

[bkietz]

I have changed factory to also be a std::function for consistency with finalizer, so the std::move is warranted

[bkietz]

@github-actions crossbow submit -g cpp -g r -g python -g wheel

[github-actions]

Revision: 4925905ea42d9571f65237603b0acc7547c9730f

Submitted crossbow builds: ursacomputing/crossbow @ actions-348ec81e98

Task	Status
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3-amd64
test-debian-11-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-r-sanitizer
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

[pitrou]

The test-ubuntu-22.04-cpp-20 build failure seems unexpected. Can you perhaps try it locally and look through the build logs?

[bkietz]

The failure is in the orc external project. Since orc doesn't depend on arrow I think this must be spurious, and the retry has succeeded.

[pitrou]

Starting to look very neat. The remaining comments are minor. Thank you!

[bkietz]

Appveyor and macos failures seem unrelated.

[jorisvandenbossche]

Seems this caused some nightly failures in the minimal C++ builds, see

• example-cpp-minimal-build-static
• example-cpp-minimal-build-static-system-dependency

/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::LoadDynamicLibrary(char const*) [clone .localalias]':
io_util.cc:(.text+0x56e0): undefined reference to `dlopen'
/usr/bin/ld: io_util.cc:(.text+0x5721): undefined reference to `dlerror'
/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::GetSymbol(void*, char const*)':
io_util.cc:(.text+0x58c3): undefined reference to `dlsym'
/usr/bin/ld: io_util.cc:(.text+0x59c1): undefined reference to `dlerror'
collect2: error: ld returned 1 exit status

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit b235f83.

There was 1 benchmark result with an error:

• Commit Run on ursa-i9-9960x at 2024-03-14 19:19:32Z
  • dataframe-to-table (R) with dataset=chi_traffic_2020_Q1, language=R

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

[bkietz]

Rationale for this change

Each filesystem implementation carries unique and potentially heavy dependencies, so it'd be useful to build them separately. Furthermore, one typically doesn't need all of them at the same time and building separate modules would allow them to be dynamically loaded as necessary. Finally, defining this interface allows custom filesystem implementations to be supported seamlessly.

What changes are included in this PR?

An initial sketch of a registry, with documentation as if the registry were complete to illustrate intended usage.

Are these changes tested?

A toy module is added and a single unit test too.

Are there any user-facing changes?

Users would be able to add their own filesystem implementations to the registry

• Closes: [C++] Allow filesystems to be implemented in separate library (+ move remote filesystems out of libarrow into their own shared libraries) #38309

[pitrou]

Ok, so I think we should provide the building blocks for lazy loading of filesystem libraries when a given URI scheme is requested. Something like:

// filesystem.{h,cc}using FileSystemFactory = std::function<
Result<std::shared_ptr<FileSystem>>(
const Uri& uri, const io::IOContext& io_context, std::string* out_path)>;
using FileSystemLoader = std::function<Status(const std::string& scheme)>;
Status RegisterFileSystemFactory(std::vector<std::string> schemes,
FileSystemFactory factory);
Status RegisterFileSystemLoader(std::vector<std::string> schemes,
FileSystemLoader loader);
Result<FileSystemLoader> MakeSharedLibraryFileSystemLoader(
std::string library_path, std::string init_function) {
using InitFuncType = Status(*)();
auto loader = [=](const std::string& scheme) -> Status {
ARROW_ASSIGN_OR_RAISE(void* ptr, SharedLibrary::LookupSymbol(library_path, init_symbol));
returnreinterpret_cast<InitFuncType>(ptr)();
};
}
Status RegisterSharedLibraryFileSystemLoader(
std::vector<std::string> schemes,
std::string library_path, std::string init_function) {
ARROW_ASSIGN_OR_RAISE(auto loader,
MakeSharedLibraryFileSystemLoader(std::move(library_path), std::move(init_function)));
returnRegisterFileSystemLoader(std::move(schemes), std::move(loader));
}
// io_util.hclassSharedLibrary {
public:static Result<SharedLibrary> Open(const std::string& path);
Result<void*> LookupSymbol(const std::string& symbol);
static Result<void*> LookupSymbol(const std::string& path, const std::string& symbol) {
ARROW_ASSIGN_OR_RAISE(auto lib, Open(path));
return lib.LookupSymbol(symbol);
}
};

And then you can use it such as:

// libarrow_s3.so
Status RegisterS3FileSystem() {
returnRegisterFileSystemFactory({"s3"}, &MakeS3FileSystem);
}
// some init code somewhere
Status InitLoadableFileSystems() {
RETURN_NOT_OK(RegisterSharedLibraryFileSystemLoader(
{"s3"}, "libarrow_s3.so", "RegisterS3FileSystem");
// other filesystems here...returnStatus::OK();
}

[bkietz]

I think we should provide the building blocks for lazy loading of filesystem libraries

I don't think this presents much improvement over using direct factories. The main advantage I see with a FileSystemLoader approach is that initialization code can be run as part of loading the filesystem, including calls to EnsureS3Initialized() and dynamic loading of the library.

However EnsureS3Initialized() could equally be called by the factory, or triggered by dynamic loading at the same time as the factory is registered. As for dynamic loading itself: either the library contains one of the built-in filesystems or it is a custom implementation. For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered. For a custom filesystem libarrow.so is unaware of the name of a library containing support for custom:// uris, and can only automatically load if

• some naming convention is adopted so that we can load libarrow_fs_custom.so
• the name is passed explicitly to libarrow.so with SetLibraryNameForScheme(uri, libname) which is slightly more complicated than passing the name to LoadLibrary(libname)

[pitrou]

For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered.

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

[bkietz]

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

I agree, but again: customization will not be resident in libarrow.so, and will require instructions like "ensure X before calling FileSystemFromUri". That being the case, I don't think any instructions could be simpler than "ensure the library is loaded before calling FileSystemFromUri".

In the specific case of a python package which renames or moves libarrow_fs_s3.so, we disable autoloading and have pyarrow/__init__.py call cffi.FFI.dlopen() with the correct path.

[bkietz]

In light of the lack of an obvious approach, I think I'll defer support for autoloading for the moment. The main feature of interest is the registry anyway, and adding autoloading will not be more difficult after the registry is defined.

[kou]

It makes sense.

[kou]

+1

[kou]

Do we need std::move() here?

Suggested change

	std::move(scheme), factory, std::move(finalizer),
	std::move(scheme), std::move(factory), std::move(finalizer),

[bkietz]

We do not since the factory is currently a function pointer instead of a std::function. I guess that could be changed as well for consistency with finalizer

[kou]

Ah, OK. I missed the FileSystemFactory definition. (I thought that it's a normal class.)
Then should we remove std::move() for factory here https://github.com/apache/arrow/pull/39067/files#diff-e487db128ea7075182ace948c56f2992b370a10b40d0bceef696becab1c9dabfR808 ?

[bkietz]

I have changed factory to also be a std::function for consistency with finalizer, so the std::move is warranted

[bkietz]

@github-actions crossbow submit -g cpp -g r -g python -g wheel

[github-actions]

Revision: 4925905ea42d9571f65237603b0acc7547c9730f

Submitted crossbow builds: ursacomputing/crossbow @ actions-348ec81e98

Task	Status
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3-amd64
test-debian-11-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-r-sanitizer
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

[pitrou]

The test-ubuntu-22.04-cpp-20 build failure seems unexpected. Can you perhaps try it locally and look through the build logs?

[bkietz]

The failure is in the orc external project. Since orc doesn't depend on arrow I think this must be spurious, and the retry has succeeded.

[pitrou]

Starting to look very neat. The remaining comments are minor. Thank you!

[bkietz]

Appveyor and macos failures seem unrelated.

[jorisvandenbossche]

Seems this caused some nightly failures in the minimal C++ builds, see

• example-cpp-minimal-build-static
• example-cpp-minimal-build-static-system-dependency

/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::LoadDynamicLibrary(char const*) [clone .localalias]':
io_util.cc:(.text+0x56e0): undefined reference to `dlopen'
/usr/bin/ld: io_util.cc:(.text+0x5721): undefined reference to `dlerror'
/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::GetSymbol(void*, char const*)':
io_util.cc:(.text+0x58c3): undefined reference to `dlsym'
/usr/bin/ld: io_util.cc:(.text+0x59c1): undefined reference to `dlerror'
collect2: error: ld returned 1 exit status

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit b235f83.

There was 1 benchmark result with an error:

• Commit Run on ursa-i9-9960x at 2024-03-14 19:19:32Z
  • dataframe-to-table (R) with dataset=chi_traffic_2020_Q1, language=R

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

[bkietz]

Rationale for this change

Each filesystem implementation carries unique and potentially heavy dependencies, so it'd be useful to build them separately. Furthermore, one typically doesn't need all of them at the same time and building separate modules would allow them to be dynamically loaded as necessary. Finally, defining this interface allows custom filesystem implementations to be supported seamlessly.

What changes are included in this PR?

An initial sketch of a registry, with documentation as if the registry were complete to illustrate intended usage.

Are these changes tested?

A toy module is added and a single unit test too.

Are there any user-facing changes?

Users would be able to add their own filesystem implementations to the registry

• Closes: [C++] Allow filesystems to be implemented in separate library (+ move remote filesystems out of libarrow into their own shared libraries) #38309

[pitrou]

Ok, so I think we should provide the building blocks for lazy loading of filesystem libraries when a given URI scheme is requested. Something like:

// filesystem.{h,cc}using FileSystemFactory = std::function<
Result<std::shared_ptr<FileSystem>>(
const Uri& uri, const io::IOContext& io_context, std::string* out_path)>;
using FileSystemLoader = std::function<Status(const std::string& scheme)>;
Status RegisterFileSystemFactory(std::vector<std::string> schemes,
FileSystemFactory factory);
Status RegisterFileSystemLoader(std::vector<std::string> schemes,
FileSystemLoader loader);
Result<FileSystemLoader> MakeSharedLibraryFileSystemLoader(
std::string library_path, std::string init_function) {
using InitFuncType = Status(*)();
auto loader = [=](const std::string& scheme) -> Status {
ARROW_ASSIGN_OR_RAISE(void* ptr, SharedLibrary::LookupSymbol(library_path, init_symbol));
returnreinterpret_cast<InitFuncType>(ptr)();
};
}
Status RegisterSharedLibraryFileSystemLoader(
std::vector<std::string> schemes,
std::string library_path, std::string init_function) {
ARROW_ASSIGN_OR_RAISE(auto loader,
MakeSharedLibraryFileSystemLoader(std::move(library_path), std::move(init_function)));
returnRegisterFileSystemLoader(std::move(schemes), std::move(loader));
}
// io_util.hclassSharedLibrary {
public:static Result<SharedLibrary> Open(const std::string& path);
Result<void*> LookupSymbol(const std::string& symbol);
static Result<void*> LookupSymbol(const std::string& path, const std::string& symbol) {
ARROW_ASSIGN_OR_RAISE(auto lib, Open(path));
return lib.LookupSymbol(symbol);
}
};

And then you can use it such as:

// libarrow_s3.so
Status RegisterS3FileSystem() {
returnRegisterFileSystemFactory({"s3"}, &MakeS3FileSystem);
}
// some init code somewhere
Status InitLoadableFileSystems() {
RETURN_NOT_OK(RegisterSharedLibraryFileSystemLoader(
{"s3"}, "libarrow_s3.so", "RegisterS3FileSystem");
// other filesystems here...returnStatus::OK();
}

[bkietz]

I think we should provide the building blocks for lazy loading of filesystem libraries

I don't think this presents much improvement over using direct factories. The main advantage I see with a FileSystemLoader approach is that initialization code can be run as part of loading the filesystem, including calls to EnsureS3Initialized() and dynamic loading of the library.

However EnsureS3Initialized() could equally be called by the factory, or triggered by dynamic loading at the same time as the factory is registered. As for dynamic loading itself: either the library contains one of the built-in filesystems or it is a custom implementation. For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered. For a custom filesystem libarrow.so is unaware of the name of a library containing support for custom:// uris, and can only automatically load if

• some naming convention is adopted so that we can load libarrow_fs_custom.so
• the name is passed explicitly to libarrow.so with SetLibraryNameForScheme(uri, libname) which is slightly more complicated than passing the name to LoadLibrary(libname)

[pitrou]

For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered.

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

[bkietz]

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

I agree, but again: customization will not be resident in libarrow.so, and will require instructions like "ensure X before calling FileSystemFromUri". That being the case, I don't think any instructions could be simpler than "ensure the library is loaded before calling FileSystemFromUri".

In the specific case of a python package which renames or moves libarrow_fs_s3.so, we disable autoloading and have pyarrow/__init__.py call cffi.FFI.dlopen() with the correct path.

[bkietz]

In light of the lack of an obvious approach, I think I'll defer support for autoloading for the moment. The main feature of interest is the registry anyway, and adding autoloading will not be more difficult after the registry is defined.

[kou]

It makes sense.

[kou]

+1

[kou]

Do we need std::move() here?

Suggested change

	std::move(scheme), factory, std::move(finalizer),
	std::move(scheme), std::move(factory), std::move(finalizer),

[bkietz]

We do not since the factory is currently a function pointer instead of a std::function. I guess that could be changed as well for consistency with finalizer

[kou]

Ah, OK. I missed the FileSystemFactory definition. (I thought that it's a normal class.)
Then should we remove std::move() for factory here https://github.com/apache/arrow/pull/39067/files#diff-e487db128ea7075182ace948c56f2992b370a10b40d0bceef696becab1c9dabfR808 ?

[bkietz]

I have changed factory to also be a std::function for consistency with finalizer, so the std::move is warranted

[bkietz]

@github-actions crossbow submit -g cpp -g r -g python -g wheel

[github-actions]

Revision: 4925905ea42d9571f65237603b0acc7547c9730f

Submitted crossbow builds: ursacomputing/crossbow @ actions-348ec81e98

Task	Status
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3-amd64
test-debian-11-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-r-sanitizer
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

[pitrou]

The test-ubuntu-22.04-cpp-20 build failure seems unexpected. Can you perhaps try it locally and look through the build logs?

[bkietz]

The failure is in the orc external project. Since orc doesn't depend on arrow I think this must be spurious, and the retry has succeeded.

[pitrou]

Starting to look very neat. The remaining comments are minor. Thank you!

[bkietz]

Appveyor and macos failures seem unrelated.

[jorisvandenbossche]

Seems this caused some nightly failures in the minimal C++ builds, see

• example-cpp-minimal-build-static
• example-cpp-minimal-build-static-system-dependency

/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::LoadDynamicLibrary(char const*) [clone .localalias]':
io_util.cc:(.text+0x56e0): undefined reference to `dlopen'
/usr/bin/ld: io_util.cc:(.text+0x5721): undefined reference to `dlerror'
/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::GetSymbol(void*, char const*)':
io_util.cc:(.text+0x58c3): undefined reference to `dlsym'
/usr/bin/ld: io_util.cc:(.text+0x59c1): undefined reference to `dlerror'
collect2: error: ld returned 1 exit status

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit b235f83.

There was 1 benchmark result with an error:

• Commit Run on ursa-i9-9960x at 2024-03-14 19:19:32Z
  • dataframe-to-table (R) with dataset=chi_traffic_2020_Q1, language=R

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

[bkietz]

Rationale for this change

Each filesystem implementation carries unique and potentially heavy dependencies, so it'd be useful to build them separately. Furthermore, one typically doesn't need all of them at the same time and building separate modules would allow them to be dynamically loaded as necessary. Finally, defining this interface allows custom filesystem implementations to be supported seamlessly.

What changes are included in this PR?

An initial sketch of a registry, with documentation as if the registry were complete to illustrate intended usage.

Are these changes tested?

A toy module is added and a single unit test too.

Are there any user-facing changes?

Users would be able to add their own filesystem implementations to the registry

• Closes: [C++] Allow filesystems to be implemented in separate library (+ move remote filesystems out of libarrow into their own shared libraries) #38309

[pitrou]

Ok, so I think we should provide the building blocks for lazy loading of filesystem libraries when a given URI scheme is requested. Something like:

// filesystem.{h,cc}using FileSystemFactory = std::function<
Result<std::shared_ptr<FileSystem>>(
const Uri& uri, const io::IOContext& io_context, std::string* out_path)>;
using FileSystemLoader = std::function<Status(const std::string& scheme)>;
Status RegisterFileSystemFactory(std::vector<std::string> schemes,
FileSystemFactory factory);
Status RegisterFileSystemLoader(std::vector<std::string> schemes,
FileSystemLoader loader);
Result<FileSystemLoader> MakeSharedLibraryFileSystemLoader(
std::string library_path, std::string init_function) {
using InitFuncType = Status(*)();
auto loader = [=](const std::string& scheme) -> Status {
ARROW_ASSIGN_OR_RAISE(void* ptr, SharedLibrary::LookupSymbol(library_path, init_symbol));
returnreinterpret_cast<InitFuncType>(ptr)();
};
}
Status RegisterSharedLibraryFileSystemLoader(
std::vector<std::string> schemes,
std::string library_path, std::string init_function) {
ARROW_ASSIGN_OR_RAISE(auto loader,
MakeSharedLibraryFileSystemLoader(std::move(library_path), std::move(init_function)));
returnRegisterFileSystemLoader(std::move(schemes), std::move(loader));
}
// io_util.hclassSharedLibrary {
public:static Result<SharedLibrary> Open(const std::string& path);
Result<void*> LookupSymbol(const std::string& symbol);
static Result<void*> LookupSymbol(const std::string& path, const std::string& symbol) {
ARROW_ASSIGN_OR_RAISE(auto lib, Open(path));
return lib.LookupSymbol(symbol);
}
};

And then you can use it such as:

// libarrow_s3.so
Status RegisterS3FileSystem() {
returnRegisterFileSystemFactory({"s3"}, &MakeS3FileSystem);
}
// some init code somewhere
Status InitLoadableFileSystems() {
RETURN_NOT_OK(RegisterSharedLibraryFileSystemLoader(
{"s3"}, "libarrow_s3.so", "RegisterS3FileSystem");
// other filesystems here...returnStatus::OK();
}

[bkietz]

I think we should provide the building blocks for lazy loading of filesystem libraries

I don't think this presents much improvement over using direct factories. The main advantage I see with a FileSystemLoader approach is that initialization code can be run as part of loading the filesystem, including calls to EnsureS3Initialized() and dynamic loading of the library.

However EnsureS3Initialized() could equally be called by the factory, or triggered by dynamic loading at the same time as the factory is registered. As for dynamic loading itself: either the library contains one of the built-in filesystems or it is a custom implementation. For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered. For a custom filesystem libarrow.so is unaware of the name of a library containing support for custom:// uris, and can only automatically load if

• some naming convention is adopted so that we can load libarrow_fs_custom.so
• the name is passed explicitly to libarrow.so with SetLibraryNameForScheme(uri, libname) which is slightly more complicated than passing the name to LoadLibrary(libname)

[pitrou]

For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered.

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

[bkietz]

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

I agree, but again: customization will not be resident in libarrow.so, and will require instructions like "ensure X before calling FileSystemFromUri". That being the case, I don't think any instructions could be simpler than "ensure the library is loaded before calling FileSystemFromUri".

In the specific case of a python package which renames or moves libarrow_fs_s3.so, we disable autoloading and have pyarrow/__init__.py call cffi.FFI.dlopen() with the correct path.

[bkietz]

In light of the lack of an obvious approach, I think I'll defer support for autoloading for the moment. The main feature of interest is the registry anyway, and adding autoloading will not be more difficult after the registry is defined.

[kou]

It makes sense.

[kou]

+1

[kou]

Do we need std::move() here?

Suggested change

	std::move(scheme), factory, std::move(finalizer),
	std::move(scheme), std::move(factory), std::move(finalizer),

[bkietz]

We do not since the factory is currently a function pointer instead of a std::function. I guess that could be changed as well for consistency with finalizer

[kou]

Ah, OK. I missed the FileSystemFactory definition. (I thought that it's a normal class.)
Then should we remove std::move() for factory here https://github.com/apache/arrow/pull/39067/files#diff-e487db128ea7075182ace948c56f2992b370a10b40d0bceef696becab1c9dabfR808 ?

[bkietz]

I have changed factory to also be a std::function for consistency with finalizer, so the std::move is warranted

[bkietz]

@github-actions crossbow submit -g cpp -g r -g python -g wheel

[github-actions]

Revision: 4925905ea42d9571f65237603b0acc7547c9730f

Submitted crossbow builds: ursacomputing/crossbow @ actions-348ec81e98

Task	Status
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3-amd64
test-debian-11-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-r-sanitizer
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

[pitrou]

The test-ubuntu-22.04-cpp-20 build failure seems unexpected. Can you perhaps try it locally and look through the build logs?

[bkietz]

The failure is in the orc external project. Since orc doesn't depend on arrow I think this must be spurious, and the retry has succeeded.

[pitrou]

Starting to look very neat. The remaining comments are minor. Thank you!

[bkietz]

Appveyor and macos failures seem unrelated.

[jorisvandenbossche]

Seems this caused some nightly failures in the minimal C++ builds, see

• example-cpp-minimal-build-static
• example-cpp-minimal-build-static-system-dependency

/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::LoadDynamicLibrary(char const*) [clone .localalias]':
io_util.cc:(.text+0x56e0): undefined reference to `dlopen'
/usr/bin/ld: io_util.cc:(.text+0x5721): undefined reference to `dlerror'
/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::GetSymbol(void*, char const*)':
io_util.cc:(.text+0x58c3): undefined reference to `dlsym'
/usr/bin/ld: io_util.cc:(.text+0x59c1): undefined reference to `dlerror'
collect2: error: ld returned 1 exit status

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit b235f83.

There was 1 benchmark result with an error:

• Commit Run on ursa-i9-9960x at 2024-03-14 19:19:32Z
  • dataframe-to-table (R) with dataset=chi_traffic_2020_Q1, language=R

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

[bkietz]

Rationale for this change

Each filesystem implementation carries unique and potentially heavy dependencies, so it'd be useful to build them separately. Furthermore, one typically doesn't need all of them at the same time and building separate modules would allow them to be dynamically loaded as necessary. Finally, defining this interface allows custom filesystem implementations to be supported seamlessly.

What changes are included in this PR?

An initial sketch of a registry, with documentation as if the registry were complete to illustrate intended usage.

Are these changes tested?

A toy module is added and a single unit test too.

Are there any user-facing changes?

Users would be able to add their own filesystem implementations to the registry

• Closes: [C++] Allow filesystems to be implemented in separate library (+ move remote filesystems out of libarrow into their own shared libraries) #38309

[pitrou]

Ok, so I think we should provide the building blocks for lazy loading of filesystem libraries when a given URI scheme is requested. Something like:

// filesystem.{h,cc}using FileSystemFactory = std::function<
Result<std::shared_ptr<FileSystem>>(
const Uri& uri, const io::IOContext& io_context, std::string* out_path)>;
using FileSystemLoader = std::function<Status(const std::string& scheme)>;
Status RegisterFileSystemFactory(std::vector<std::string> schemes,
FileSystemFactory factory);
Status RegisterFileSystemLoader(std::vector<std::string> schemes,
FileSystemLoader loader);
Result<FileSystemLoader> MakeSharedLibraryFileSystemLoader(
std::string library_path, std::string init_function) {
using InitFuncType = Status(*)();
auto loader = [=](const std::string& scheme) -> Status {
ARROW_ASSIGN_OR_RAISE(void* ptr, SharedLibrary::LookupSymbol(library_path, init_symbol));
returnreinterpret_cast<InitFuncType>(ptr)();
};
}
Status RegisterSharedLibraryFileSystemLoader(
std::vector<std::string> schemes,
std::string library_path, std::string init_function) {
ARROW_ASSIGN_OR_RAISE(auto loader,
MakeSharedLibraryFileSystemLoader(std::move(library_path), std::move(init_function)));
returnRegisterFileSystemLoader(std::move(schemes), std::move(loader));
}
// io_util.hclassSharedLibrary {
public:static Result<SharedLibrary> Open(const std::string& path);
Result<void*> LookupSymbol(const std::string& symbol);
static Result<void*> LookupSymbol(const std::string& path, const std::string& symbol) {
ARROW_ASSIGN_OR_RAISE(auto lib, Open(path));
return lib.LookupSymbol(symbol);
}
};

And then you can use it such as:

// libarrow_s3.so
Status RegisterS3FileSystem() {
returnRegisterFileSystemFactory({"s3"}, &MakeS3FileSystem);
}
// some init code somewhere
Status InitLoadableFileSystems() {
RETURN_NOT_OK(RegisterSharedLibraryFileSystemLoader(
{"s3"}, "libarrow_s3.so", "RegisterS3FileSystem");
// other filesystems here...returnStatus::OK();
}

[bkietz]

I think we should provide the building blocks for lazy loading of filesystem libraries

I don't think this presents much improvement over using direct factories. The main advantage I see with a FileSystemLoader approach is that initialization code can be run as part of loading the filesystem, including calls to EnsureS3Initialized() and dynamic loading of the library.

However EnsureS3Initialized() could equally be called by the factory, or triggered by dynamic loading at the same time as the factory is registered. As for dynamic loading itself: either the library contains one of the built-in filesystems or it is a custom implementation. For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered. For a custom filesystem libarrow.so is unaware of the name of a library containing support for custom:// uris, and can only automatically load if

• some naming convention is adopted so that we can load libarrow_fs_custom.so
• the name is passed explicitly to libarrow.so with SetLibraryNameForScheme(uri, libname) which is slightly more complicated than passing the name to LoadLibrary(libname)

[pitrou]

For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered.

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

[bkietz]

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

I agree, but again: customization will not be resident in libarrow.so, and will require instructions like "ensure X before calling FileSystemFromUri". That being the case, I don't think any instructions could be simpler than "ensure the library is loaded before calling FileSystemFromUri".

In the specific case of a python package which renames or moves libarrow_fs_s3.so, we disable autoloading and have pyarrow/__init__.py call cffi.FFI.dlopen() with the correct path.

[bkietz]

In light of the lack of an obvious approach, I think I'll defer support for autoloading for the moment. The main feature of interest is the registry anyway, and adding autoloading will not be more difficult after the registry is defined.

[kou]

It makes sense.

[kou]

+1

[kou]

Do we need std::move() here?

Suggested change

	std::move(scheme), factory, std::move(finalizer),
	std::move(scheme), std::move(factory), std::move(finalizer),

[bkietz]

We do not since the factory is currently a function pointer instead of a std::function. I guess that could be changed as well for consistency with finalizer

[kou]

Ah, OK. I missed the FileSystemFactory definition. (I thought that it's a normal class.)
Then should we remove std::move() for factory here https://github.com/apache/arrow/pull/39067/files#diff-e487db128ea7075182ace948c56f2992b370a10b40d0bceef696becab1c9dabfR808 ?

[bkietz]

I have changed factory to also be a std::function for consistency with finalizer, so the std::move is warranted

[bkietz]

@github-actions crossbow submit -g cpp -g r -g python -g wheel

[github-actions]

Revision: 4925905ea42d9571f65237603b0acc7547c9730f

Submitted crossbow builds: ursacomputing/crossbow @ actions-348ec81e98

Task	Status
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3-amd64
test-debian-11-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-r-sanitizer
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

[pitrou]

The test-ubuntu-22.04-cpp-20 build failure seems unexpected. Can you perhaps try it locally and look through the build logs?

[bkietz]

The failure is in the orc external project. Since orc doesn't depend on arrow I think this must be spurious, and the retry has succeeded.

[pitrou]

Starting to look very neat. The remaining comments are minor. Thank you!

[bkietz]

Appveyor and macos failures seem unrelated.

[jorisvandenbossche]

Seems this caused some nightly failures in the minimal C++ builds, see

• example-cpp-minimal-build-static
• example-cpp-minimal-build-static-system-dependency

/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::LoadDynamicLibrary(char const*) [clone .localalias]':
io_util.cc:(.text+0x56e0): undefined reference to `dlopen'
/usr/bin/ld: io_util.cc:(.text+0x5721): undefined reference to `dlerror'
/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::GetSymbol(void*, char const*)':
io_util.cc:(.text+0x58c3): undefined reference to `dlsym'
/usr/bin/ld: io_util.cc:(.text+0x59c1): undefined reference to `dlerror'
collect2: error: ld returned 1 exit status

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit b235f83.

There was 1 benchmark result with an error:

• Commit Run on ursa-i9-9960x at 2024-03-14 19:19:32Z
  • dataframe-to-table (R) with dataset=chi_traffic_2020_Q1, language=R

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

[bkietz]

Rationale for this change

Each filesystem implementation carries unique and potentially heavy dependencies, so it'd be useful to build them separately. Furthermore, one typically doesn't need all of them at the same time and building separate modules would allow them to be dynamically loaded as necessary. Finally, defining this interface allows custom filesystem implementations to be supported seamlessly.

What changes are included in this PR?

An initial sketch of a registry, with documentation as if the registry were complete to illustrate intended usage.

Are these changes tested?

A toy module is added and a single unit test too.

Are there any user-facing changes?

Users would be able to add their own filesystem implementations to the registry

• Closes: [C++] Allow filesystems to be implemented in separate library (+ move remote filesystems out of libarrow into their own shared libraries) #38309

[pitrou]

Ok, so I think we should provide the building blocks for lazy loading of filesystem libraries when a given URI scheme is requested. Something like:

// filesystem.{h,cc}using FileSystemFactory = std::function<
Result<std::shared_ptr<FileSystem>>(
const Uri& uri, const io::IOContext& io_context, std::string* out_path)>;
using FileSystemLoader = std::function<Status(const std::string& scheme)>;
Status RegisterFileSystemFactory(std::vector<std::string> schemes,
FileSystemFactory factory);
Status RegisterFileSystemLoader(std::vector<std::string> schemes,
FileSystemLoader loader);
Result<FileSystemLoader> MakeSharedLibraryFileSystemLoader(
std::string library_path, std::string init_function) {
using InitFuncType = Status(*)();
auto loader = [=](const std::string& scheme) -> Status {
ARROW_ASSIGN_OR_RAISE(void* ptr, SharedLibrary::LookupSymbol(library_path, init_symbol));
returnreinterpret_cast<InitFuncType>(ptr)();
};
}
Status RegisterSharedLibraryFileSystemLoader(
std::vector<std::string> schemes,
std::string library_path, std::string init_function) {
ARROW_ASSIGN_OR_RAISE(auto loader,
MakeSharedLibraryFileSystemLoader(std::move(library_path), std::move(init_function)));
returnRegisterFileSystemLoader(std::move(schemes), std::move(loader));
}
// io_util.hclassSharedLibrary {
public:static Result<SharedLibrary> Open(const std::string& path);
Result<void*> LookupSymbol(const std::string& symbol);
static Result<void*> LookupSymbol(const std::string& path, const std::string& symbol) {
ARROW_ASSIGN_OR_RAISE(auto lib, Open(path));
return lib.LookupSymbol(symbol);
}
};

And then you can use it such as:

// libarrow_s3.so
Status RegisterS3FileSystem() {
returnRegisterFileSystemFactory({"s3"}, &MakeS3FileSystem);
}
// some init code somewhere
Status InitLoadableFileSystems() {
RETURN_NOT_OK(RegisterSharedLibraryFileSystemLoader(
{"s3"}, "libarrow_s3.so", "RegisterS3FileSystem");
// other filesystems here...returnStatus::OK();
}

[bkietz]

I think we should provide the building blocks for lazy loading of filesystem libraries

I don't think this presents much improvement over using direct factories. The main advantage I see with a FileSystemLoader approach is that initialization code can be run as part of loading the filesystem, including calls to EnsureS3Initialized() and dynamic loading of the library.

However EnsureS3Initialized() could equally be called by the factory, or triggered by dynamic loading at the same time as the factory is registered. As for dynamic loading itself: either the library contains one of the built-in filesystems or it is a custom implementation. For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered. For a custom filesystem libarrow.so is unaware of the name of a library containing support for custom:// uris, and can only automatically load if

• some naming convention is adopted so that we can load libarrow_fs_custom.so
• the name is passed explicitly to libarrow.so with SetLibraryNameForScheme(uri, libname) which is slightly more complicated than passing the name to LoadLibrary(libname)

[pitrou]

For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered.

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

[bkietz]

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

I agree, but again: customization will not be resident in libarrow.so, and will require instructions like "ensure X before calling FileSystemFromUri". That being the case, I don't think any instructions could be simpler than "ensure the library is loaded before calling FileSystemFromUri".

In the specific case of a python package which renames or moves libarrow_fs_s3.so, we disable autoloading and have pyarrow/__init__.py call cffi.FFI.dlopen() with the correct path.

[bkietz]

In light of the lack of an obvious approach, I think I'll defer support for autoloading for the moment. The main feature of interest is the registry anyway, and adding autoloading will not be more difficult after the registry is defined.

[kou]

It makes sense.

[kou]

+1

[kou]

Do we need std::move() here?

Suggested change

	std::move(scheme), factory, std::move(finalizer),
	std::move(scheme), std::move(factory), std::move(finalizer),

[bkietz]

We do not since the factory is currently a function pointer instead of a std::function. I guess that could be changed as well for consistency with finalizer

[kou]

Ah, OK. I missed the FileSystemFactory definition. (I thought that it's a normal class.)
Then should we remove std::move() for factory here https://github.com/apache/arrow/pull/39067/files#diff-e487db128ea7075182ace948c56f2992b370a10b40d0bceef696becab1c9dabfR808 ?

[bkietz]

I have changed factory to also be a std::function for consistency with finalizer, so the std::move is warranted

[bkietz]

@github-actions crossbow submit -g cpp -g r -g python -g wheel

[github-actions]

Revision: 4925905ea42d9571f65237603b0acc7547c9730f

Submitted crossbow builds: ursacomputing/crossbow @ actions-348ec81e98

Task	Status
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3-amd64
test-debian-11-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-r-sanitizer
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

[pitrou]

The test-ubuntu-22.04-cpp-20 build failure seems unexpected. Can you perhaps try it locally and look through the build logs?

[bkietz]

The failure is in the orc external project. Since orc doesn't depend on arrow I think this must be spurious, and the retry has succeeded.

[pitrou]

Starting to look very neat. The remaining comments are minor. Thank you!

[bkietz]

Appveyor and macos failures seem unrelated.

[jorisvandenbossche]

Seems this caused some nightly failures in the minimal C++ builds, see

• example-cpp-minimal-build-static
• example-cpp-minimal-build-static-system-dependency

/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::LoadDynamicLibrary(char const*) [clone .localalias]':
io_util.cc:(.text+0x56e0): undefined reference to `dlopen'
/usr/bin/ld: io_util.cc:(.text+0x5721): undefined reference to `dlerror'
/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::GetSymbol(void*, char const*)':
io_util.cc:(.text+0x58c3): undefined reference to `dlsym'
/usr/bin/ld: io_util.cc:(.text+0x59c1): undefined reference to `dlerror'
collect2: error: ld returned 1 exit status

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit b235f83.

There was 1 benchmark result with an error:

• Commit Run on ursa-i9-9960x at 2024-03-14 19:19:32Z
  • dataframe-to-table (R) with dataset=chi_traffic_2020_Q1, language=R

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

[bkietz]

Rationale for this change

Each filesystem implementation carries unique and potentially heavy dependencies, so it'd be useful to build them separately. Furthermore, one typically doesn't need all of them at the same time and building separate modules would allow them to be dynamically loaded as necessary. Finally, defining this interface allows custom filesystem implementations to be supported seamlessly.

What changes are included in this PR?

An initial sketch of a registry, with documentation as if the registry were complete to illustrate intended usage.

Are these changes tested?

A toy module is added and a single unit test too.

Are there any user-facing changes?

Users would be able to add their own filesystem implementations to the registry

• Closes: [C++] Allow filesystems to be implemented in separate library (+ move remote filesystems out of libarrow into their own shared libraries) #38309

[pitrou]

Ok, so I think we should provide the building blocks for lazy loading of filesystem libraries when a given URI scheme is requested. Something like:

// filesystem.{h,cc}using FileSystemFactory = std::function<
Result<std::shared_ptr<FileSystem>>(
const Uri& uri, const io::IOContext& io_context, std::string* out_path)>;
using FileSystemLoader = std::function<Status(const std::string& scheme)>;
Status RegisterFileSystemFactory(std::vector<std::string> schemes,
FileSystemFactory factory);
Status RegisterFileSystemLoader(std::vector<std::string> schemes,
FileSystemLoader loader);
Result<FileSystemLoader> MakeSharedLibraryFileSystemLoader(
std::string library_path, std::string init_function) {
using InitFuncType = Status(*)();
auto loader = [=](const std::string& scheme) -> Status {
ARROW_ASSIGN_OR_RAISE(void* ptr, SharedLibrary::LookupSymbol(library_path, init_symbol));
returnreinterpret_cast<InitFuncType>(ptr)();
};
}
Status RegisterSharedLibraryFileSystemLoader(
std::vector<std::string> schemes,
std::string library_path, std::string init_function) {
ARROW_ASSIGN_OR_RAISE(auto loader,
MakeSharedLibraryFileSystemLoader(std::move(library_path), std::move(init_function)));
returnRegisterFileSystemLoader(std::move(schemes), std::move(loader));
}
// io_util.hclassSharedLibrary {
public:static Result<SharedLibrary> Open(const std::string& path);
Result<void*> LookupSymbol(const std::string& symbol);
static Result<void*> LookupSymbol(const std::string& path, const std::string& symbol) {
ARROW_ASSIGN_OR_RAISE(auto lib, Open(path));
return lib.LookupSymbol(symbol);
}
};

And then you can use it such as:

// libarrow_s3.so
Status RegisterS3FileSystem() {
returnRegisterFileSystemFactory({"s3"}, &MakeS3FileSystem);
}
// some init code somewhere
Status InitLoadableFileSystems() {
RETURN_NOT_OK(RegisterSharedLibraryFileSystemLoader(
{"s3"}, "libarrow_s3.so", "RegisterS3FileSystem");
// other filesystems here...returnStatus::OK();
}

[bkietz]

I think we should provide the building blocks for lazy loading of filesystem libraries

I don't think this presents much improvement over using direct factories. The main advantage I see with a FileSystemLoader approach is that initialization code can be run as part of loading the filesystem, including calls to EnsureS3Initialized() and dynamic loading of the library.

However EnsureS3Initialized() could equally be called by the factory, or triggered by dynamic loading at the same time as the factory is registered. As for dynamic loading itself: either the library contains one of the built-in filesystems or it is a custom implementation. For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered. For a custom filesystem libarrow.so is unaware of the name of a library containing support for custom:// uris, and can only automatically load if

• some naming convention is adopted so that we can load libarrow_fs_custom.so
• the name is passed explicitly to libarrow.so with SetLibraryNameForScheme(uri, libname) which is slightly more complicated than passing the name to LoadLibrary(libname)

[pitrou]

For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered.

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

[bkietz]

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

I agree, but again: customization will not be resident in libarrow.so, and will require instructions like "ensure X before calling FileSystemFromUri". That being the case, I don't think any instructions could be simpler than "ensure the library is loaded before calling FileSystemFromUri".

In the specific case of a python package which renames or moves libarrow_fs_s3.so, we disable autoloading and have pyarrow/__init__.py call cffi.FFI.dlopen() with the correct path.

[bkietz]

In light of the lack of an obvious approach, I think I'll defer support for autoloading for the moment. The main feature of interest is the registry anyway, and adding autoloading will not be more difficult after the registry is defined.

[kou]

It makes sense.

[kou]

+1

[kou]

Do we need std::move() here?

Suggested change

	std::move(scheme), factory, std::move(finalizer),
	std::move(scheme), std::move(factory), std::move(finalizer),

[bkietz]

We do not since the factory is currently a function pointer instead of a std::function. I guess that could be changed as well for consistency with finalizer

[kou]

Ah, OK. I missed the FileSystemFactory definition. (I thought that it's a normal class.)
Then should we remove std::move() for factory here https://github.com/apache/arrow/pull/39067/files#diff-e487db128ea7075182ace948c56f2992b370a10b40d0bceef696becab1c9dabfR808 ?

[bkietz]

I have changed factory to also be a std::function for consistency with finalizer, so the std::move is warranted

[bkietz]

@github-actions crossbow submit -g cpp -g r -g python -g wheel

[github-actions]

Revision: 4925905ea42d9571f65237603b0acc7547c9730f

Submitted crossbow builds: ursacomputing/crossbow @ actions-348ec81e98

Task	Status
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3-amd64
test-debian-11-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-r-sanitizer
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

[pitrou]

The test-ubuntu-22.04-cpp-20 build failure seems unexpected. Can you perhaps try it locally and look through the build logs?

[bkietz]

The failure is in the orc external project. Since orc doesn't depend on arrow I think this must be spurious, and the retry has succeeded.

[pitrou]

Starting to look very neat. The remaining comments are minor. Thank you!

[bkietz]

Appveyor and macos failures seem unrelated.

[jorisvandenbossche]

Seems this caused some nightly failures in the minimal C++ builds, see

• example-cpp-minimal-build-static
• example-cpp-minimal-build-static-system-dependency

/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::LoadDynamicLibrary(char const*) [clone .localalias]':
io_util.cc:(.text+0x56e0): undefined reference to `dlopen'
/usr/bin/ld: io_util.cc:(.text+0x5721): undefined reference to `dlerror'
/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::GetSymbol(void*, char const*)':
io_util.cc:(.text+0x58c3): undefined reference to `dlsym'
/usr/bin/ld: io_util.cc:(.text+0x59c1): undefined reference to `dlerror'
collect2: error: ld returned 1 exit status

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit b235f83.

There was 1 benchmark result with an error:

• Commit Run on ursa-i9-9960x at 2024-03-14 19:19:32Z
  • dataframe-to-table (R) with dataset=chi_traffic_2020_Q1, language=R

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

[bkietz]

Rationale for this change

Each filesystem implementation carries unique and potentially heavy dependencies, so it'd be useful to build them separately. Furthermore, one typically doesn't need all of them at the same time and building separate modules would allow them to be dynamically loaded as necessary. Finally, defining this interface allows custom filesystem implementations to be supported seamlessly.

What changes are included in this PR?

An initial sketch of a registry, with documentation as if the registry were complete to illustrate intended usage.

Are these changes tested?

A toy module is added and a single unit test too.

Are there any user-facing changes?

Users would be able to add their own filesystem implementations to the registry

• Closes: [C++] Allow filesystems to be implemented in separate library (+ move remote filesystems out of libarrow into their own shared libraries) #38309

[pitrou]

Ok, so I think we should provide the building blocks for lazy loading of filesystem libraries when a given URI scheme is requested. Something like:

// filesystem.{h,cc}using FileSystemFactory = std::function<
Result<std::shared_ptr<FileSystem>>(
const Uri& uri, const io::IOContext& io_context, std::string* out_path)>;
using FileSystemLoader = std::function<Status(const std::string& scheme)>;
Status RegisterFileSystemFactory(std::vector<std::string> schemes,
FileSystemFactory factory);
Status RegisterFileSystemLoader(std::vector<std::string> schemes,
FileSystemLoader loader);
Result<FileSystemLoader> MakeSharedLibraryFileSystemLoader(
std::string library_path, std::string init_function) {
using InitFuncType = Status(*)();
auto loader = [=](const std::string& scheme) -> Status {
ARROW_ASSIGN_OR_RAISE(void* ptr, SharedLibrary::LookupSymbol(library_path, init_symbol));
returnreinterpret_cast<InitFuncType>(ptr)();
};
}
Status RegisterSharedLibraryFileSystemLoader(
std::vector<std::string> schemes,
std::string library_path, std::string init_function) {
ARROW_ASSIGN_OR_RAISE(auto loader,
MakeSharedLibraryFileSystemLoader(std::move(library_path), std::move(init_function)));
returnRegisterFileSystemLoader(std::move(schemes), std::move(loader));
}
// io_util.hclassSharedLibrary {
public:static Result<SharedLibrary> Open(const std::string& path);
Result<void*> LookupSymbol(const std::string& symbol);
static Result<void*> LookupSymbol(const std::string& path, const std::string& symbol) {
ARROW_ASSIGN_OR_RAISE(auto lib, Open(path));
return lib.LookupSymbol(symbol);
}
};

And then you can use it such as:

// libarrow_s3.so
Status RegisterS3FileSystem() {
returnRegisterFileSystemFactory({"s3"}, &MakeS3FileSystem);
}
// some init code somewhere
Status InitLoadableFileSystems() {
RETURN_NOT_OK(RegisterSharedLibraryFileSystemLoader(
{"s3"}, "libarrow_s3.so", "RegisterS3FileSystem");
// other filesystems here...returnStatus::OK();
}

[bkietz]

I think we should provide the building blocks for lazy loading of filesystem libraries

I don't think this presents much improvement over using direct factories. The main advantage I see with a FileSystemLoader approach is that initialization code can be run as part of loading the filesystem, including calls to EnsureS3Initialized() and dynamic loading of the library.

However EnsureS3Initialized() could equally be called by the factory, or triggered by dynamic loading at the same time as the factory is registered. As for dynamic loading itself: either the library contains one of the built-in filesystems or it is a custom implementation. For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered. For a custom filesystem libarrow.so is unaware of the name of a library containing support for custom:// uris, and can only automatically load if

• some naming convention is adopted so that we can load libarrow_fs_custom.so
• the name is passed explicitly to libarrow.so with SetLibraryNameForScheme(uri, libname) which is slightly more complicated than passing the name to LoadLibrary(libname)

[pitrou]

For the builtin S3 filesystem, it's reasonable that libarrow.so could include the string "libarrow_fs_s3.so" in order to automatically load when an s3:// uri is encountered.

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

[bkietz]

This depends on how libarrow_fs_s3.so is distributed, and which directory it is installed. It is certainly a reasonable default setting, but it can be useful to make it customizable.

I agree, but again: customization will not be resident in libarrow.so, and will require instructions like "ensure X before calling FileSystemFromUri". That being the case, I don't think any instructions could be simpler than "ensure the library is loaded before calling FileSystemFromUri".

In the specific case of a python package which renames or moves libarrow_fs_s3.so, we disable autoloading and have pyarrow/__init__.py call cffi.FFI.dlopen() with the correct path.

[bkietz]

In light of the lack of an obvious approach, I think I'll defer support for autoloading for the moment. The main feature of interest is the registry anyway, and adding autoloading will not be more difficult after the registry is defined.

[kou]

It makes sense.

[kou]

+1

[kou]

Do we need std::move() here?

Suggested change

	std::move(scheme), factory, std::move(finalizer),
	std::move(scheme), std::move(factory), std::move(finalizer),

[bkietz]

We do not since the factory is currently a function pointer instead of a std::function. I guess that could be changed as well for consistency with finalizer

[kou]

Ah, OK. I missed the FileSystemFactory definition. (I thought that it's a normal class.)
Then should we remove std::move() for factory here https://github.com/apache/arrow/pull/39067/files#diff-e487db128ea7075182ace948c56f2992b370a10b40d0bceef696becab1c9dabfR808 ?

[bkietz]

I have changed factory to also be a std::function for consistency with finalizer, so the std::move is warranted

[bkietz]

@github-actions crossbow submit -g cpp -g r -g python -g wheel

[github-actions]

Revision: 4925905ea42d9571f65237603b0acc7547c9730f

Submitted crossbow builds: ursacomputing/crossbow @ actions-348ec81e98

Task	Status
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3-amd64
test-debian-11-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-r-sanitizer
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

[pitrou]

The test-ubuntu-22.04-cpp-20 build failure seems unexpected. Can you perhaps try it locally and look through the build logs?

[bkietz]

The failure is in the orc external project. Since orc doesn't depend on arrow I think this must be spurious, and the retry has succeeded.

[pitrou]

Starting to look very neat. The remaining comments are minor. Thank you!

[bkietz]

Appveyor and macos failures seem unrelated.

[jorisvandenbossche]

Seems this caused some nightly failures in the minimal C++ builds, see

• example-cpp-minimal-build-static
• example-cpp-minimal-build-static-system-dependency

/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::LoadDynamicLibrary(char const*) [clone .localalias]':
io_util.cc:(.text+0x56e0): undefined reference to `dlopen'
/usr/bin/ld: io_util.cc:(.text+0x5721): undefined reference to `dlerror'
/usr/bin/ld: /usr/local/lib/libarrow.a(io_util.cc.o): in function `arrow::internal::GetSymbol(void*, char const*)':
io_util.cc:(.text+0x58c3): undefined reference to `dlsym'
/usr/bin/ld: io_util.cc:(.text+0x59c1): undefined reference to `dlerror'
collect2: error: ld returned 1 exit status

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit b235f83.

There was 1 benchmark result with an error:

• Commit Run on ursa-i9-9960x at 2024-03-14 19:19:32Z
  • dataframe-to-table (R) with dataset=chi_traffic_2020_Q1, language=R

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.