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.

Sign up for GitHub

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

Jump to bottom

[C++][FS][Azure] Implement DeleteFile() #38703

Closed
kou opened this issue Nov 14, 2023 · 4 comments · Fixed by #39840
Closed

[C++][FS][Azure] Implement DeleteFile() #38703

kou opened this issue Nov 14, 2023 · 4 comments · Fixed by #39840
Assignees
@av8or1
Labels
Component: C++ Type: enhancement
Milestone
16.0.0

Comments

@kou
Copy link
Member

kou commented Nov 14, 2023

Describe the enhancement requested

It's not implemented yet.

This is a child of GH-18014.

Component(s)

C++
@kou kou mentioned this issue Nov 14, 2023
Closed
@av8or1
Copy link
Contributor

av8or1 commented Dec 4, 2023

Take

@av8or1
Copy link
Contributor

av8or1 commented Jan 8, 2024

Tom, I have an initial implementation completed. However, I have concerns surrounding testing. Specifically:

  1. How have other contributors been testing similar functionality?
  2. Is there a scripted process that I can follow to replicate that testing?
  3. I took a look at the azurefs_test.cc file and noted the modifications made to provide regression for similar functionality. However I am wondering what all use cases I would need to support for DeleteFile? I am not as familiar with this type of testing, as we do our own in-house stuff, but this seems straightforward enough.

Finally, I implemented this using DeleteIfExists() rather than Delete(), for I saw that the former method was used elsewhere in the Azure work. Any feedback regarding that choice?

@kou
Copy link
Member Author

kou commented Jan 10, 2024 •
edited
Loading

Tom, I have an initial implementation completed. However, I have concerns surrounding testing. Specifically:

Great! Could you open a draft PR for it?

  1. How have other contributors been testing similar functionality?

I'm using ninja -C cpp.build debug/arrow-azurefs-test && LD_LIBRARY_PATH=$PWD/cpp.build/debug cpp.build/debug/arrow-azurefs-test. Here are some notes for the command line:

  • I'm on Debian GNU/Linux
  • I use cpp.build as my build directory for cpp/
  • The current directory is the top directory of cloned apache/arrow
  1. Is there a scripted process that I can follow to replicate that testing?

https://arrow.apache.org/docs/developers/cpp/development.html#running-unit-tests is the standard process but it runs all tests.

  1. I took a look at the azurefs_test.cc file and noted the modifications made to provide regression for similar functionality. However I am wondering what all use cases I would need to support for DeleteFile? I am not as familiar with this type of testing, as we do our own in-house stuff, but this seems straightforward enough.

We can borrow test patterns from GCS's one for now: https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/gcsfs_test.cc

We'll use generic tests eventually after we implement all features: #39069

Finally, I implemented this using DeleteIfExists() rather than Delete(), for I saw that the former method was used elsewhere in the Azure work. Any feedback regarding that choice?

Let's discuss this on a draft PR.

@av8or1
Copy link
Contributor

av8or1 commented Jan 17, 2024 •
edited by kou
Loading

Thanks. I have opened a PR:
#39648

Note that this is my first attempt at the process of contributing to the Arrow library.

This was referenced Jan 18, 2024
Closed
Closed
@github-actions github-actions bot mentioned this issue Jan 29, 2024
Merged
@kou kou closed this as completed in #39840 Feb 8, 2024
kou added a commit that referenced this issue Feb 8, 2024
@av8or1 @kou
GH-38703: [C++][FS][Azure] Implement DeleteFile() (#39840)
fa26fa5 
### Rationale for this change

`DeleteFile()` API isn't implemented yet.

### What changes are included in this PR?

Implement `DeleteFile()` by the "Delete Blob" API: https://learn.microsoft.com/en-us/rest/api/storageservices/delete-blob

### 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 a container
* An intentional failure where a file delete is attempted, but the target is a directory

### Are there any user-facing changes?

Yes.

* Closes: #38703

Lead-authored-by: av8or1 <fivetomins@yahoo.com>
Co-authored-by: jerry.adair <Jerry.Adair@sas.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 16.0.0 milestone Feb 8, 2024
@felipecrv felipecrv mentioned this issue Feb 14, 2024
Closed
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@av8or1 @kou @dgreiss
apacheGH-38703: [C++][FS][Azure] Implement DeleteFile() (apache#39840)
75f0a72 
### Rationale for this change

`DeleteFile()` API isn't implemented yet.

### What changes are included in this PR?

Implement `DeleteFile()` by the "Delete Blob" API: https://learn.microsoft.com/en-us/rest/api/storageservices/delete-blob

### 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 a container
* An intentional failure where a file delete is attempted, but the target is a directory

### Are there any user-facing changes?

Yes.

* Closes: apache#38703

Lead-authored-by: av8or1 <fivetomins@yahoo.com>
Co-authored-by: jerry.adair <Jerry.Adair@sas.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@raulcd raulcd mentioned this issue Feb 20, 2024
Closed
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
@av8or1 @kou @zanmato1984
apacheGH-38703: [C++][FS][Azure] Implement DeleteFile() (apache#39840)
72773fe 
### Rationale for this change

`DeleteFile()` API isn't implemented yet.

### What changes are included in this PR?

Implement `DeleteFile()` by the "Delete Blob" API: https://learn.microsoft.com/en-us/rest/api/storageservices/delete-blob

### 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 a container
* An intentional failure where a file delete is attempted, but the target is a directory

### Are there any user-facing changes?

Yes.

* Closes: apache#38703

Lead-authored-by: av8or1 <fivetomins@yahoo.com>
Co-authored-by: jerry.adair <Jerry.Adair@sas.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
@av8or1 @kou @thisisnic
apacheGH-38703: [C++][FS][Azure] Implement DeleteFile() (apache#39840)
cf176b4 
### Rationale for this change

`DeleteFile()` API isn't implemented yet.

### What changes are included in this PR?

Implement `DeleteFile()` by the "Delete Blob" API: https://learn.microsoft.com/en-us/rest/api/storageservices/delete-blob

### 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 a container
* An intentional failure where a file delete is attempted, but the target is a directory

### Are there any user-facing changes?

Yes.

* Closes: apache#38703

Lead-authored-by: av8or1 <fivetomins@yahoo.com>
Co-authored-by: jerry.adair <Jerry.Adair@sas.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@av8or1 av8or1

Labels
Component: C++ Type: enhancement
Projects
None yet
Milestone
16.0.0
2 participants
@kou @av8or1