GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code

[AlenkaF]

Rationale for this change

ObjectType and FileStatistics in io/hdfs.h have been deprecated for a while and can be removed.

What changes are included in this PR?

ObjectType and FileStatistics structs are removed and instead FileSystem API in arrow::fs is used. Together with this change, the hdfs connected code is moved from cpp/src/arrow/io to cpp/src/arrow/filesystem merging FileSystem and HadoopFileSystem classes from arrow::io into the public HadoopFileSystem class.

Are these changes tested?

Existing tests should pass.

Are there any user-facing changes?

Deprecated structs are removed and all hdfs related code is now a part of the filesystem module.

• GitHub Issue: [C++] Remove deprecated ObjectType and FileStatistics #45747

Also closes: #22457 (not sure about io/interfaces.h?)

[AlenkaF]

Ah, this will not work. If we want to remove ObjectType and FileStatistics from io/hdfs.h and instead use FileSystem API, then filesystem component would have to be enabled by default. Also, if I understand correctly, we want to do a refactoring of io/hdfs anyways which would include the changes in this PR? (#22457)

cc @pitrou

[pitrou]

Ah, this will not work. If we want to remove ObjectType and FileStatistics from io/hdfs.h and instead use FileSystem API, then filesystem component would have to be enabled by default.

Well, ARROW_HDFS=ON could imply ARROW_FILESYSTEM=ON. I don't think that's a problem.

Also, if I understand correctly, we want to do a refactoring of io/hdfs anyways which would include the changes in this PR?

Yes, indeed. io/hdfs could be moved to filesystem/hdfs_internal or something similar.

[AlenkaF]

OK, I will then move io/hdfs to filesystem/hdfs_internal. Thanks!

[AlenkaF]

@pitrou I think this is ready for review. The failing builds have an issue opened: #46077

[pitrou]

Thanks for doing this @AlenkaF! Here are some comments.

[pitrou]

Are all these declarations actually needed by PyArrow?

[AlenkaF]

No, most of them aren't and are copied from libarrow.pxd. I can remove the unused ones - but am not sure if some external application can actually use them?

[pitrou]

Don't we need to link to arrow::hadoop as was done above? cc @kou for advice

[AlenkaF]

Hm, yeah. I will add a link as above as it makes sense.

[AlenkaF]

Ah, it is there already (that explains why nothing failed =) )

arrow/cpp/src/arrow/CMakeLists.txt

Lines 857 to 861 in 53ef438

	if(ARROW_HDFS)
	foreach(ARROW_FILESYSTEM_TARGET ${ARROW_FILESYSTEM_TARGETS})
	target_link_libraries(${ARROW_FILESYSTEM_TARGET}PRIVATEarrow::hadoop)
	endforeach()
	endif()

Not sure if the line with CMAKE_DL_LIBS is also needed here then?

[pitrou]

Ok, but we don't want to keep those two unofficial FileSystem and HadoopFileSystem classes which create confusion with the other (public) filesystem classes.

Ideally, those two classes disappear and their implementation code gets folded into the public HadoopFileSystem class.

If that's too annoying, we should at least merge those two classes and give them a less ambiguous name, for example HdfsClient.

[AlenkaF]

Ok, will go with the disappearing =)
IIUC hdfs_io.h will be removed altogether:

• FileSystem and HadoopFileSystem will go into hdfs.cc, folded into the public HadoopFileSystem
• HdfsConnectionConfig will also go into hdfs.cc
• declarations that are left will go into hdfs_internal.cc

[pitrou]

Most of these declarations should IMHO go into the arrow::filesystem::internal namespace, except for HdfsConnectionConfig which can go into arrow::filesystem.

[AlenkaF]

Hi @pitrou, could you please take a quick look at the changes when you have a moment?

I've done my best to implement the suggested changes, but am sure there's still room for improvement.
A couple of issues I could use your input on:

• Some C++ tests are failing in hdfs_test, with the following error:

  /arrow/cpp/src/arrow/filesystem/hdfs_test.cc:90: HadoopFileSystem::Make failed, it is possible when we don't have proper driver on this node, err msg is IOError: Unable to load libjvm
  

  I'm not sure how best to resolve this — any guidance would be appreciated.

• The MSVC compiler is complaining about a forward-declared friend function I'm using in hdfs.cc. Do you have any advice on how to better organise this?

The Python and MATLAB test failures are not related.
Thanks in advance!

[pitrou]

Hi @AlenkaF

• Some C++ tests are failing in hdfs_test, with the following error:

I think you're misreading the output, the test is actually skipped when the driver fails unloading, which is normal:

