GH-32438: [C++] Create target file when OpenAppendStream targets a nonexistent path - #51321
Open
mohammadnaqvi04 wants to merge 2 commits into
Open
GH-32438: [C++] Create target file when OpenAppendStream targets a nonexistent path#51321mohammadnaqvi04 wants to merge 2 commits into
mohammadnaqvi04 wants to merge 2 commits into
Conversation
…ppendStream targets a nonexistent path
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
FileSystem::OpenAppendStreamis documented to create the target file if it doesn't already exist. On HDFS this instead raises a not-found error.HadoopFileSystemImpl::OpenWritablealways opens in append mode when asked to append, but Hadoop'sFileSystem.append()requires the file to already exist, unlike a local append-mode open. See #32438.What changes are included in this PR?
OpenWritablenow only sets append mode when the target already exists; otherwise it falls back to a plain create, matching every other backend. Adds an existence check against the NameNode before an append-mode open, since that's the only way to know which mode to request.TestHadoopFileSystemGeneric::allow_append_to_new_file's override tofalse, seemingly added when HDFS was first wired into the shared filesystem test suite. HDFS now runs the same append-to-new-file check as every other backend.Are these changes tested?
Yes. With the override removed,
TestHadoopFileSystemGeneric.OpenAppendStreamfails on the unpatched code with the same not-found error from the issue, and passes with this change, run against the Docker-based HDFS test setup (ci/scripts/integration_hdfs.sh). The rest of the HDFS C++ suite andpyarrow.tests.test_fspass with no regressions.Unrelated to this change:
ci/scripts/install_minio.shcurrently fails for anyone building the CI images locally, since MinIO discontinued the binary distribution it fetches from. I patched around it locally to get a working build. If this is fixed properly upstream and CI behaves differently as a result, I wouldn't have had visibility into that from my local setup.Are there any user-facing changes?
Yes: appending to a nonexistent file on HDFS now succeeds and creates the file, instead of raising an error.
This PR contains a "Critical Fix". It fixes a case where an operation documented to succeed instead raised an error.