-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-38703: [C++][FS][Azure] Implement DeleteFile() #39840
Changes from 25 commits
6947360
7df1f8d
a08d5a1
79e6206
8559881
6e082dd
4863b20
fdde91a
b812aaa
3330284
b89a07a
4b1c635
516be74
6c0fa18
110932b
953da8e
6512c12
d2e6f9b
58888b4
5af7159
b96af56
79e80b7
022bd57
58c4ccd
3cce85c
168356e
dd60704
39eb5ae
fc7ada2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1382,6 +1382,32 @@ TEST_F(TestAzuriteFileSystem, DeleteDirContentsFailureNonexistent) { | |
this->TestDeleteDirContentsFailureNonexistent(); | ||
} | ||
|
||
TEST_F(TestAzuriteFileSystem, DeleteFileSuccess) { | ||
CreateFile(fs(), "abc", "data"); | ||
arrow::fs::AssertFileInfo(fs(), "abc", FileType::File); | ||
ASSERT_OK(fs()->DeleteFile("abc")); | ||
arrow::fs::AssertFileInfo(fs(), "abc", FileType::NotFound); | ||
} | ||
|
||
TEST_F(TestAzuriteFileSystem, DeleteFileFailureNonexistent) { | ||
ASSERT_RAISES(IOError, fs()->DeleteFile("nonexistent")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This deletes a container not a blob. Could you create a container and delete an nonexistent blob in the container? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I have changed the test to the following:
|
||
} | ||
|
||
TEST_F(TestAzuriteFileSystem, DeleteFileFailureContainer) { | ||
const auto container_name = PreexistingData::RandomContainerName(rng_); | ||
ASSERT_OK(fs()->CreateDir(container_name)); | ||
arrow::fs::AssertFileInfo(fs(), container_name, FileType::Directory); | ||
ASSERT_RAISES(IOError, fs()->DeleteFile(container_name)); | ||
} | ||
|
||
av8or1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
TEST_F(TestAzuriteFileSystem, DeleteFileFailureDirectory) { | ||
const auto directory_name = | ||
ConcatAbstractPath(PreexistingData::RandomContainerName(rng_), "directory"); | ||
ASSERT_OK(fs()->CreateDir(directory_name)); | ||
arrow::fs::AssertFileInfo(fs(), directory_name, FileType::Directory); | ||
ASSERT_RAISES(IOError, fs()->DeleteFile(directory_name)); | ||
} | ||
|
||
TEST_F(TestAzuriteFileSystem, CopyFileSuccessDestinationNonexistent) { | ||
auto data = SetUpPreexistingData(); | ||
const auto destination_path = data.ContainerPath("copy-destionation"); | ||
|
@@ -1868,6 +1894,5 @@ TEST_F(TestAzuriteFileSystem, OpenInputFileClosed) { | |
ASSERT_RAISES(Invalid, stream->ReadAt(1, 1)); | ||
ASSERT_RAISES(Invalid, stream->Seek(2)); | ||
} | ||
|
||
} // namespace fs | ||
} // namespace arrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test failed:
https://github.com/apache/arrow/actions/runs/7761414467/job/21171628885?pr=39840#step:6:3519
Could you run tests on your local environment? If you're on Linux, the following will work (you also need to install Azurite by
sudo npm install -g azurite
):(Or you can use Docker instead.)
I think that we need to do something like the following:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also:
UBUNTU=22.04 archery docker run ubuntu-cpp-sanitizer
that is used for the "AMD64 Ubuntu 22.04 C++ ASAN UBSAN" CI jobThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I have incorporated that suggestion into the DeleteFileSuccess regression test. Thanks!
I did not see this failure now, but I will re-run again tomorrow to verify that I ran the tests correctly.