Uh oh!
There was an error while loading. Please reload this page.
GH-38703: [C++][FS][Azure] Implement DeleteFile() - #39692
Conversation
There was a problem hiding this comment.
Needless file? Is this not the regression testing? Should I not update this file when adding functionality such as this? Mind you, I ask only because I am trying to learn the process used here. I am willing to back out my changes, just wanting to learn. Thanks!
There was a problem hiding this comment.
I think that /azurefs_test.cc in the top-level directory is needless.
Because it should be at cpp/src/arrow/filesystem/azurefs_test.cc. (And it already exists.)
There was a problem hiding this comment.
Apologies, things picked up at work. Just now revisiting this. OK, I am confused. The version of azurefs_test.cc that I submitted was located within cpp/src/arrow/filesystem/ ... as was azurefs.cc. So where did I go wrong with this?
| auto file_client = | ||
| datalake_service_client_->GetFileSystemClient(location.container) | ||
| .GetDirectoryClient(directory).GetFileClient(file); |
There was a problem hiding this comment.
You can GetFileClient(location.path) from filesystem client -- you don't have to create a directory client.
| return internal::ExceptionToStatus( | ||
| "Failed to delete a file: " + location.all + ": " + | ||
| file_client.GetUrl(), | ||
| exception); |
There was a problem hiding this comment.
It's been a while since you rebased. This function expects exception to be the first parameter now and all the other components can be passed as separate arguments, so you don't have to + the strings yourself.
| .GetDirectoryClient(directory).GetFileClient(file); | ||
| try { | ||
| auto response = file_client.DeleteIfExists(); |
There was a problem hiding this comment.
If you use Delete() you get an exception when the file doesn't exist and then you can return PathNotFound(location).
av8or1
commented
Jan 25, 2024
Based on the feedback from kou and felipecrv, I think I'll close this PR, reset my local repo, incorporate felipecrv's feedback and then open a new PR. Thanks |
Rationale for this change
What changes are included in this PR?
Resolution of the conflict in ~cpp/src/arrow/filesystem/azurefs_test.cc. Here are my original comments regarding this modification in the abstract:
Modified:
~cpp/src/arrow/filesystem/azurerefs.cc
~cpp/src/arrow/filesystem/azurerefs_test.cc
These modifications implement the method and provide regression testing, respectively.
Are these changes tested?
I tested the modification by creating a file via the web browser on our internal ADLS, then ran a sample program that deleted the file. I added three regression tests to cover the use case scenarios of:
A valid delete attempt, where "valid" means that the file exists and is indeed a file
An intentional failure where a file delete is attempted, but the file does not exist
An intentional failure where a file delete is attempted, but the target is not a file
Regarding the modifications to the regression suite... I don't know how to run the Arrow regression. Need instructions. The original code in ~cpp/src/arrow/filesystem/azurefs.cc is tested, as previously mentioned in #39648.
Are there any user-facing changes?
Overall, this code includes a new method to delete a file on an Azure server. That is likely something that will need inclusion in the documentation, but as this is my first PR, I can't say how things are usually done with the Arrow project.
DeleteFile()#38703