Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3671577497 Thank you! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
alamb merged PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on code in PR #546:
URL:
https://github.com/apache/arrow-rs-object-store/pull/546#discussion_r2551155363
##
src/path/mod.rs:
##
@@ -255,14 +262,39 @@ impl Path {
Self::parse(decoded)
}
-/// Returns the [`PathPart`] of this [`Path`]
-pub fn parts(&self) -> impl Iterator> {
-self.raw
-.split_terminator(DELIMITER)
-.map(|s| PathPart { raw: s.into() })
+/// Returns the number of [`PathPart`]s in this [`Path`]
+///
+/// # Performance
+///
+/// This operation is `O(n)`, similar to calling `.parts().count()`
manually.
+pub fn len(&self) -> usize {
Review Comment:
Would renaming `empty` and `is_empty` into `root` and `is_root` make sense?
Edit: done now.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3618339392 That should do it. All the new stuff has tests directly covering it now. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on code in PR #546:
URL:
https://github.com/apache/arrow-rs-object-store/pull/546#discussion_r2593795469
##
src/path/mod.rs:
##
@@ -348,13 +398,22 @@ where
I: Into>,
{
fn from_iter>(iter: T) -> Self {
-let raw = T::into_iter(iter)
-.map(|s| s.into())
-.filter(|s| !s.raw.is_empty())
-.map(|s| s.raw)
-.join(DELIMITER);
+let mut this = Self::ROOT;
+this.extend(iter);
+this
+}
+}
-Self { raw }
+impl<'a, I: Into>> Extend for Path {
+fn extend>(&mut self, iter: T) {
Review Comment:
Hmm, note that I haven't added any docs on `Path` directly. Since this is a
standard trait, I expect people to find it by looking at the list of
implemented traits 🤔
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
alamb commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3617569454 > Is there a way to run the same checks that run in CI locally? Should I do anything other than `cargo test && cargo clippy`? > > When tests run, a few items end up marked as unused. I assume that's fine. Or should I fix that in main in a separate PR first? Could be a toolchain update thing. You can look at the output of the CI runs and see what commands it is running (they are all listed in various yml files in `.github` as well( -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3613410772 Is there a way to run the same checks that run in CI locally? Should I do anything other than `cargo test && cargo clippy`? When tests run, a few items end up marked as unused. I assume that's fine. Or should I fix that in main in a separate PR first? Could be a toolchain update thing. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
alamb commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3612871743 Sounds reasonable (but I haven't thought about it deeply yet) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3612500881 I was going to say I'll probably do it this weekend. But, oh look, I only meant to see how much I'll need to change and now it's done. Haha. I added `impl IntoIterator for &Path`. I think that isn't controversial since it would be symmetrical to existing `FromIterator`? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
alamb commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3612091237 We are preparing the next release in a few days so if we want to include this one we need to get it ready to go -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
alamb commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3581612314 please mark it ready for review when it is ready for another look -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
alamb commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3581611344 marking as draft while CI issues are resolved -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3581393228 Thank you, will address! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
alamb commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3575700425 CI appears to show some regressions -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
alamb commented on code in PR #546:
URL:
https://github.com/apache/arrow-rs-object-store/pull/546#discussion_r2560008656
##
src/path/mod.rs:
##
@@ -348,13 +398,22 @@ where
I: Into>,
{
fn from_iter>(iter: T) -> Self {
-let raw = T::into_iter(iter)
-.map(|s| s.into())
-.filter(|s| !s.raw.is_empty())
-.map(|s| s.raw)
-.join(DELIMITER);
+let mut this = Self::ROOT;
+this.extend(iter);
+this
+}
+}
-Self { raw }
+impl<'a, I: Into>> Extend for Path {
+fn extend>(&mut self, iter: T) {
Review Comment:
could you please add tests (perhaps doctests) using this feature explicitly
to help others find it?
##
src/path/mod.rs:
##
@@ -255,14 +269,53 @@ impl Path {
Self::parse(decoded)
}
-/// Returns the [`PathPart`] of this [`Path`]
-pub fn parts(&self) -> impl Iterator> {
-self.raw
-.split_terminator(DELIMITER)
-.map(|s| PathPart { raw: s.into() })
+/// Returns the number of [`PathPart`]s in this [`Path`]
+///
+/// This is equivalent to calling `.parts().count()` manually.
+///
+/// # Performance
+///
+/// This operation is `O(n)`.
+#[doc(alias = "len")]
+pub fn parts_count(&self) -> usize {
+self.raw.split_terminator(DELIMITER).count()
+}
+
+/// True if this [`Path`] points to the root of the store, equivalent to
`Path::from("/")`.
+///
+/// See also [`Path::root`].
+///
+/// # Example
+///
+/// ```
+/// # use object_store::path::Path;
+/// assert!(Path::from("/").is_root());
+/// assert!(Path::parse("").unwrap().is_root());
+/// ```
+pub fn is_root(&self) -> bool {
+self.raw.is_empty()
+}
+
+/// Returns the [`PathPart`]s of this [`Path`]
+pub fn parts(&self) -> PathParts<'_> {
+PathParts::new(&self.raw)
+}
+
+/// Returns a copy of this [`Path`] with the last path segment removed
+///
+/// Returns `None` if this path has zero segments.
Review Comment:
this seems pretty similar to
https://doc.rust-lang.org/std/path/struct.Path.html#method.parent
Should we call this method `parent` as well to follow existing convention?
##
src/path/mod.rs:
##
@@ -469,6 +533,23 @@ mod tests {
assert_eq!(Path::default().parts().count(), 0);
}
+#[test]
+fn parts_count() {
+assert_eq!(Path::ROOT.parts().count(), Path::ROOT.parts_count());
+
+let path = path("foo/bar/baz");
+assert_eq!(path.parts().count(), path.parts_count());
Review Comment:
could you also please add an assertion that the count is `3`?
##
src/path/mod.rs:
##
@@ -348,13 +398,22 @@ where
I: Into>,
{
fn from_iter>(iter: T) -> Self {
-let raw = T::into_iter(iter)
-.map(|s| s.into())
-.filter(|s| !s.raw.is_empty())
-.map(|s| s.raw)
-.join(DELIMITER);
+let mut this = Self::ROOT;
+this.extend(iter);
+this
+}
+}
-Self { raw }
+impl<'a, I: Into>> Extend for Path {
+fn extend>(&mut self, iter: T) {
+for s in iter.into_iter() {
+let s = s.into();
+if s.raw.is_empty() {
+continue;
+}
+self.raw.push(DELIMITER_CHAR);
Review Comment:
Can you add some tests showing how this works for:
1. Empty iterators
2. Iterator with a single element
3. Iterator with multiple elements
4. Extending an existing (non empty) Path? (it looks like maybe this will
add an extra `/` 🤔 )
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
alamb commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3575650125 > Could I have one more CI run please? Done -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3572179200 Could I have one more CI run please? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on code in PR #546:
URL:
https://github.com/apache/arrow-rs-object-store/pull/546#discussion_r2551155363
##
src/path/mod.rs:
##
@@ -255,14 +262,39 @@ impl Path {
Self::parse(decoded)
}
-/// Returns the [`PathPart`] of this [`Path`]
-pub fn parts(&self) -> impl Iterator> {
-self.raw
-.split_terminator(DELIMITER)
-.map(|s| PathPart { raw: s.into() })
+/// Returns the number of [`PathPart`]s in this [`Path`]
+///
+/// # Performance
+///
+/// This operation is `O(n)`, similar to calling `.parts().count()`
manually.
+pub fn len(&self) -> usize {
Review Comment:
Would renaming `empty` and `is_empty` into `root` and `is_root` make sense?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
tustvold commented on code in PR #546:
URL:
https://github.com/apache/arrow-rs-object-store/pull/546#discussion_r2541497644
##
src/path/mod.rs:
##
@@ -255,14 +262,39 @@ impl Path {
Self::parse(decoded)
}
-/// Returns the [`PathPart`] of this [`Path`]
-pub fn parts(&self) -> impl Iterator> {
-self.raw
-.split_terminator(DELIMITER)
-.map(|s| PathPart { raw: s.into() })
+/// Returns the number of [`PathPart`]s in this [`Path`]
+///
+/// # Performance
+///
+/// This operation is `O(n)`, similar to calling `.parts().count()`
manually.
+pub fn len(&self) -> usize {
Review Comment:
This collides with the existing len provided by the deref to str
Edit: actually we don't implement deref but still callings this parts_count
or something might be less ambiguous
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
tustvold commented on code in PR #546:
URL:
https://github.com/apache/arrow-rs-object-store/pull/546#discussion_r2541497644
##
src/path/mod.rs:
##
@@ -255,14 +262,39 @@ impl Path {
Self::parse(decoded)
}
-/// Returns the [`PathPart`] of this [`Path`]
-pub fn parts(&self) -> impl Iterator> {
-self.raw
-.split_terminator(DELIMITER)
-.map(|s| PathPart { raw: s.into() })
+/// Returns the number of [`PathPart`]s in this [`Path`]
+///
+/// # Performance
+///
+/// This operation is `O(n)`, similar to calling `.parts().count()`
manually.
+pub fn len(&self) -> usize {
Review Comment:
This collides with the existing len provided by the deref to str
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3516064178 One alternative I'm considering is a `fn prefix` that takes ownership and avoids cloning. The caller can always clone the original path first if necessary. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] `Path` improvements [arrow-rs-object-store]
Kinrany commented on PR #546: URL: https://github.com/apache/arrow-rs-object-store/pull/546#issuecomment-3516041983 Draft for now: everything should work but I'd like to know which of these changes would be welcome, if any. Thanks for maintaining this by the way 🙂 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
