You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AlenkaF (via GitHub)" <gi...@apache.org> on 2025/04/01 10:28:25 UTC
[PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics [arrow]
AlenkaF opened a new pull request, #45998:
URL: https://github.com/apache/arrow/pull/45998
### 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.
### Are these changes tested?
Existing tests should pass.
### Are there any user-facing changes?
Deprecated structs are removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3505329080
##########
python/pyarrow/io.pxi:
##########
@@ -46,18 +46,6 @@ cdef extern from "Python.h":
char *v, Py_ssize_t len) except NULL
Review Comment:
I don't see `have_libhdfs()` as being a part of the public API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r4036438379
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -288,6 +287,12 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
void ConfigureKerberosTicketCachePath(c_string path)
void ConfigureExtraConf(c_string key, c_string value)
+ const c_string& host() const
+ int port() const
+ const c_string& user() const
+ const c_string& kerb_ticket() const
+ const unordered_map[c_string, c_string]&extra_conf() const
Review Comment:
The Cython declaration for `extra_conf()` is missing a space between the reference declarator and the method name (`]&extra_conf()`), unlike other `const T& method()` declarations in the codebase. This is likely to be parsed incorrectly by Cython (and fail to compile).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5712517690
Revision: 0625e9eae926a306e5e15e756cdf94ae7b24f0fc
Submitted crossbow builds: [ursacomputing/crossbow @ actions-0d69c54052](https://github.com/ursacomputing/crossbow/branches/all?query=actions-0d69c54052)
|Task|Status|
|----|------|
|test-conda-python-3.11-hdfs-2.10.2|[](https://github.com/ursacomputing/crossbow/actions/runs/35208304489/job/105159410351)|
|test-conda-python-3.11-hdfs-3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/35208305044/job/105159411286)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd merged PR #45998:
URL: https://github.com/apache/arrow/pull/45998
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3505098794
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -19,21 +19,34 @@
#include <memory>
#include <string>
+#include <unordered_map>
#include <vector>
#include "arrow/filesystem/filesystem.h"
-#include "arrow/io/hdfs.h"
#include "arrow/util/uri.h"
namespace arrow::fs {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
+struct HdfsPathInfo;
Review Comment:
OK, I need to re-think this - somehow I forgot there are Python bindings attached to this ๐
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3615333256
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -52,12 +49,22 @@ struct ARROW_EXPORT HdfsOptions {
static Result<HdfsOptions> FromUri(const ::arrow::util::Uri& uri);
static Result<HdfsOptions> FromUri(const std::string& uri);
+
+ const std::string& host() const { return host_; }
+ int port() const { return port_; }
+ const std::string& user() const { return user_; }
+ const std::string& kerb_ticket() const { return kerb_ticket_; }
+ std::unordered_map<std::string, std::string> extra_conf() const { return extra_conf_; }
Review Comment:
`HdfsOptions::extra_conf()` returns the `unordered_map` by value, which forces a copy on each call (e.g., `for (const auto& kv : options_.extra_conf())`). Returning a const reference would avoid needless allocations/copies and match the other accessors that return `const std::string&`.
Note this will require updating the corresponding Cython declaration in `python/pyarrow/includes/libarrow_fs.pxd` to keep the C++/Cython signatures consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3620959722
##########
cpp/src/arrow/io/hdfs_test.cc:
##########
@@ -1,466 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one
-// or more contributor license agreements. See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership. The ASF licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License. You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied. See the License for the
-// specific language governing permissions and limitations
-// under the License.
-
-#include <atomic>
-#include <cstdint>
-#include <cstdlib>
-#include <cstring>
-#include <filesystem>
-#include <iostream>
-#include <memory>
-#include <sstream> // IWYU pragma: keep
-#include <string>
-#include <thread>
-#include <vector>
-
-#include <gtest/gtest.h>
-
-#include "arrow/buffer.h"
-#include "arrow/io/hdfs.h"
-#include "arrow/io/hdfs_internal.h"
-#include "arrow/io/interfaces.h"
-#include "arrow/status.h"
-#include "arrow/testing/gtest_util.h"
-#include "arrow/testing/util.h"
-
-namespace arrow {
-namespace io {
-
-std::vector<uint8_t> RandomData(int64_t size) {
- std::vector<uint8_t> buffer(size);
- random_bytes(size, 0, buffer.data());
- return buffer;
-}
-
-class TestHadoopFileSystem : public ::testing::Test {
- public:
- Status MakeScratchDir() {
- if (client_->Exists(scratch_dir_)) {
- RETURN_NOT_OK((client_->Delete(scratch_dir_, true)));
- }
- return client_->MakeDirectory(scratch_dir_);
- }
-
- Status WriteDummyFile(const std::string& path, const uint8_t* buffer, int64_t size,
- bool append = false, int buffer_size = 0, int16_t replication = 0,
- int default_block_size = 0) {
- std::shared_ptr<HdfsOutputStream> file;
- RETURN_NOT_OK(client_->OpenWritable(path, append, buffer_size, replication,
- default_block_size, &file));
-
- RETURN_NOT_OK(file->Write(buffer, size));
- RETURN_NOT_OK(file->Close());
-
- return Status::OK();
- }
-
- std::string ScratchPath(const std::string& name) {
- std::stringstream ss;
- ss << scratch_dir_ << "/" << name;
- return ss.str();
- }
-
- std::string HdfsAbsPath(const std::string& relpath) {
- std::stringstream ss;
- ss << "hdfs://" << conf_.host << ":" << conf_.port << relpath;
- return ss.str();
- }
-
- // Set up shared state between unit tests
- void SetUp() {
- internal::LibHdfsShim* driver_shim;
-
- client_ = nullptr;
- scratch_dir_ =
- (std::filesystem::temp_directory_path() / "arrow-hdfs/scratch-").string();
- int random_size = 4;
- scratch_dir_.resize(scratch_dir_.size() + random_size, '%');
- random_alnum(
- random_size, 0,
- reinterpret_cast<uint8_t*>(&scratch_dir_[scratch_dir_.size() - random_size]));
-
- loaded_driver_ = false;
-
- Status msg = ConnectLibHdfs(&driver_shim);
- if (!msg.ok()) {
- if (std::getenv("ARROW_HDFS_TEST_LIBHDFS_REQUIRE")) {
- FAIL() << "Loading libhdfs failed: " << msg.ToString();
- } else {
- std::cout << "Loading libhdfs failed, skipping tests gracefully: "
- << msg.ToString() << std::endl;
- }
- return;
- }
-
- loaded_driver_ = true;
-
- const char* host = std::getenv("ARROW_HDFS_TEST_HOST");
- const char* port = std::getenv("ARROW_HDFS_TEST_PORT");
- const char* user = std::getenv("ARROW_HDFS_TEST_USER");
-
- ASSERT_TRUE(user != nullptr) << "Set ARROW_HDFS_TEST_USER";
-
- conf_.host = host == nullptr ? "localhost" : host;
- conf_.user = user;
- conf_.port = port == nullptr ? 20500 : atoi(port);
-
- ASSERT_OK(HadoopFileSystem::Connect(&conf_, &client_));
- }
-
- void TearDown() {
- if (client_) {
- if (client_->Exists(scratch_dir_)) {
- ARROW_EXPECT_OK(client_->Delete(scratch_dir_, true));
- }
- ARROW_EXPECT_OK(client_->Disconnect());
- }
- }
-
- HdfsConnectionConfig conf_;
- bool loaded_driver_;
-
- // Resources shared amongst unit tests
- std::string scratch_dir_;
- std::shared_ptr<HadoopFileSystem> client_;
-};
-
-#define SKIP_IF_NO_DRIVER() \
- if (!this->loaded_driver_) { \
- GTEST_SKIP() << "Driver not loaded, skipping"; \
- }
-
-TEST_F(TestHadoopFileSystem, ConnectsAgain) {
- SKIP_IF_NO_DRIVER();
-
- std::shared_ptr<HadoopFileSystem> client;
- ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client));
- ASSERT_OK(client->Disconnect());
-}
-
-TEST_F(TestHadoopFileSystem, MultipleClients) {
- SKIP_IF_NO_DRIVER();
-
- ASSERT_OK(this->MakeScratchDir());
-
- std::shared_ptr<HadoopFileSystem> client1;
- std::shared_ptr<HadoopFileSystem> client2;
- ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client1));
- ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client2));
- ASSERT_OK(client1->Disconnect());
-
- // client2 continues to function after equivalent client1 has shutdown
- std::vector<HdfsPathInfo> listing;
- ASSERT_OK(client2->ListDirectory(this->scratch_dir_, &listing));
- ASSERT_OK(client2->Disconnect());
-}
-
-TEST_F(TestHadoopFileSystem, MakeDirectory) {
- SKIP_IF_NO_DRIVER();
-
- std::string path = this->ScratchPath("create-directory");
-
- if (this->client_->Exists(path)) {
- ASSERT_OK(this->client_->Delete(path, true));
- }
-
- ASSERT_OK(this->client_->MakeDirectory(path));
- ASSERT_TRUE(this->client_->Exists(path));
- std::vector<HdfsPathInfo> listing;
- ARROW_EXPECT_OK(this->client_->ListDirectory(path, &listing));
- ASSERT_EQ(0, listing.size());
- ARROW_EXPECT_OK(this->client_->Delete(path, true));
- ASSERT_FALSE(this->client_->Exists(path));
- ASSERT_RAISES(IOError, this->client_->ListDirectory(path, &listing));
-}
-
-TEST_F(TestHadoopFileSystem, GetCapacityUsed) {
- SKIP_IF_NO_DRIVER();
-
- // Who knows what is actually in your DFS cluster, but expect it to have
- // positive used bytes and capacity
- int64_t nbytes = 0;
- ASSERT_OK(this->client_->GetCapacity(&nbytes));
- ASSERT_LT(0, nbytes);
-
- ASSERT_OK(this->client_->GetUsed(&nbytes));
- ASSERT_LT(0, nbytes);
-}
-
-TEST_F(TestHadoopFileSystem, GetPathInfo) {
- SKIP_IF_NO_DRIVER();
-
- HdfsPathInfo info;
-
- ASSERT_OK(this->MakeScratchDir());
-
- // Directory info
- ASSERT_OK(this->client_->GetPathInfo(this->scratch_dir_, &info));
- ASSERT_EQ(ObjectType::DIRECTORY, info.kind);
- ASSERT_EQ(this->HdfsAbsPath(this->scratch_dir_), info.name);
- ASSERT_EQ(this->conf_.user, info.owner);
-
- // TODO(wesm): test group, other attrs
-
- auto path = this->ScratchPath("test-file");
-
- const int size = 100;
-
- std::vector<uint8_t> buffer = RandomData(size);
-
- ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size));
- ASSERT_OK(this->client_->GetPathInfo(path, &info));
-
- ASSERT_EQ(ObjectType::FILE, info.kind);
- ASSERT_EQ(this->HdfsAbsPath(path), info.name);
- ASSERT_EQ(this->conf_.user, info.owner);
- ASSERT_EQ(size, info.size);
-}
-
-TEST_F(TestHadoopFileSystem, GetPathInfoNotExist) {
- // ARROW-2919: Test that the error message is reasonable
- SKIP_IF_NO_DRIVER();
-
- ASSERT_OK(this->MakeScratchDir());
- auto path = this->ScratchPath("path-does-not-exist");
-
- HdfsPathInfo info;
- Status s = this->client_->GetPathInfo(path, &info);
- ASSERT_TRUE(s.IsIOError());
-
- const std::string error_message = s.ToString();
-
- // Check that the file path is found in the error message
- ASSERT_LT(error_message.find(path), std::string::npos);
-}
-
-TEST_F(TestHadoopFileSystem, AppendToFile) {
- SKIP_IF_NO_DRIVER();
-
- ASSERT_OK(this->MakeScratchDir());
-
- auto path = this->ScratchPath("test-file");
- const int size = 100;
-
- std::vector<uint8_t> buffer = RandomData(size);
- ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size));
-
- // now append
- ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size, true));
-
- HdfsPathInfo info;
- ASSERT_OK(this->client_->GetPathInfo(path, &info));
- ASSERT_EQ(size * 2, info.size);
-}
-
-TEST_F(TestHadoopFileSystem, ListDirectory) {
- SKIP_IF_NO_DRIVER();
-
- const int size = 100;
- std::vector<uint8_t> data = RandomData(size);
-
- auto p1 = this->ScratchPath("test-file-1");
- auto p2 = this->ScratchPath("test-file-2");
- auto d1 = this->ScratchPath("test-dir-1");
-
- ASSERT_OK(this->MakeScratchDir());
- ASSERT_OK(this->WriteDummyFile(p1, data.data(), size));
- ASSERT_OK(this->WriteDummyFile(p2, data.data(), size / 2));
- ASSERT_OK(this->client_->MakeDirectory(d1));
-
- std::vector<HdfsPathInfo> listing;
- ASSERT_OK(this->client_->ListDirectory(this->scratch_dir_, &listing));
-
- // Do it again, appends!
- ASSERT_OK(this->client_->ListDirectory(this->scratch_dir_, &listing));
-
- ASSERT_EQ(6, static_cast<int>(listing.size()));
-
- // Argh, well, shouldn't expect the listing to be in any particular order
- for (size_t i = 0; i < listing.size(); ++i) {
- const HdfsPathInfo& info = listing[i];
- if (info.name == this->HdfsAbsPath(p1)) {
- ASSERT_EQ(ObjectType::FILE, info.kind);
- ASSERT_EQ(size, info.size);
- } else if (info.name == this->HdfsAbsPath(p2)) {
- ASSERT_EQ(ObjectType::FILE, info.kind);
- ASSERT_EQ(size / 2, info.size);
- } else if (info.name == this->HdfsAbsPath(d1)) {
- ASSERT_EQ(ObjectType::DIRECTORY, info.kind);
- } else {
- FAIL() << "Unexpected path: " << info.name;
- }
- }
-}
-
-TEST_F(TestHadoopFileSystem, ReadableMethods) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5715612029
OK, this is ready. CI and `test-conda-python-3.11-hdfs-*` are green.
Last Copilot's comment is:
1. dealing with unrelated changes (can do a follow-up)
2. wrong: `GetPathInfoFailed` is used only in the files where it is defined. Also, the code compiled so no mistake here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r4035446821
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -114,4 +121,6 @@ class ARROW_EXPORT HadoopFileSystem : public FileSystem {
std::unique_ptr<Impl> impl_;
};
+ARROW_EXPORT Status HaveLibHdfs();
Review Comment:
Entries removed in https://github.com/apache/arrow/pull/45998/changes/0625e9eae926a306e5e15e756cdf94ae7b24f0fc as the tests now live under filesystems and are tested within that module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r2073379635
##########
cpp/src/arrow/filesystem/hdfs_io.h:
##########
@@ -38,18 +40,6 @@ namespace io {
class HdfsReadableFile;
class HdfsOutputStream;
-/// DEPRECATED. Use the FileSystem API in arrow::fs instead.
-struct ObjectType {
- enum type { FILE, DIRECTORY };
-};
-
-/// DEPRECATED. Use the FileSystem API in arrow::fs instead.
-struct ARROW_EXPORT FileStatistics {
- /// Size of file, -1 if finding length is unsupported
- int64_t size;
- ObjectType::type kind;
-};
-
class ARROW_EXPORT FileSystem {
Review Comment:
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r2073351700
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -833,7 +830,11 @@ if(ARROW_FILESYSTEM)
PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
endif()
if(ARROW_HDFS)
- list(APPEND ARROW_FILESYSTEM_SRCS filesystem/hdfs.cc)
+ list(APPEND
+ ARROW_FILESYSTEM_SRCS
+ filesystem/hdfs.cc
+ filesystem/hdfs_io.cc
+ filesystem/hdfs_internal.cc)
Review Comment:
Ah, it is there already (that explains why nothing failed =) )
https://github.com/apache/arrow/blob/53ef438333efd4a6eb1421b3e812d10e5a8ed633/cpp/src/arrow/CMakeLists.txt#L857-L861
Not sure if the line with `CMAKE_DL_LIBS` is also needed here then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "benibus (via GitHub)" <gi...@apache.org>.
benibus commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r2136802611
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -22,18 +22,32 @@
#include <vector>
#include "arrow/filesystem/filesystem.h"
-#include "arrow/io/hdfs.h"
#include "arrow/util/uri.h"
+namespace arrow::io::internal {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
+struct HdfsPathInfo;
+} // namespace arrow::io::internal
Review Comment:
Here as well.
##########
cpp/src/arrow/filesystem/hdfs_internal.h:
##########
@@ -33,8 +38,13 @@ namespace arrow {
class Status;
+using internal::IOErrorFromErrno;
+
namespace io::internal {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
Review Comment:
Why are we using the `io::internal` namespace here instead of `fs::internal`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3505060158
##########
python/pyarrow/_hdfs.pyx:
##########
@@ -23,9 +23,22 @@ from pyarrow.includes.libarrow_fs cimport *
from pyarrow._fs cimport FileSystem
from pyarrow.lib import frombytes, tobytes
+from pyarrow.lib cimport check_status
from pyarrow.util import _stringify_path
+def _have_libhdfs():
+ """
+ Return true if HDFS (HadoopFileSystem) library is set up correctly.
+ """
+ try:
+ with nogil:
+ check_status(HaveLibHdfs())
+ return True
+ except Exception:
+ return False
Review Comment:
`HdfsOptions` in C++ no longer has a `connection_config` field (it now stores host/port/user/kerb_ticket/extra_conf as private members with accessors), but the Python bindings still model and use `opts.connection_config.*` (e.g. in `HadoopFileSystem.__reduce__`). This will fail to compile (and/or will be incorrect ABI-wise) once the C++ refactor lands. Update the bindings to use the new `HdfsOptions` accessors (`host()`, `port()`, `user()`, `kerb_ticket()`, `extra_conf()`) and adjust the `.pxd` accordingly (remove `HdfsConnectionConfig` / `connection_config`).
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -311,6 +311,14 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
const CIOContext& io_context,
int64_t chunk_size, c_bool use_threads)
+ CStatus HaveLibHdfs()
+
+ cdef cppclass HdfsConnectionConfig:
+ c_string host
+ int port
+ c_string user
+ c_string kerb_ticket
+ unordered_map[c_string, c_string] extra_conf
Review Comment:
The Cython declarations for `arrow::fs::HdfsOptions` still expose a `connection_config` member and add a new `HdfsConnectionConfig` type, but `arrow::fs::HdfsOptions` no longer has that member/type (it now uses private fields with accessors). This mismatch breaks the generated C++ when code accesses `opts.connection_config` (see `_hdfs.pyx.__reduce__`) and should be updated to match the new C++ header (`cpp/src/arrow/filesystem/hdfs.h`).
##########
python/pyarrow/io.pxi:
##########
@@ -46,18 +46,6 @@ cdef extern from "Python.h":
char *v, Py_ssize_t len) except NULL
Review Comment:
`pyarrow.io.have_libhdfs()` was removed, but `pyarrow.have_libhdfs()` is still provided. If `pyarrow.io.have_libhdfs` is part of the public Python API, this is a backwards-incompatible removal; consider keeping a thin wrapper in `pyarrow.io` that delegates to `pyarrow.have_libhdfs()` (or to `_hdfs._have_libhdfs`) so existing user code keeps working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3527532841
##########
cpp/src/arrow/filesystem/hdfs.cc:
##########
@@ -39,28 +38,114 @@ using util::Uri;
namespace fs {
+#define CHECK_FAILURE(RETURN_VALUE, WHAT) \
+ do { \
+ if (RETURN_VALUE == -1) { \
+ return IOErrorFromErrno(errno, "HDFS ", WHAT, " failed"); \
+ } \
+ } while (0)
+
+static constexpr int kDefaultHdfsBufferSize = 1 << 16;
Review Comment:
this seems to be copied from `cpp/src/arrow/io/hdfs.cc`
Do we require code on both filesystem and io? Should we have a common place instead of repeating code?Reading the description of the PR I thought we were completely removing `cpp/src/arrow/io/hdfs.cc` but the file seems to be there. What is the code that should land here on the other?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2890868468
Aha, I see! Thanks, will look into it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r2073309002
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -300,6 +300,80 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
const CIOContext& io_context,
int64_t chunk_size, c_bool use_threads)
+cdef extern from "arrow/filesystem/hdfs_io.h" namespace "arrow::io" nogil:
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3614957671
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -288,6 +287,12 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
void ConfigureKerberosTicketCachePath(c_string path)
void ConfigureExtraConf(c_string key, c_string value)
+ const c_string& host()
+ int port()
+ const c_string& user()
+ const c_string& kerb_ticket()
+ unordered_map[c_string, c_string] extra_conf()
+
cdef cppclass CHadoopFileSystem "arrow::fs::HadoopFileSystem"(CFileSystem):
@staticmethod
CResult[shared_ptr[CHadoopFileSystem]] Make(
Review Comment:
The Cython pxd declares `HadoopFileSystem.options()` as a non-`const` method, but in C++ it's declared `HdfsOptions options() const` (see `cpp/src/arrow/filesystem/hdfs.h`). This can lead to undefined references / wrong mangled symbol when `_hdfs.pyx` calls `self.hdfs.options()`.
Update the pxd signature to include `const`.
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -288,6 +287,12 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
void ConfigureKerberosTicketCachePath(c_string path)
void ConfigureExtraConf(c_string key, c_string value)
+ const c_string& host()
+ int port()
+ const c_string& user()
+ const c_string& kerb_ticket()
+ unordered_map[c_string, c_string] extra_conf()
Review Comment:
The `HdfsOptions` getters are declared as non-`const` methods in the Cython pxd, but in C++ they are `...() const` (see `cpp/src/arrow/filesystem/hdfs.h`). This will generate bindings that reference the wrong mangled symbols (and can fail to link) when `_hdfs.pyx` calls these methods.
Declare them as `const` methods in the pxd to match the C++ API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3620226361
##########
cpp/src/arrow/filesystem/hdfs.cc:
##########
@@ -39,28 +38,114 @@ using util::Uri;
namespace fs {
+#define CHECK_FAILURE(RETURN_VALUE, WHAT) \
+ do { \
+ if (RETURN_VALUE == -1) { \
+ return IOErrorFromErrno(errno, "HDFS ", WHAT, " failed"); \
+ } \
+ } while (0)
+
+static constexpr int kDefaultHdfsBufferSize = 1 << 16;
Review Comment:
Should be ready for another round of review ๐
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5681399485
OK, there are still some parts I need to fix. Will ping once I am done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5680678674
Revision: 053dcf8489e5f932a8ef0d20c07061f8654ad62c
Submitted crossbow builds: [ursacomputing/crossbow @ actions-8fa7df5117](https://github.com/ursacomputing/crossbow/branches/all?query=actions-8fa7df5117)
|Task|Status|
|----|------|
|test-conda-python-3.11-hdfs-2.10.2|[](https://github.com/ursacomputing/crossbow/actions/runs/34972896814/job/104393263628)|
|test-conda-python-3.11-hdfs-3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/34972896536/job/104393262014)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2958911523
@benibus @pitrou would you mind taking another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3374356693
##########
cpp/src/arrow/filesystem/hdfs.cc:
##########
@@ -186,24 +292,102 @@ class HadoopFileSystem::Impl {
"': parent is not a directory");
}
}
- RETURN_NOT_OK(client_->MakeDirectory(path));
+ RETURN_NOT_OK(MakeDirectory(path));
+ return Status::OK();
+ }
+
+ Status MakeDirectory(const std::string& path) {
+ if (IsDirectory(path)) {
+ return Status::OK();
+ }
+ RETURN_NOT_OK(MakeDirectory(path));
+ return Status::OK();
+ }
Review Comment:
Nice find! ๐
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-4750069655
@raulcd @pitrou would you mind having a look at this refactor? Copilot run couple of reviews and actually found one bug =)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r4015996069
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -1016,7 +1008,7 @@ if(ARROW_FILESYSTEM)
endif()
endif()
if(ARROW_HDFS)
- list(APPEND ARROW_FILESYSTEM_SRCS filesystem/hdfs.cc)
+ list(APPEND ARROW_FILESYSTEM_SRCS filesystem/hdfs.cc filesystem/hdfs_internal.cc)
Review Comment:
The old HDFS object-library setup linked `${CMAKE_DL_LIBS}` for the dynamic libhdfs shim, but this move only adds `arrow::hadoop` to the filesystem target. On platforms where `${CMAKE_DL_LIBS}` is `dl`, the new filesystem target no longer carries the required dynamic-loader dependency used by `hdfs_internal.cc`, causing link failures or an unrecorded runtime dependency.
##########
cpp/src/arrow/meson.build:
##########
@@ -484,7 +482,10 @@ disable Arrow gcs support with -Dgcs=disabled.
endif
if needs_hdfs
- arrow_filesystem_srcs += ['filesystem/hdfs.cc']
+ arrow_filesystem_srcs += [
+ 'filesystem/hdfs.cc',
+ 'filesystem/hdfs_internal.cc',
+ ]
Review Comment:
This move leaves the Meson and HDFS integration references to the deleted io test untouched: `cpp/src/arrow/io/meson.build` still installs `io/hdfs.h` and configures the missing `io/hdfs_test.cc`, while `ci/scripts/integration_hdfs.sh` still invokes the removed `arrow-io-hdfs-test` binary. With HDFS enabled, Meson configuration or the integration job will fail; update these references to the filesystem header/`arrow-hdfs-test` (or remove them).
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -114,4 +121,6 @@ class ARROW_EXPORT HadoopFileSystem : public FileSystem {
std::unique_ptr<Impl> impl_;
};
+ARROW_EXPORT Status HaveLibHdfs();
Review Comment:
Moving HDFS out of `arrow/io` leaves Meson references to the deleted files in `cpp/src/arrow/io/meson.build`: it still installs `hdfs.h` and, when HDFS is enabled, builds `hdfs_test.cc`. Meson configuration/builds will therefore fail with missing paths; remove those entries now that the header and test live under `arrow/filesystem`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5680623668
@github-actions crossbow submit test-*hdfs*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-4854718443
PR is ready for another round of review (suggestions applied, code rebased and liners run).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3395409865
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -19,21 +19,36 @@
#include <memory>
#include <string>
+#include <unordered_map>
#include <vector>
#include "arrow/filesystem/filesystem.h"
-#include "arrow/io/hdfs.h"
#include "arrow/util/uri.h"
namespace arrow::fs {
+namespace internal {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
+struct HdfsPathInfo;
+} // namespace internal
+
+struct HdfsConnectionConfig {
+ std::string host;
+ int port;
+ std::string user;
+ std::string kerb_ticket;
+ std::unordered_map<std::string, std::string> extra_conf;
+};
Review Comment:
Keeping the code as is as this has been defined before the refactor (uninitialized).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r2137369780
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -301,6 +301,19 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
const CIOContext& io_context,
int64_t chunk_size, c_bool use_threads)
+ CStatus HaveLibHdfs()
+
+ cdef enum HdfsDriver" arrow::io::HdfsDriver":
+ HdfsDriver_LIBHDFS" arrow::io::HdfsDriver::LIBHDFS"
+ HdfsDriver_LIBHDFS3" arrow::io::HdfsDriver::LIBHDFS3"
Review Comment:
I need to remove `HdfsDriver` as it is a leftover from: https://github.com/apache/arrow/commit/4e53749097ba687afd5e000067925def2e2802c9#diff-8f9209d74ff1ddfc6fed20ab199b6d158d0052382fae2c7bcef9f0bd96cebc95
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2890402955
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2772286327
OK, I will then move `io/hdfs` to `filesystem/hdfs_internal`. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2769271498
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? (https://github.com/apache/arrow/issues/22457)
cc @pitrou
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3481929838
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -19,21 +19,34 @@
#include <memory>
#include <string>
+#include <unordered_map>
#include <vector>
#include "arrow/filesystem/filesystem.h"
-#include "arrow/io/hdfs.h"
#include "arrow/util/uri.h"
namespace arrow::fs {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
+struct HdfsPathInfo;
Review Comment:
Correct ๐ฌ Will move to internal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3504941125
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -19,21 +19,34 @@
#include <memory>
#include <string>
+#include <unordered_map>
#include <vector>
#include "arrow/filesystem/filesystem.h"
-#include "arrow/io/hdfs.h"
#include "arrow/util/uri.h"
namespace arrow::fs {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
+struct HdfsPathInfo;
Review Comment:
I have decided to remove the `HdfsConnectionConfig` struct altogether and add members directly to the `HdfsOptions` as private fields. Does that sound reasonable to you @pitrou?
Commit: https://github.com/apache/arrow/pull/45998/changes/b10f15974f68561dd1825d83dfa57ec24890a432
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5678841715
@github-actions crossbow submit test-*hdfs*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5713744231
I added a fix for the 3rd item in [last comment from Copilot](https://github.com/apache/arrow/pull/45998#pullrequestreview-5233980957), connected to `extra_conf()`, here: https://github.com/apache/arrow/pull/45998/changes/2741e3f26a593a05c13d5315a5f625d6fa596a3b
(1st and 2nd items seem like they are dealing with an existing behavior and can be fixed as a follow-up in a separate PR)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2890868302
> * 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:
```c++
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:
```c++
class ARROW_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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2772076541
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r2073347504
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -833,7 +830,11 @@ if(ARROW_FILESYSTEM)
PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
endif()
if(ARROW_HDFS)
- list(APPEND ARROW_FILESYSTEM_SRCS filesystem/hdfs.cc)
+ list(APPEND
+ ARROW_FILESYSTEM_SRCS
+ filesystem/hdfs.cc
+ filesystem/hdfs_io.cc
+ filesystem/hdfs_internal.cc)
Review Comment:
Hm, yeah. I will add a link as above as it makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3374394620
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -19,21 +19,36 @@
#include <memory>
#include <string>
+#include <unordered_map>
#include <vector>
#include "arrow/filesystem/filesystem.h"
-#include "arrow/io/hdfs.h"
#include "arrow/util/uri.h"
namespace arrow::fs {
+namespace internal {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
+struct HdfsPathInfo;
+} // namespace internal
+
+struct HdfsConnectionConfig {
+ std::string host;
+ int port;
+ std::string user;
+ std::string kerb_ticket;
+ std::unordered_map<std::string, std::string> extra_conf;
+};
Review Comment:
`HdfsConnectionConfig::port` is currently left uninitialized by default construction, so `HdfsOptions options;` followed by `HadoopFileSystem::Make(options, ...)` can pass an indeterminate port into `BuilderSetNameNodePort`, which is undefined behavior. Initialize the port member to a known default (0 is already treated as "default port" in `HdfsOptions::FromUri`).
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -88,10 +100,49 @@ class ARROW_EXPORT HadoopFileSystem : public FileSystem {
Status DeleteFile(const std::string& path) override;
+ Status MakeDirectory(const std::string& path);
+
+ bool Exists(const std::string& path);
+
+ Status GetPathInfoStatus(const std::string& path, internal::HdfsPathInfo* info);
+
+ Status ListDirectory(const std::string& path,
+ std::vector<internal::HdfsPathInfo>* listing);
Review Comment:
Several new public `HadoopFileSystem` methods expose `arrow::fs::internal` types in their signatures (e.g. `internal::HdfsPathInfo`, `internal::HdfsReadableFile`, `internal::HdfsOutputStream`). Since this header is part of the public filesystem API, this effectively makes those `internal` types part of the public ABI/API surface and constrains future refactors. Consider either (a) making these types public (not in an `internal` namespace) if they are intended for external consumption, or (b) keeping these helpers private/hidden and exposing equivalent functionality via existing `FileSystem` APIs and `FileInfo`/streams.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3614798484
##########
cpp/src/arrow/filesystem/hdfs.cc:
##########
@@ -39,28 +38,114 @@ using util::Uri;
namespace fs {
+#define CHECK_FAILURE(RETURN_VALUE, WHAT) \
+ do { \
+ if (RETURN_VALUE == -1) { \
+ return IOErrorFromErrno(errno, "HDFS ", WHAT, " failed"); \
+ } \
+ } while (0)
+
+static constexpr int kDefaultHdfsBufferSize = 1 << 16;
Review Comment:
Oh, thank you @raulcd for this comment! I had to spend some time thinking about why I would left `cpp/src/arrow/io/hdfs.cc` with such a small change, as it doesn't make sense. I came to the conclusion this is most probably an unfortunate leftover from a, quite painful, merge conflict I had here 3 weeks ago. I wasn't sure if I missed something there. So I guess I did.
I will remove the file, look for any other possible issues with Claude, and check if the CI passes.
Thanks again!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3479964258
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -88,10 +98,48 @@ class ARROW_EXPORT HadoopFileSystem : public FileSystem {
Status DeleteFile(const std::string& path) override;
+ Status MakeDirectory(const std::string& path);
+
+ bool Exists(const std::string& path);
+
+ Status GetPathInfoStatus(const std::string& path, HdfsPathInfo* info);
+
+ Status ListDirectory(const std::string& path, std::vector<HdfsPathInfo>* listing);
Review Comment:
`HdfsPathInfo` is only forward-declared, but itโs used inside `std::vector<HdfsPathInfo>` in the public `HadoopFileSystem::ListDirectory` signature. Instantiating `std::vector<T>` requires `T` to be complete at the point of declaration, so this header will not compile for downstream users. Either move the `HdfsPathInfo` definition into this public header (and remove the duplicate definition from `hdfs_internal.h`), or change these APIs to use an already-public type (e.g. `FileInfo`) and keep `HdfsPathInfo` internal.
##########
cpp/src/arrow/filesystem/hdfs.cc:
##########
@@ -39,28 +38,115 @@ using util::Uri;
namespace fs {
+#define CHECK_FAILURE(RETURN_VALUE, WHAT) \
+ do { \
+ if (RETURN_VALUE == -1) { \
+ return IOErrorFromErrno(errno, "HDFS ", WHAT, " failed"); \
+ } \
+ } while (0)
+
+static constexpr int kDefaultHdfsBufferSize = 1 << 16;
+
using internal::GetAbstractPathParent;
using internal::MakeAbstractPathRelative;
using internal::RemoveLeadingSlash;
+// ----------------------------------------------------------------------
+// HDFS client
+
+// TODO(wesm): this could throw std::bad_alloc in the course of copying strings
+// into the path info object
+static void SetPathInfo(const hdfsFileInfo* input, HdfsPathInfo* out) {
+ out->kind = input->mKind == kObjectKindFile ? arrow::fs::FileType::File
+ : arrow::fs::FileType::Directory;
+ out->name = std::string(input->mName);
+ out->owner = std::string(input->mOwner);
+ out->group = std::string(input->mGroup);
+
+ out->last_access_time = static_cast<int32_t>(input->mLastAccess);
+ out->last_modified_time = static_cast<int32_t>(input->mLastMod);
+ out->size = static_cast<int64_t>(input->mSize);
+
+ out->replication = input->mReplication;
+ out->block_size = input->mBlockSize;
+
+ out->permissions = input->mPermissions;
+}
+
+namespace {
+
+Status GetPathInfoFailed(const std::string& path) {
+ return IOErrorFromErrno(errno, "Calling GetPathInfo for '", path, "' failed");
+}
+
+} // namespace
+
class HadoopFileSystem::Impl {
public:
Impl(HdfsOptions options, const io::IOContext& io_context)
- : options_(std::move(options)), io_context_(io_context) {}
+ : options_(std::move(options)),
+ io_context_(io_context),
+ driver_(NULLPTR),
+ port_(0),
+ client_(NULLPTR) {}
~Impl() { ARROW_WARN_NOT_OK(Close(), "Failed to disconnect hdfs client"); }
Status Init() {
- io::internal::LibHdfsShim* driver_shim;
- RETURN_NOT_OK(ConnectLibHdfs(&driver_shim));
- RETURN_NOT_OK(io::HadoopFileSystem::Connect(&options_.connection_config, &client_));
+ const HdfsConnectionConfig* config = &options_.connection_config;
+ RETURN_NOT_OK(ConnectLibHdfs(&driver_));
Review Comment:
`ConnectLibHdfs()` is declared in `arrow::fs::internal` (see `arrow/filesystem/hdfs_internal.h`), but here it is called unqualified. This wonโt compile because unqualified lookup in `arrow::fs` wonโt find names in the nested `arrow::fs::internal` namespace.
##########
python/pyarrow/io.pxi:
##########
@@ -32,7 +32,7 @@ import warnings
from io import BufferedIOBase, IOBase, TextIOBase, UnsupportedOperation
from queue import Queue, Empty as QueueEmpty
-from pyarrow.lib cimport check_status, HaveLibHdfs
+from pyarrow.lib cimport check_status
Review Comment:
This removes `pyarrow.io.have_libhdfs()`, which was previously part of the `pyarrow.io` public surface. Even though `pyarrow.have_libhdfs()` is now provided, existing code importing `have_libhdfs` from `pyarrow.io` will break. Consider keeping a small compatibility wrapper in `pyarrow.io` (optionally with a deprecation warning) that forwards to the new location.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5678814239
@github-actions crossbow submit test-conda-python-3.11-hdfs-*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5712467350
```
Unable to match any tasks for `test-hdfs`
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/35207971116
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2890860924
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2789399921
@pitrou I think this is ready for review. The failing builds have an issue opened: https://github.com/apache/arrow/issues/46077
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r2065856352
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -408,15 +408,12 @@ arrow_add_object_library(ARROW_IO
io/caching.cc
io/compressed.cc
io/file.cc
- io/hdfs.cc
- io/hdfs_internal.cc
io/interfaces.cc
io/memory.cc
io/slow.cc
io/stdio.cc
io/transform.cc)
foreach(ARROW_IO_TARGET ${ARROW_IO_TARGETS})
- target_link_libraries(${ARROW_IO_TARGET} PRIVATE arrow::hadoop)
if(NOT MSVC)
target_link_libraries(${ARROW_IO_TARGET} PRIVATE ${CMAKE_DL_LIBS})
endif()
Review Comment:
I think `CMAKE_DL_LIBS` was for HDFS, you need to move it as well?
##########
cpp/src/arrow/filesystem/CMakeLists.txt:
##########
@@ -143,4 +143,12 @@ endif()
if(ARROW_HDFS)
add_arrow_test(hdfs_test EXTRA_LABELS filesystem)
+ add_arrow_test(hdfs_test_io
Review Comment:
Nit: should call this `hdfs_io_test`, or perhaps better `hdfs_internals_test`
##########
cpp/src/arrow/filesystem/hdfs_io.h:
##########
@@ -38,18 +40,6 @@ namespace io {
class HdfsReadableFile;
Review Comment:
Most of these declarations should IMHO go into the `arrow::filesystem::internal` namespace, except for `HdfsConnectionConfig` which can go into `arrow::filesystem`.
##########
cpp/src/arrow/filesystem/api.h:
##########
@@ -32,3 +32,6 @@
#ifdef ARROW_S3
# include "arrow/filesystem/s3fs.h" // IWYU pragma: export
#endif
+#ifdef ARROW_HDFS
+# include "arrow/filesystem/hdfs_io.h" // IWYU pragma: export
Review Comment:
I don't think we need to include this one here. (also, it's included implicitly by `hdfs.h`)
##########
cpp/src/arrow/filesystem/hdfs_io.h:
##########
@@ -38,18 +40,6 @@ namespace io {
class HdfsReadableFile;
class HdfsOutputStream;
-/// DEPRECATED. Use the FileSystem API in arrow::fs instead.
-struct ObjectType {
- enum type { FILE, DIRECTORY };
-};
-
-/// DEPRECATED. Use the FileSystem API in arrow::fs instead.
-struct ARROW_EXPORT FileStatistics {
- /// Size of file, -1 if finding length is unsupported
- int64_t size;
- ObjectType::type kind;
-};
-
class ARROW_EXPORT FileSystem {
Review Comment:
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`.
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -833,7 +830,11 @@ if(ARROW_FILESYSTEM)
PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
endif()
if(ARROW_HDFS)
- list(APPEND ARROW_FILESYSTEM_SRCS filesystem/hdfs.cc)
+ list(APPEND
+ ARROW_FILESYSTEM_SRCS
+ filesystem/hdfs.cc
+ filesystem/hdfs_io.cc
+ filesystem/hdfs_internal.cc)
Review Comment:
Don't we need to link to `arrow::hadoop` as was done above? cc @kou for advice
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -300,6 +300,80 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
const CIOContext& io_context,
int64_t chunk_size, c_bool use_threads)
+cdef extern from "arrow/filesystem/hdfs_io.h" namespace "arrow::io" nogil:
Review Comment:
Are all these declarations actually needed by PyArrow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3479726977
##########
cpp/src/arrow/filesystem/hdfs_internal_test.cc:
##########
Review Comment:
Yeah, very true =) Will remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3783648338
##########
cpp/src/arrow/io/hdfs_test.cc:
##########
@@ -1,466 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one
-// or more contributor license agreements. See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership. The ASF licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License. You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied. See the License for the
-// specific language governing permissions and limitations
-// under the License.
-
-#include <atomic>
-#include <cstdint>
-#include <cstdlib>
-#include <cstring>
-#include <filesystem>
-#include <iostream>
-#include <memory>
-#include <sstream> // IWYU pragma: keep
-#include <string>
-#include <thread>
-#include <vector>
-
-#include <gtest/gtest.h>
-
-#include "arrow/buffer.h"
-#include "arrow/io/hdfs.h"
-#include "arrow/io/hdfs_internal.h"
-#include "arrow/io/interfaces.h"
-#include "arrow/status.h"
-#include "arrow/testing/gtest_util.h"
-#include "arrow/testing/util.h"
-
-namespace arrow {
-namespace io {
-
-std::vector<uint8_t> RandomData(int64_t size) {
- std::vector<uint8_t> buffer(size);
- random_bytes(size, 0, buffer.data());
- return buffer;
-}
-
-class TestHadoopFileSystem : public ::testing::Test {
- public:
- Status MakeScratchDir() {
- if (client_->Exists(scratch_dir_)) {
- RETURN_NOT_OK((client_->Delete(scratch_dir_, true)));
- }
- return client_->MakeDirectory(scratch_dir_);
- }
-
- Status WriteDummyFile(const std::string& path, const uint8_t* buffer, int64_t size,
- bool append = false, int buffer_size = 0, int16_t replication = 0,
- int default_block_size = 0) {
- std::shared_ptr<HdfsOutputStream> file;
- RETURN_NOT_OK(client_->OpenWritable(path, append, buffer_size, replication,
- default_block_size, &file));
-
- RETURN_NOT_OK(file->Write(buffer, size));
- RETURN_NOT_OK(file->Close());
-
- return Status::OK();
- }
-
- std::string ScratchPath(const std::string& name) {
- std::stringstream ss;
- ss << scratch_dir_ << "/" << name;
- return ss.str();
- }
-
- std::string HdfsAbsPath(const std::string& relpath) {
- std::stringstream ss;
- ss << "hdfs://" << conf_.host << ":" << conf_.port << relpath;
- return ss.str();
- }
-
- // Set up shared state between unit tests
- void SetUp() {
- internal::LibHdfsShim* driver_shim;
-
- client_ = nullptr;
- scratch_dir_ =
- (std::filesystem::temp_directory_path() / "arrow-hdfs/scratch-").string();
- int random_size = 4;
- scratch_dir_.resize(scratch_dir_.size() + random_size, '%');
- random_alnum(
- random_size, 0,
- reinterpret_cast<uint8_t*>(&scratch_dir_[scratch_dir_.size() - random_size]));
-
- loaded_driver_ = false;
-
- Status msg = ConnectLibHdfs(&driver_shim);
- if (!msg.ok()) {
- if (std::getenv("ARROW_HDFS_TEST_LIBHDFS_REQUIRE")) {
- FAIL() << "Loading libhdfs failed: " << msg.ToString();
- } else {
- std::cout << "Loading libhdfs failed, skipping tests gracefully: "
- << msg.ToString() << std::endl;
- }
- return;
- }
-
- loaded_driver_ = true;
-
- const char* host = std::getenv("ARROW_HDFS_TEST_HOST");
- const char* port = std::getenv("ARROW_HDFS_TEST_PORT");
- const char* user = std::getenv("ARROW_HDFS_TEST_USER");
-
- ASSERT_TRUE(user != nullptr) << "Set ARROW_HDFS_TEST_USER";
-
- conf_.host = host == nullptr ? "localhost" : host;
- conf_.user = user;
- conf_.port = port == nullptr ? 20500 : atoi(port);
-
- ASSERT_OK(HadoopFileSystem::Connect(&conf_, &client_));
- }
-
- void TearDown() {
- if (client_) {
- if (client_->Exists(scratch_dir_)) {
- ARROW_EXPECT_OK(client_->Delete(scratch_dir_, true));
- }
- ARROW_EXPECT_OK(client_->Disconnect());
- }
- }
-
- HdfsConnectionConfig conf_;
- bool loaded_driver_;
-
- // Resources shared amongst unit tests
- std::string scratch_dir_;
- std::shared_ptr<HadoopFileSystem> client_;
-};
-
-#define SKIP_IF_NO_DRIVER() \
- if (!this->loaded_driver_) { \
- GTEST_SKIP() << "Driver not loaded, skipping"; \
- }
-
-TEST_F(TestHadoopFileSystem, ConnectsAgain) {
- SKIP_IF_NO_DRIVER();
-
- std::shared_ptr<HadoopFileSystem> client;
- ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client));
- ASSERT_OK(client->Disconnect());
-}
-
-TEST_F(TestHadoopFileSystem, MultipleClients) {
- SKIP_IF_NO_DRIVER();
-
- ASSERT_OK(this->MakeScratchDir());
-
- std::shared_ptr<HadoopFileSystem> client1;
- std::shared_ptr<HadoopFileSystem> client2;
- ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client1));
- ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client2));
- ASSERT_OK(client1->Disconnect());
-
- // client2 continues to function after equivalent client1 has shutdown
- std::vector<HdfsPathInfo> listing;
- ASSERT_OK(client2->ListDirectory(this->scratch_dir_, &listing));
- ASSERT_OK(client2->Disconnect());
-}
-
-TEST_F(TestHadoopFileSystem, MakeDirectory) {
- SKIP_IF_NO_DRIVER();
-
- std::string path = this->ScratchPath("create-directory");
-
- if (this->client_->Exists(path)) {
- ASSERT_OK(this->client_->Delete(path, true));
- }
-
- ASSERT_OK(this->client_->MakeDirectory(path));
- ASSERT_TRUE(this->client_->Exists(path));
- std::vector<HdfsPathInfo> listing;
- ARROW_EXPECT_OK(this->client_->ListDirectory(path, &listing));
- ASSERT_EQ(0, listing.size());
- ARROW_EXPECT_OK(this->client_->Delete(path, true));
- ASSERT_FALSE(this->client_->Exists(path));
- ASSERT_RAISES(IOError, this->client_->ListDirectory(path, &listing));
-}
-
-TEST_F(TestHadoopFileSystem, GetCapacityUsed) {
- SKIP_IF_NO_DRIVER();
-
- // Who knows what is actually in your DFS cluster, but expect it to have
- // positive used bytes and capacity
- int64_t nbytes = 0;
- ASSERT_OK(this->client_->GetCapacity(&nbytes));
- ASSERT_LT(0, nbytes);
-
- ASSERT_OK(this->client_->GetUsed(&nbytes));
- ASSERT_LT(0, nbytes);
-}
-
-TEST_F(TestHadoopFileSystem, GetPathInfo) {
- SKIP_IF_NO_DRIVER();
-
- HdfsPathInfo info;
-
- ASSERT_OK(this->MakeScratchDir());
-
- // Directory info
- ASSERT_OK(this->client_->GetPathInfo(this->scratch_dir_, &info));
- ASSERT_EQ(ObjectType::DIRECTORY, info.kind);
- ASSERT_EQ(this->HdfsAbsPath(this->scratch_dir_), info.name);
- ASSERT_EQ(this->conf_.user, info.owner);
-
- // TODO(wesm): test group, other attrs
-
- auto path = this->ScratchPath("test-file");
-
- const int size = 100;
-
- std::vector<uint8_t> buffer = RandomData(size);
-
- ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size));
- ASSERT_OK(this->client_->GetPathInfo(path, &info));
-
- ASSERT_EQ(ObjectType::FILE, info.kind);
- ASSERT_EQ(this->HdfsAbsPath(path), info.name);
- ASSERT_EQ(this->conf_.user, info.owner);
- ASSERT_EQ(size, info.size);
-}
-
-TEST_F(TestHadoopFileSystem, GetPathInfoNotExist) {
- // ARROW-2919: Test that the error message is reasonable
- SKIP_IF_NO_DRIVER();
-
- ASSERT_OK(this->MakeScratchDir());
- auto path = this->ScratchPath("path-does-not-exist");
-
- HdfsPathInfo info;
- Status s = this->client_->GetPathInfo(path, &info);
- ASSERT_TRUE(s.IsIOError());
-
- const std::string error_message = s.ToString();
-
- // Check that the file path is found in the error message
- ASSERT_LT(error_message.find(path), std::string::npos);
-}
-
-TEST_F(TestHadoopFileSystem, AppendToFile) {
- SKIP_IF_NO_DRIVER();
-
- ASSERT_OK(this->MakeScratchDir());
-
- auto path = this->ScratchPath("test-file");
- const int size = 100;
-
- std::vector<uint8_t> buffer = RandomData(size);
- ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size));
-
- // now append
- ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size, true));
-
- HdfsPathInfo info;
- ASSERT_OK(this->client_->GetPathInfo(path, &info));
- ASSERT_EQ(size * 2, info.size);
-}
-
-TEST_F(TestHadoopFileSystem, ListDirectory) {
- SKIP_IF_NO_DRIVER();
-
- const int size = 100;
- std::vector<uint8_t> data = RandomData(size);
-
- auto p1 = this->ScratchPath("test-file-1");
- auto p2 = this->ScratchPath("test-file-2");
- auto d1 = this->ScratchPath("test-dir-1");
-
- ASSERT_OK(this->MakeScratchDir());
- ASSERT_OK(this->WriteDummyFile(p1, data.data(), size));
- ASSERT_OK(this->WriteDummyFile(p2, data.data(), size / 2));
- ASSERT_OK(this->client_->MakeDirectory(d1));
-
- std::vector<HdfsPathInfo> listing;
- ASSERT_OK(this->client_->ListDirectory(this->scratch_dir_, &listing));
-
- // Do it again, appends!
- ASSERT_OK(this->client_->ListDirectory(this->scratch_dir_, &listing));
-
- ASSERT_EQ(6, static_cast<int>(listing.size()));
-
- // Argh, well, shouldn't expect the listing to be in any particular order
- for (size_t i = 0; i < listing.size(); ++i) {
- const HdfsPathInfo& info = listing[i];
- if (info.name == this->HdfsAbsPath(p1)) {
- ASSERT_EQ(ObjectType::FILE, info.kind);
- ASSERT_EQ(size, info.size);
- } else if (info.name == this->HdfsAbsPath(p2)) {
- ASSERT_EQ(ObjectType::FILE, info.kind);
- ASSERT_EQ(size / 2, info.size);
- } else if (info.name == this->HdfsAbsPath(d1)) {
- ASSERT_EQ(ObjectType::DIRECTORY, info.kind);
- } else {
- FAIL() << "Unexpected path: " << info.name;
- }
- }
-}
-
-TEST_F(TestHadoopFileSystem, ReadableMethods) {
Review Comment:
@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. https://github.com/apache/arrow/pull/45998/changes/5b7f1bca850919cd30c870cbe35f4a2f46142dfe
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5712455575
@github-actions crossbow submit test-hdfs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5712487946
@github-actions crossbow submit test-*hdfs*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5678825788
```
Unable to match any tasks for `test-conda-python-3.11-hdfs-*`
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/34959045831
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2996561344
@pitrou gentle ping. Would I be too optimistic to try to get it into 21.0.0?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-2894514536
@pitrou I cleaned up the CI failures (others are not related) and am hoping this changes will not be too bad to review :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3474213065
##########
cpp/src/arrow/filesystem/hdfs_internal_test.cc:
##########
Review Comment:
Is this file useful at all? `hdfs_test.cc` is supposed to exercise all public filesystem APIs already.
##########
cpp/src/arrow/filesystem/hdfs_internal.cc:
##########
@@ -51,12 +52,20 @@
namespace arrow {
using internal::GetEnvVarNative;
+using internal::IOErrorFromErrno;
using internal::PlatformFilename;
-namespace io::internal {
+namespace fs::internal {
namespace {
+#define CHECK_FAILURE(RETURN_VALUE, WHAT) \
Review Comment:
Can you `#undef` this at the end of the file?
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -19,21 +19,34 @@
#include <memory>
#include <string>
+#include <unordered_map>
#include <vector>
#include "arrow/filesystem/filesystem.h"
-#include "arrow/io/hdfs.h"
#include "arrow/util/uri.h"
namespace arrow::fs {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
+struct HdfsPathInfo;
Review Comment:
I don't think we should expose these?
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -88,10 +98,48 @@ class ARROW_EXPORT HadoopFileSystem : public FileSystem {
Status DeleteFile(const std::string& path) override;
+ Status MakeDirectory(const std::string& path);
Review Comment:
I understand these methods existed on the legacy HDFS filesystem class, but I think we should only implement the standard `FileSystem` methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3505339558
##########
cpp/src/arrow/filesystem/hdfs.h:
##########
@@ -19,21 +19,34 @@
#include <memory>
#include <string>
+#include <unordered_map>
#include <vector>
#include "arrow/filesystem/filesystem.h"
-#include "arrow/io/hdfs.h"
#include "arrow/util/uri.h"
namespace arrow::fs {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
+struct HdfsPathInfo;
Review Comment:
Yeah, went with this idea and updated the Python bindings also: https://github.com/apache/arrow/pull/45998/changes/972f7a4b3e2360e29bf95d64cff41070c7316347
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r2136977525
##########
cpp/src/arrow/filesystem/hdfs_internal.h:
##########
@@ -33,8 +38,13 @@ namespace arrow {
class Status;
+using internal::IOErrorFromErrno;
+
namespace io::internal {
+class HdfsReadableFile;
+class HdfsOutputStream;
+
Review Comment:
I think I wanted to change as little as possible. But yeah, should be changed to `fs::internal`. Will do!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r3766783453
##########
cpp/src/arrow/io/hdfs_test.cc:
##########
@@ -1,466 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one
-// or more contributor license agreements. See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership. The ASF licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License. You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied. See the License for the
-// specific language governing permissions and limitations
-// under the License.
-
-#include <atomic>
-#include <cstdint>
-#include <cstdlib>
-#include <cstring>
-#include <filesystem>
-#include <iostream>
-#include <memory>
-#include <sstream> // IWYU pragma: keep
-#include <string>
-#include <thread>
-#include <vector>
-
-#include <gtest/gtest.h>
-
-#include "arrow/buffer.h"
-#include "arrow/io/hdfs.h"
-#include "arrow/io/hdfs_internal.h"
-#include "arrow/io/interfaces.h"
-#include "arrow/status.h"
-#include "arrow/testing/gtest_util.h"
-#include "arrow/testing/util.h"
-
-namespace arrow {
-namespace io {
-
-std::vector<uint8_t> RandomData(int64_t size) {
- std::vector<uint8_t> buffer(size);
- random_bytes(size, 0, buffer.data());
- return buffer;
-}
-
-class TestHadoopFileSystem : public ::testing::Test {
- public:
- Status MakeScratchDir() {
- if (client_->Exists(scratch_dir_)) {
- RETURN_NOT_OK((client_->Delete(scratch_dir_, true)));
- }
- return client_->MakeDirectory(scratch_dir_);
- }
-
- Status WriteDummyFile(const std::string& path, const uint8_t* buffer, int64_t size,
- bool append = false, int buffer_size = 0, int16_t replication = 0,
- int default_block_size = 0) {
- std::shared_ptr<HdfsOutputStream> file;
- RETURN_NOT_OK(client_->OpenWritable(path, append, buffer_size, replication,
- default_block_size, &file));
-
- RETURN_NOT_OK(file->Write(buffer, size));
- RETURN_NOT_OK(file->Close());
-
- return Status::OK();
- }
-
- std::string ScratchPath(const std::string& name) {
- std::stringstream ss;
- ss << scratch_dir_ << "/" << name;
- return ss.str();
- }
-
- std::string HdfsAbsPath(const std::string& relpath) {
- std::stringstream ss;
- ss << "hdfs://" << conf_.host << ":" << conf_.port << relpath;
- return ss.str();
- }
-
- // Set up shared state between unit tests
- void SetUp() {
- internal::LibHdfsShim* driver_shim;
-
- client_ = nullptr;
- scratch_dir_ =
- (std::filesystem::temp_directory_path() / "arrow-hdfs/scratch-").string();
- int random_size = 4;
- scratch_dir_.resize(scratch_dir_.size() + random_size, '%');
- random_alnum(
- random_size, 0,
- reinterpret_cast<uint8_t*>(&scratch_dir_[scratch_dir_.size() - random_size]));
-
- loaded_driver_ = false;
-
- Status msg = ConnectLibHdfs(&driver_shim);
- if (!msg.ok()) {
- if (std::getenv("ARROW_HDFS_TEST_LIBHDFS_REQUIRE")) {
- FAIL() << "Loading libhdfs failed: " << msg.ToString();
- } else {
- std::cout << "Loading libhdfs failed, skipping tests gracefully: "
- << msg.ToString() << std::endl;
- }
- return;
- }
-
- loaded_driver_ = true;
-
- const char* host = std::getenv("ARROW_HDFS_TEST_HOST");
- const char* port = std::getenv("ARROW_HDFS_TEST_PORT");
- const char* user = std::getenv("ARROW_HDFS_TEST_USER");
-
- ASSERT_TRUE(user != nullptr) << "Set ARROW_HDFS_TEST_USER";
-
- conf_.host = host == nullptr ? "localhost" : host;
- conf_.user = user;
- conf_.port = port == nullptr ? 20500 : atoi(port);
-
- ASSERT_OK(HadoopFileSystem::Connect(&conf_, &client_));
- }
-
- void TearDown() {
- if (client_) {
- if (client_->Exists(scratch_dir_)) {
- ARROW_EXPECT_OK(client_->Delete(scratch_dir_, true));
- }
- ARROW_EXPECT_OK(client_->Disconnect());
- }
- }
-
- HdfsConnectionConfig conf_;
- bool loaded_driver_;
-
- // Resources shared amongst unit tests
- std::string scratch_dir_;
- std::shared_ptr<HadoopFileSystem> client_;
-};
-
-#define SKIP_IF_NO_DRIVER() \
- if (!this->loaded_driver_) { \
- GTEST_SKIP() << "Driver not loaded, skipping"; \
- }
-
-TEST_F(TestHadoopFileSystem, ConnectsAgain) {
- SKIP_IF_NO_DRIVER();
-
- std::shared_ptr<HadoopFileSystem> client;
- ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client));
- ASSERT_OK(client->Disconnect());
-}
-
-TEST_F(TestHadoopFileSystem, MultipleClients) {
- SKIP_IF_NO_DRIVER();
-
- ASSERT_OK(this->MakeScratchDir());
-
- std::shared_ptr<HadoopFileSystem> client1;
- std::shared_ptr<HadoopFileSystem> client2;
- ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client1));
- ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client2));
- ASSERT_OK(client1->Disconnect());
-
- // client2 continues to function after equivalent client1 has shutdown
- std::vector<HdfsPathInfo> listing;
- ASSERT_OK(client2->ListDirectory(this->scratch_dir_, &listing));
- ASSERT_OK(client2->Disconnect());
-}
-
-TEST_F(TestHadoopFileSystem, MakeDirectory) {
- SKIP_IF_NO_DRIVER();
-
- std::string path = this->ScratchPath("create-directory");
-
- if (this->client_->Exists(path)) {
- ASSERT_OK(this->client_->Delete(path, true));
- }
-
- ASSERT_OK(this->client_->MakeDirectory(path));
- ASSERT_TRUE(this->client_->Exists(path));
- std::vector<HdfsPathInfo> listing;
- ARROW_EXPECT_OK(this->client_->ListDirectory(path, &listing));
- ASSERT_EQ(0, listing.size());
- ARROW_EXPECT_OK(this->client_->Delete(path, true));
- ASSERT_FALSE(this->client_->Exists(path));
- ASSERT_RAISES(IOError, this->client_->ListDirectory(path, &listing));
-}
-
-TEST_F(TestHadoopFileSystem, GetCapacityUsed) {
- SKIP_IF_NO_DRIVER();
-
- // Who knows what is actually in your DFS cluster, but expect it to have
- // positive used bytes and capacity
- int64_t nbytes = 0;
- ASSERT_OK(this->client_->GetCapacity(&nbytes));
- ASSERT_LT(0, nbytes);
-
- ASSERT_OK(this->client_->GetUsed(&nbytes));
- ASSERT_LT(0, nbytes);
-}
-
-TEST_F(TestHadoopFileSystem, GetPathInfo) {
- SKIP_IF_NO_DRIVER();
-
- HdfsPathInfo info;
-
- ASSERT_OK(this->MakeScratchDir());
-
- // Directory info
- ASSERT_OK(this->client_->GetPathInfo(this->scratch_dir_, &info));
- ASSERT_EQ(ObjectType::DIRECTORY, info.kind);
- ASSERT_EQ(this->HdfsAbsPath(this->scratch_dir_), info.name);
- ASSERT_EQ(this->conf_.user, info.owner);
-
- // TODO(wesm): test group, other attrs
-
- auto path = this->ScratchPath("test-file");
-
- const int size = 100;
-
- std::vector<uint8_t> buffer = RandomData(size);
-
- ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size));
- ASSERT_OK(this->client_->GetPathInfo(path, &info));
-
- ASSERT_EQ(ObjectType::FILE, info.kind);
- ASSERT_EQ(this->HdfsAbsPath(path), info.name);
- ASSERT_EQ(this->conf_.user, info.owner);
- ASSERT_EQ(size, info.size);
-}
-
-TEST_F(TestHadoopFileSystem, GetPathInfoNotExist) {
- // ARROW-2919: Test that the error message is reasonable
- SKIP_IF_NO_DRIVER();
-
- ASSERT_OK(this->MakeScratchDir());
- auto path = this->ScratchPath("path-does-not-exist");
-
- HdfsPathInfo info;
- Status s = this->client_->GetPathInfo(path, &info);
- ASSERT_TRUE(s.IsIOError());
-
- const std::string error_message = s.ToString();
-
- // Check that the file path is found in the error message
- ASSERT_LT(error_message.find(path), std::string::npos);
-}
-
-TEST_F(TestHadoopFileSystem, AppendToFile) {
- SKIP_IF_NO_DRIVER();
-
- ASSERT_OK(this->MakeScratchDir());
-
- auto path = this->ScratchPath("test-file");
- const int size = 100;
-
- std::vector<uint8_t> buffer = RandomData(size);
- ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size));
-
- // now append
- ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size, true));
-
- HdfsPathInfo info;
- ASSERT_OK(this->client_->GetPathInfo(path, &info));
- ASSERT_EQ(size * 2, info.size);
-}
-
-TEST_F(TestHadoopFileSystem, ListDirectory) {
- SKIP_IF_NO_DRIVER();
-
- const int size = 100;
- std::vector<uint8_t> data = RandomData(size);
-
- auto p1 = this->ScratchPath("test-file-1");
- auto p2 = this->ScratchPath("test-file-2");
- auto d1 = this->ScratchPath("test-dir-1");
-
- ASSERT_OK(this->MakeScratchDir());
- ASSERT_OK(this->WriteDummyFile(p1, data.data(), size));
- ASSERT_OK(this->WriteDummyFile(p2, data.data(), size / 2));
- ASSERT_OK(this->client_->MakeDirectory(d1));
-
- std::vector<HdfsPathInfo> listing;
- ASSERT_OK(this->client_->ListDirectory(this->scratch_dir_, &listing));
-
- // Do it again, appends!
- ASSERT_OK(this->client_->ListDirectory(this->scratch_dir_, &listing));
-
- ASSERT_EQ(6, static_cast<int>(listing.size()));
-
- // Argh, well, shouldn't expect the listing to be in any particular order
- for (size_t i = 0; i < listing.size(); ++i) {
- const HdfsPathInfo& info = listing[i];
- if (info.name == this->HdfsAbsPath(p1)) {
- ASSERT_EQ(ObjectType::FILE, info.kind);
- ASSERT_EQ(size, info.size);
- } else if (info.name == this->HdfsAbsPath(p2)) {
- ASSERT_EQ(ObjectType::FILE, info.kind);
- ASSERT_EQ(size / 2, info.size);
- } else if (info.name == this->HdfsAbsPath(d1)) {
- ASSERT_EQ(ObjectType::DIRECTORY, info.kind);
- } else {
- FAIL() << "Unexpected path: " << info.name;
- }
- }
-}
-
-TEST_F(TestHadoopFileSystem, ReadableMethods) {
Review Comment:
There has been some moving around with this test:
- first it has been moved under filesystems and named `hdfs_test_io.ccโ`: https://github.com/apache/arrow/pull/45998/changes/b0cca50504ef28d289037a0e71fd4da055cc1c3d#diff-65fee404eb58a58c664cbbab64f066736a723a6cb5b094d94c43f8dcb076047b
- then it has been renamed to `hdfs_io_test.cc`: https://github.com/apache/arrow/pull/45998/changes/caab0d078a3802f4f0fec59e99f433c2eb1a0e81
- and again renamed to `hdfs_internal_test.cc` and updated a bit when merging internal classes here: https://github.com/apache/arrow/pull/45998/changes/2bbb63097824dcb60d5a65b94109945b101bf639#diff-f4795a70b2c29c3464b6042a8a19ae801493bff7fa711e7db0d6443c53e32dea
- at the end it has been removed in https://github.com/apache/arrow/pull/45998/changes/b9aab57f256606042a5ada704ceaa833b1e8961c as per this comment: https://github.com/apache/arrow/pull/45998#discussion_r3474213065
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r4035450752
##########
cpp/src/arrow/meson.build:
##########
@@ -484,7 +482,10 @@ disable Arrow gcs support with -Dgcs=disabled.
endif
if needs_hdfs
- arrow_filesystem_srcs += ['filesystem/hdfs.cc']
+ arrow_filesystem_srcs += [
+ 'filesystem/hdfs.cc',
+ 'filesystem/hdfs_internal.cc',
+ ]
Review Comment:
References removed in https://github.com/apache/arrow/pull/45998/changes/0625e9eae926a306e5e15e756cdf94ae7b24f0fc as the tests now live under filesystems and are tested within that module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5678876056
Revision: 5b7f1bca850919cd30c870cbe35f4a2f46142dfe
Submitted crossbow builds: [ursacomputing/crossbow @ actions-cf54006206](https://github.com/ursacomputing/crossbow/branches/all?query=actions-cf54006206)
|Task|Status|
|----|------|
|test-conda-python-3.10-hdfs-2.9.2|[](https://github.com/ursacomputing/crossbow/actions/runs/34959384342/job/104349071867)|
|test-conda-python-3.10-hdfs-3.2.1|[](https://github.com/ursacomputing/crossbow/actions/runs/34959384040/job/104349070524)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #45998:
URL: https://github.com/apache/arrow/pull/45998#discussion_r4035429802
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -1016,7 +1008,7 @@ if(ARROW_FILESYSTEM)
endif()
endif()
if(ARROW_HDFS)
- list(APPEND ARROW_FILESYSTEM_SRCS filesystem/hdfs.cc)
+ list(APPEND ARROW_FILESYSTEM_SRCS filesystem/hdfs.cc filesystem/hdfs_internal.cc)
Review Comment:
Added a fix here: https://github.com/apache/arrow/pull/45998/changes/0625e9eae926a306e5e15e756cdf94ae7b24f0fc#diff-8e690315a407919d147f16a9a534fc9f5c3f6345b58820e2057afa7cd14bea6b. Now `dlopen` and other dl symbols will be able to resolve on Linux machines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5713773810
Revision: 2741e3f26a593a05c13d5315a5f625d6fa596a3b
Submitted crossbow builds: [ursacomputing/crossbow @ actions-aab95289e3](https://github.com/ursacomputing/crossbow/branches/all?query=actions-aab95289e3)
|Task|Status|
|----|------|
|test-conda-python-3.11-hdfs-2.10.2|[](https://github.com/ursacomputing/crossbow/actions/runs/35217158947/job/105188241947)|
|test-conda-python-3.11-hdfs-3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/35217158022/job/105188238647)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45747: [C++] Remove deprecated ObjectType and FileStatistics, refactor hdfs code [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #45998:
URL: https://github.com/apache/arrow/pull/45998#issuecomment-5713748560
@github-actions crossbow submit test-*hdfs*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org