You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/03/15 18:20:37 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #5616: Minor: Add Documentation and Examples to `TableReference`
alamb opened a new pull request, #5616:
URL: https://github.com/apache/arrow-datafusion/pull/5616
# Which issue does this PR close?
Related to https://github.com/apache/arrow-datafusion/pull/5615
# Rationale for this change
While working on https://github.com/apache/arrow-datafusion/pull/5615 I realized how complicated the TableReference has become so I felt more documentataion was in order
# What changes are included in this PR?
Add Documentation and Examples to `TableReference`
# Are these changes tested?
Yes, doc tests are tested
# Are there any user-facing changes?
More docs
--
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-datafusion] alamb commented on a diff in pull request #5616: Minor: Add Documentation and Examples to `TableReference`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5616:
URL: https://github.com/apache/arrow-datafusion/pull/5616#discussion_r1137666484
##########
datafusion/common/src/table_reference.rs:
##########
@@ -80,14 +121,18 @@ impl std::fmt::Display for TableReference<'_> {
}
impl<'a> TableReference<'a> {
- /// Convenience method for creating a `Bare` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Bare`]
+ ///
+ /// As described on [`TableReference`] this does *NO* parsing at
+ /// all -- so "Foo.Bar" stays as a reference to the table named
+ /// "Foo.Bar" (rather than "foo"."bar")
pub fn bare(table: impl Into<Cow<'a, str>>) -> TableReference<'a> {
TableReference::Bare {
table: table.into(),
}
}
- /// Convenience method for creating a `Partial` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Partial`]
Review Comment:
👍 in fd68dc057
--
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-datafusion] alamb commented on a diff in pull request #5616: Minor: Add Documentation and Examples to `TableReference`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5616:
URL: https://github.com/apache/arrow-datafusion/pull/5616#discussion_r1140048270
##########
datafusion/common/src/table_reference.rs:
##########
@@ -80,14 +121,20 @@ impl std::fmt::Display for TableReference<'_> {
}
impl<'a> TableReference<'a> {
- /// Convenience method for creating a `Bare` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Bare`]
+ ///
+ /// As described on [`TableReference`] this does *NO* parsing at
+ /// all, so "Foo.Bar" stays as a reference to the table named
+ /// "Foo.Bar" (rather than "foo"."bar")
pub fn bare(table: impl Into<Cow<'a, str>>) -> TableReference<'a> {
TableReference::Bare {
table: table.into(),
}
}
- /// Convenience method for creating a `Partial` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Partial`].
+ ///
+ /// As described on [`TableReference`] this does parsing at all.
Review Comment:
```suggestion
/// As described on [`TableReference`] this does *NO* parsing at all.
```
--
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-datafusion] alamb commented on a diff in pull request #5616: Minor: Add Documentation and Examples to `TableReference`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5616:
URL: https://github.com/apache/arrow-datafusion/pull/5616#discussion_r1137571604
##########
datafusion/common/src/table_reference.rs:
##########
@@ -35,7 +35,38 @@ impl<'a> std::fmt::Display for ResolvedTableReference<'a> {
}
}
-/// Represents a path to a table that may require further resolution
+/// [`TableReference`]s represent a multi part identifier (path) to a
+/// table that may require further resolution.
+///
+/// # Creating [`TableReference`]
+///
+/// When converting strings to [`TableReference`]s, the string is
Review Comment:
The SQL parsing behavior added in https://github.com/apache/arrow-datafusion/pull/5343 is somewhat surprising so I think it should be well documented
--
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-datafusion] alamb commented on a diff in pull request #5616: Minor: Add Documentation and Examples to `TableReference`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5616:
URL: https://github.com/apache/arrow-datafusion/pull/5616#discussion_r1140048645
##########
datafusion/common/src/table_reference.rs:
##########
@@ -80,14 +121,20 @@ impl std::fmt::Display for TableReference<'_> {
}
impl<'a> TableReference<'a> {
- /// Convenience method for creating a `Bare` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Bare`]
+ ///
+ /// As described on [`TableReference`] this does *NO* parsing at
+ /// all, so "Foo.Bar" stays as a reference to the table named
+ /// "Foo.Bar" (rather than "foo"."bar")
pub fn bare(table: impl Into<Cow<'a, str>>) -> TableReference<'a> {
TableReference::Bare {
table: table.into(),
}
}
- /// Convenience method for creating a `Partial` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Partial`].
+ ///
+ /// As described on [`TableReference`] this does parsing at all.
Review Comment:
Excellent call -- thank you
##########
datafusion/common/src/table_reference.rs:
##########
@@ -103,7 +150,9 @@ impl<'a> TableReference<'a> {
}
}
- /// Convenience method for creating a `Full` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Full`]
+ ///
+ /// As described on [`TableReference`] this does parsing at all.
Review Comment:
```suggestion
/// As described on [`TableReference`] this does *NO* parsing at all.
```
--
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-datafusion] alamb commented on pull request #5616: Minor: Add Documentation and Examples to `TableReference`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5616:
URL: https://github.com/apache/arrow-datafusion/pull/5616#issuecomment-1470559207
FYI @Jefffrey
--
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-datafusion] Jefffrey commented on a diff in pull request #5616: Minor: Add Documentation and Examples to `TableReference`
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5616:
URL: https://github.com/apache/arrow-datafusion/pull/5616#discussion_r1137688102
##########
datafusion/common/src/table_reference.rs:
##########
@@ -80,14 +121,20 @@ impl std::fmt::Display for TableReference<'_> {
}
impl<'a> TableReference<'a> {
- /// Convenience method for creating a `Bare` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Bare`]
+ ///
+ /// As described on [`TableReference`] this does *NO* parsing at
+ /// all, so "Foo.Bar" stays as a reference to the table named
+ /// "Foo.Bar" (rather than "foo"."bar")
pub fn bare(table: impl Into<Cow<'a, str>>) -> TableReference<'a> {
TableReference::Bare {
table: table.into(),
}
}
- /// Convenience method for creating a `Partial` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Partial`].
+ ///
+ /// As described on [`TableReference`] this does parsing at all.
Review Comment:
`... this does *NO* parsing at all.`
ditto for `full`
--
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-datafusion] alamb merged pull request #5616: Minor: Add Documentation and Examples to `TableReference`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5616:
URL: https://github.com/apache/arrow-datafusion/pull/5616
--
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-datafusion] Jefffrey commented on a diff in pull request #5616: Minor: Add Documentation and Examples to `TableReference`
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5616:
URL: https://github.com/apache/arrow-datafusion/pull/5616#discussion_r1137610388
##########
datafusion/common/src/table_reference.rs:
##########
@@ -189,7 +234,8 @@ impl<'a> TableReference<'a> {
}
}
- /// Converts directly into an [`OwnedTableReference`]
+ /// Converts directly into an [`OwnedTableReference`] by copying
Review Comment:
copying -> cloning maybe?
not sure how explicit need to be about cloning in documentation
##########
datafusion/common/src/table_reference.rs:
##########
@@ -80,14 +121,18 @@ impl std::fmt::Display for TableReference<'_> {
}
impl<'a> TableReference<'a> {
- /// Convenience method for creating a `Bare` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Bare`]
+ ///
+ /// As described on [`TableReference`] this does *NO* parsing at
+ /// all -- so "Foo.Bar" stays as a reference to the table named
+ /// "Foo.Bar" (rather than "foo"."bar")
pub fn bare(table: impl Into<Cow<'a, str>>) -> TableReference<'a> {
TableReference::Bare {
table: table.into(),
}
}
- /// Convenience method for creating a `Partial` variant of `TableReference`
+ /// Convenience method for creating a [`TableReference::Partial`]
Review Comment:
maybe also mention these do no parsing either for their parts? ditto for full
##########
datafusion/common/src/table_reference.rs:
##########
@@ -260,7 +310,7 @@ impl<'a> TableReference<'a> {
}
}
-/// Parse a `String` into a OwnedTableReference
+/// Parse a `String` into a OwnedTableReference as a SQL identifier.
Review Comment:
`... as a multipart SQL identifier.`
##########
datafusion/common/src/table_reference.rs:
##########
@@ -231,14 +287,8 @@ impl<'a> TableReference<'a> {
}
}
- /// Forms a [`TableReference`] by attempting to parse `s` as a multipart identifier,
- /// failing that then taking the entire unnormalized input as the identifier itself.
- ///
- /// Will normalize (convert to lowercase) any unquoted identifiers.
- ///
- /// e.g. `Foo` will be parsed as `foo`, and `"Foo"".bar"` will be parsed as
- /// `Foo".bar` (note the preserved case and requiring two double quotes to represent
- /// a single double quote in the identifier)
+ /// Forms a [`TableReference`] by parsing `s` as a multipart
+ /// identifier. See docs on [`TableReference`] for more details.
Review Comment:
`... as a multipart SQL identifier.`
--
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