(...)
dlopen(/usr/java/latest//lib/amd64/server/libjvm.so) failed: /usr/java/latest//lib/amd64/server/libjvm.so: cannot open shared object file: No such file or directory
/arrow/cpp/src/arrow/filesystem/hdfs_internal.cc:294 try_dlopen(libjvm_potential_paths, "libjvm")
/arrow/cpp/src/arrow/filesystem/hdfs.cc:95 ConnectLibHdfs(&driver_)
/arrow/cpp/src/arrow/filesystem/hdfs.cc:725 ptr->impl_->Init()
/arrow/cpp/src/arrow/filesystem/hdfs_test.cc:205: Skipped
Driver not loaded, skipping

https://github.com/apache/arrow/actions/runs/15109276550/job/42464862030?pr=45998#step:7:3277

The problem is in the other tests, because it seems a destructor crashes:

[ RUN ] TestHadoopFileSystem.DeleteDirContents
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
Running '/build/cpp/debug/arrow-hdfs-test' produced core dump at '/tmp/core.arrow-hdfs-test.25630', printing backtrace:
[New LWP 25630]
[New LWP 25631]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/build/cpp/debug/arrow-hdfs-test'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007fc37b23d5be in arrow::io::internal::LibHdfsShim::Disconnect () at /arrow/cpp/src/arrow/filesystem/hdfs_internal.cc:340
340	int LibHdfsShim::Disconnect(hdfsFS fs) { return this->hdfsDisconnect(fs); }
[Current thread is 1 (Thread 0x7fc374633dc0 (LWP 25630))]
(...)

https://github.com/apache/arrow/actions/runs/15109276550/job/42464862030?pr=45998#step:7:3281

[pitrou]

• The MSVC compiler is complaining about a forward-declared friend function I'm using in hdfs.cc. Do you have any advice on how to better organise this?

Hmm, rather than trying to find the exact explanation, a simple solution would be to change these functions into static methods, for example this:

ARROW_EXPORT Status MakeReadableFile(const std::string& path, int32_t buffer_size,
const io::IOContext& io_context, LibHdfsShim* driver,
hdfsFS fs, hdfsFile file,
std::shared_ptr<HdfsReadableFile>* out);

would become:

classARROW_EXPORT HdfsReadableFile : public RandomAccessFile {
public:
(...)
static Result<std::shared_ptr<HdfsReadableFile>> Make(
const std::string& path, int32_t buffer_size,
const io::IOContext& io_context, LibHdfsShim* driver,
hdfsFS fs, hdfsFile file);

[AlenkaF]

Aha, I see! Thanks, will look into it.

[AlenkaF]

@pitrou I cleaned up the CI failures (others are not related) and am hoping this changes will not be too bad to review :)

[benibus]

Thanks! This looks pretty good to me. Just a few comments.

[AlenkaF]

@benibus@pitrou would you mind taking another look?

[AlenkaF]

@pitrou gentle ping. Would I be too optimistic to try to get it into 21.0.0?

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

[AlenkaF]

PR is ready for another round of review (suggestions applied, code rebased and liners run).

[raulcd]

@AlenkaF not thoroughly reviewing, I am just trying to understand the expected architecture a little better before further review.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

cpp/src/arrow/filesystem/hdfs_internal.cc:35

• This file uses std::min and std::numeric_limits later (e.g. in the moved HDFS read/write implementations), but it doesn't include <algorithm> / <limits> directly. Relying on transitive includes is brittle and can fail to compile on some platforms/toolchains.

Add the missing standard headers explicitly.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

cpp/src/arrow/filesystem/hdfs_internal.cc:37

• hdfs_internal.cc uses std::numeric_limits and std::min, but the file doesn't include <limits> / <algorithm>. This can fail to compile depending on transitive includes and is fragile w.r.t. IWYU.

[raulcd]

Thanks for the work here @AlenkaF . This will end up with a much cleaner API!

[raulcd]

Shouldn't some of these tests be moved to arrow/filesystem/hdfs_test.cc instead of just removing the full test file? For example we are moving HdfsReadableFile from the old io/hdfs.cc to the new filesystem/hdfs_internal.cc and we are not adding new tests for it (which were part of this test file). Same with HdfsOutputStream and other functionality. I am worried we might be reducing test coverage a tad too much as I can't see those covered deeply in the existing public API. Some of those tests should apply (with some API and include modifications) quite closely.

[AlenkaF]

There has been some moving around with this test:

• first it has been moved under filesystems and named hdfs_test_io.cc‎: b0cca50#diff-65fee404eb58a58c664cbbab64f066736a723a6cb5b094d94c43f8dcb076047b
• then it has been renamed to hdfs_io_test.cc: caab0d0
• and again renamed to hdfs_internal_test.cc and updated a bit when merging internal classes here: 2bbb630#diff-f4795a70b2c29c3464b6042a8a19ae801493bff7fa711e7db0d6443c53e32dea
• at the end it has been removed in b9aab57 as per this comment: GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code #45998 (comment)

Claude suggests adding back these sections and removing the rest:

• LargeFile (regression test)
• ThreadSafety (concurrent use of the shared internal client)
• ConnectsAgain / MultipleClients (connection lifecycle regressions)

I think it is a good start.

[AlenkaF]

@raulcd I have used Claude to add the tests mentioned in above comment. I checked them and they look OK to me, the differences are only with the APIs used to set things up. 5b7f1bc

I also checked that HdfsReadableFile is tested by being used in OpenReadable which in turn is used and tested via OpenInputFile and TestHadoopFileSystemGeneric/OpenInputStream.
Similar for HdfsOutputStream which is tested via OpenWritable and TestHadoopFileSystemGeneric.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

cpp/src/arrow/filesystem/hdfs_internal.cc:37

• hdfs_internal.cc now uses std::numeric_limits and std::min (see ReadAt/Write implementations later in this file), but the required standard headers <limits> and <algorithm> are not included here. This can fail to compile depending on transitive includes.
  cpp/src/arrow/filesystem/hdfs_internal.h:43
• arrow/filesystem/hdfs_internal.h adds using internal::IOErrorFromErrno; at namespace scope. Even though this is an internal header, using declarations in headers can leak names into includers and cause ambiguous lookup / unexpected overload resolution. Prefer qualifying internal::IOErrorFromErrno (or adding a using in the .cc file) instead of exporting it from the header.