Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI][C++] TestHadoopFileSystem.FileSystemFromUri fails on our nightly hdfs tests #35635

Closed
raulcd opened this issue May 17, 2023 · 2 comments · Fixed by #36063
Closed

[CI][C++] TestHadoopFileSystem.FileSystemFromUri fails on our nightly hdfs tests #35635

raulcd opened this issue May 17, 2023 · 2 comments · Fixed by #36063

Comments

@raulcd
Copy link
Member

raulcd commented May 17, 2023

Describe the bug, including details regarding any error messages, version, and platform.

Nightly HDFS tests have been failing for the last couple of weeks:

The error failure is the following:

 [ RUN      ] TestHadoopFileSystem.FileSystemFromUri
/arrow/cpp/src/arrow/filesystem/hdfs_test.cc:119: !!! uri = hdfs://impala:8020/?replication=0&user=hdfs
/arrow/cpp/src/arrow/filesystem/hdfs_test.cc:123: Failure
Expected equality of these values:
  path
    Which is: ""
  "/"
[  FAILED  ] TestHadoopFileSystem.FileSystemFromUri (32 ms)

This was introduced on:
https://github.com/apache/arrow/pull/34420/files#diff-fb718c6107b7bbdeac010067dd0633e0eb3bad5ea054013ffc3bd19652748362

I've been able to fix the build locally by applying this patch to modify the assert on the test:

diff --git a/cpp/src/arrow/filesystem/hdfs_test.cc b/cpp/src/arrow/filesystem/hdfs_test.cc
index 7ad9e6c..5c692ec 100644
--- a/cpp/src/arrow/filesystem/hdfs_test.cc
+++ b/cpp/src/arrow/filesystem/hdfs_test.cc
@@ -120,7 +120,7 @@ class TestHadoopFileSystem : public ::testing::Test, public HadoopFileSystemTest
     ASSERT_OK_AND_ASSIGN(uri_fs, FileSystemFromUri(ss.str(), &path));
     ASSERT_EQ(path, "/");
     ASSERT_OK_AND_ASSIGN(path, uri_fs->PathFromUri(ss.str()));
-    ASSERT_EQ(path, "/");
+    ASSERT_EQ(path, "");
 
     // Sanity check
     ASSERT_OK(uri_fs->CreateDir("AB"));

but I am unsure whether this is the expected behavior or there is an underlying issue.

Component(s)

C++, Continuous Integration

@raulcd
Copy link
Member Author

raulcd commented May 17, 2023

@westonpace do you think the assert should be modified here or there's something else to be done?

@raulcd raulcd changed the title [C++][CI] TestHadoopFileSystem.FileSystemFromUri fails on our nightly hdfs tests [CI][C++] TestHadoopFileSystem.FileSystemFromUri fails on our nightly hdfs tests May 17, 2023
@westonpace
Copy link
Member

No, we shouldn't fix the assert. The HDFS filesystem should be returning / in this case (we trim the trailing slash but not if the entire path is /). I can put a fix together.

@westonpace westonpace self-assigned this May 18, 2023
@jorisvandenbossche jorisvandenbossche added this to the 13.0.0 milestone May 24, 2023
westonpace pushed a commit that referenced this issue Jun 20, 2023
…lper to fix HDFS tests (#36063)

### Rationale for this change

As discussed on the issue the HDFS filesystem should be returning `/` if the entire path is `/`

### What changes are included in this PR?

Change behavior of helper `PathFromUriHelper` when ignoring host to preserve `/`

### Are these changes tested?

On CI and archery.

### Are there any user-facing changes?

No
* Closes: #35635

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment