You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "metesynnada (via GitHub)" <gi...@apache.org> on 2023/03/09 08:05:32 UTC
[GitHub] [arrow-rs] metesynnada opened a new pull request, #3824: Append API imp for LocalFileSystem
metesynnada opened a new pull request, #3824:
URL: https://github.com/apache/arrow-rs/pull/3824
# Which issue does this PR close?
Closes #3740 and #3742 since we go down a different path.
# Rationale for this change
This PR adds the `LocalFileSystem` implementation for the "append" API.
# What changes are included in this PR?
- Append API implementation for `LocalFileSystem`.
However, to simplify things, we used the `tokio::fs` feature which is not supported on `wasm32`. There is another implementation on https://github.com/synnada-ai/arrow-rs/pull/5 that alters the `LocalUpload` struct to support both staged and unstaged file writings.
# Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #3824: [ObjectStore] Add `append` API impl for `LocalFileSystem`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3824:
URL: https://github.com/apache/arrow-rs/pull/3824#issuecomment-1464891217
Thanks everyone@!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb merged pull request #3824: [ObjectStore] Add `append` API impl for `LocalFileSystem`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #3824:
URL: https://github.com/apache/arrow-rs/pull/3824
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #3824: [ObjectStore] Add `append` API impl for `LocalFileSystem`
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #3824:
URL: https://github.com/apache/arrow-rs/pull/3824#issuecomment-1464891545
Benchmark runs are scheduled for baseline = c96274a562625f091ca4c06fca21ac35ef330358 and contender = 9ce0ebb06550be943febc226f61bf083016d7652. 9ce0ebb06550be943febc226f61bf083016d7652 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/30d35e79d15c44f8b94bd680e6ff7c34...6ed64233b5fc4c3abe0d99ffe68299b0/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/c4b2346cac2c48828ee5e2eeb2794832...c5e782c157ea4ed3801f3cf7661e9c20/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/76f846d38a06406d8a31d51d65d132c5...d76e175857494d0cb9e192fb8fd20d4f/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/25f4576c4ff3456cbddbbae42f12c499...a0e993104048497a9ade45082f152215/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #3824: [ObjectStore] Add `append` API impl for `LocalFileSystem`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3824:
URL: https://github.com/apache/arrow-rs/pull/3824#issuecomment-1464735343
> > My experience of target-specific feature selection has been that they cause very strange and hard to debug problems, the ahash getrandom nonsense is a frequent source of pain.
> I wonder if we could instead add a default-disabled top-level feature, maybe append_local that gates this functionality? This would also help with flagging the functionality as not part of the standard object store contract.
I am not sure about the WASM usecase in general for object_store (as in I don't really understand why it is an an important target to support for object_store at all). If we are looking for simplifications maybe we can remove that feature support instead of making a new feature flag 🤔
I agree with @ozankabak that making a new feature flag for append seems overly complicated. I think this would be fine to merge as is.
What do you think @tustvold ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3824: [ObjectStore] Add `append` API impl for `LocalFileSystem`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3824:
URL: https://github.com/apache/arrow-rs/pull/3824#discussion_r1131220522
##########
arrow-csv/src/writer.rs:
##########
@@ -194,7 +194,7 @@ impl<W: Write> Writer<W> {
}
/// A CSV writer builder
-#[derive(Debug)]
+#[derive(Clone, Debug)]
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #3824: [ObjectStore] Add `append` API impl for `LocalFileSystem`
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #3824:
URL: https://github.com/apache/arrow-rs/pull/3824#issuecomment-1464870290
Fine with me, was just a suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ozankabak commented on pull request #3824: [ObjectStore] Add `append` API impl for `LocalFileSystem`
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #3824:
URL: https://github.com/apache/arrow-rs/pull/3824#issuecomment-1464328245
IMO the target-specific selection in this case is straightforward/surgical and I don't immediately see any potential headaches related to this in the future. Given that the only target this doesn't support this is WASM, keeping the selection seems OK to me.
BTW, I am not categorically against the `append_local` idea as it doesn't really matter for Datafusion -- we will simply enable it there. However, it seems like an avoidable complexity in general. Our initial thinking was to remove the selection when support arrives, and we already discovered [workarounds](https://github.com/synnada-ai/arrow-rs/pull/5) in case we want to support it sooner rather than later.
@alamb, @tustvold: Let us know what you think and we can finalize accordingly. Thanks for the reviews!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org