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

GH-38703: [C++][FS][Azure] Implement DeleteFile() #39648

Closed
wants to merge 0 commits into from
Closed

GH-38703: [C++][FS][Azure] Implement DeleteFile() #39648

wants to merge 0 commits into from

Conversation

av8or1
Copy link
Contributor

@av8or1 av8or1 commented Jan 17, 2024

Rationale for this change

Completed the modifications to implement the DeleteFile() method to support deletion of files on ADLS.

What changes are included in this PR?

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:

  1. A valid delete attempt, where "valid" means that the file exists and is indeed a file
  2. An intentional failure where a file delete is attempted, but the file does not exist
  3. An intentional failure where a file delete is attempted, but the target is not a file

Are there any user-facing changes?

This is my first contribution to the Arrow library. Therefore I am not certain if the addition of a relatively low-level feature such as this would propagate to external documentation. That said, given the target audience of the library (developers), I would imagine that such a method would have documentation.

Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@kou
Copy link
Member

kou commented Jan 18, 2024

@av8or1

  1. Could you read the auto generated comment GH-38703: [C++][FS][Azure] Implement DeleteFile() #39648 (comment) and follow the description?
  2. Could you fill our pull request template?
  3. Could you rebase on main and resolve a conflict?

@av8or1
Copy link
Contributor Author

av8or1 commented Jan 18, 2024

  1. Could you read the auto generated comment [C++][FS][Azure] Implement DeleteFile() #39648 (comment) and follow the description?

Sure, I'll do that straight away.

  1. Could you fill our pull request template?

I did this at the time I opened the pull request. I suppose that it was the location within the template that I put my commentary that caused it to not be included? Dunno, but I'll revisit this and re-enter my original commentary.

  1. Could you rebase on main and resolve a conflict?

Is that how conflicts should be resolved? I have not done a rebase in quite a while, so I'll go back and take a look at exactly what that means and how to proceed. I'll update this PR as I make progress.

Thank you for the feedback.

@av8or1 av8or1 changed the title [C++][FS][Azure] Implement DeleteFile() GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY} Jan 18, 2024
@av8or1 av8or1 changed the title GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY} GH-38703: [C++][FS][Azure] Implement DeleteFile() Jan 18, 2024
@av8or1
Copy link
Contributor Author

av8or1 commented Jan 18, 2024

OK, #1 and #2 are completed. Working on the rebase ...

Note that the modifications that I made to azurefs_test.cc were limited to augmentations. That is, I didn't modify any existing tests, I just added new tests of my own. FWIW.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C++][FS][Azure] Implement DeleteFile()
2 participants