You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/11/25 12:24:32 UTC
[PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
alamb opened a new pull request, #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549
Draft as it builds on https://github.com/apache/datafusion-sqlparser-rs/pull/1435
I took another pass over the code in https://github.com/apache/datafusion-sqlparser-rs/pull/1435 and found some places
I thought adding some more documentation would be helpful.
Changes
1. Updates rustdocs, and adds a bunch of examples
2. Adds links to https://github.com/apache/datafusion-sqlparser-rs/issues/1548
TODO (notes for myself)
- [ ] Discuss Location::new() vs Location::of
- [ ] Setup a testing framework
- [ ]
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549#issuecomment-2508957398
Thanks again @iffyio and @lovasoa
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549#issuecomment-2504328354
Thanks @iffyio -- I'll plan to leave this open for a few more days in case anyone else wants a chance to 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549#discussion_r1859072040
##########
README.md:
##########
@@ -123,9 +128,9 @@ SQL was first standardized in 1987, and revisions of the standard have been
published regularly since. Most revisions have added significant new features to
the language, and as a result no database claims to support the full breadth of
features. This parser currently supports most of the SQL-92 syntax, plus some
-syntax from newer versions that have been explicitly requested, plus some MSSQL,
-PostgreSQL, and other dialect-specific syntax. Whenever possible, the [online
-SQL:2016 grammar][sql-2016-grammar] is used to guide what syntax to accept.
+syntax from newer versions that have been explicitly requested, plus various
Review Comment:
drive by clarification
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549#discussion_r1856850339
##########
docs/source_spans.md:
##########
@@ -1,52 +0,0 @@
-
Review Comment:
This content is great. However, I think it may be hard to find given it is in a separate file
I moved the content into two places:
1. docs in lib.rs for stuff that is relevant to users
2. The https://github.com/apache/datafusion-sqlparser-rs/issues/1548 for things that are related to contributors
I think that will make it more likely the documentation can be discovered
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549#discussion_r1856850339
##########
docs/source_spans.md:
##########
@@ -1,52 +0,0 @@
-
Review Comment:
I moved the content into two places:
1. docs in lib.rs for stuff that is relevant to users
2. The https://github.com/apache/datafusion-sqlparser-rs/issues/1548 for things that are related to contributors
I think that will make it more likely the documentation can be discovered
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549#discussion_r1859075112
##########
src/ast/spans.rs:
##########
@@ -21,21 +21,51 @@ use super::{
/// Given an iterator of spans, return the [Span::union] of all spans.
fn union_spans<I: Iterator<Item = Span>>(iter: I) -> Span {
- iter.reduce(|acc, item| acc.union(&item))
- .unwrap_or(Span::empty())
+ Span::union_iter(iter)
}
-/// A trait for AST nodes that have a source span for use in diagnostics.
+/// Trait for AST nodes that have a source location information.
///
-/// Source spans are not guaranteed to be entirely accurate. They may
-/// be missing keywords or other tokens. Some nodes may not have a computable
-/// span at all, in which case they return [`Span::empty()`].
+/// # Notes:
+///
+/// Source [`Span`] are not yet complete. They may be missing:
+///
+/// 1. keywords or other tokens
+/// 2. span information entirely, in which case they return [`Span::empty()`].
+///
+/// Note Some impl blocks (rendered below) are annotated with which nodes are
+/// missing spans. See [this ticket] for additional information and status.
+///
+/// [this ticket]: https://github.com/apache/datafusion-sqlparser-rs/issues/1548
+///
+/// # Example
Review Comment:
I felt an example will help people understand how to use the Spans
##########
src/ast/helpers/attached_token.rs:
##########
@@ -80,3 +128,9 @@ impl From<TokenWithLocation> for AttachedToken {
AttachedToken(value)
}
}
+
+impl From<AttachedToken> for TokenWithLocation {
Review Comment:
this seemed to be an obvious gap so I just added it to avoid a papercut
##########
src/lib.rs:
##########
@@ -61,13 +64,67 @@
//! // The original SQL text can be generated from the AST
//! assert_eq!(ast[0].to_string(), sql);
//! ```
-//!
//! [sqlparser crates.io page]: https://crates.io/crates/sqlparser
//! [`Parser::parse_sql`]: crate::parser::Parser::parse_sql
//! [`Parser::new`]: crate::parser::Parser::new
//! [`AST`]: crate::ast
//! [`ast`]: crate::ast
//! [`Dialect`]: crate::dialect::Dialect
+//!
+//! # Source Spans
Review Comment:
This content is largely migrated from `docs/source_spans.md`
##########
src/tokenizer.rs:
##########
@@ -448,10 +470,25 @@ impl fmt::Debug for Location {
}
impl Location {
- pub fn of(line: u64, column: u64) -> Self {
+ /// Return an "empty" / unknown location
+ pub fn empty() -> Self {
+ Self { line: 0, column: 0 }
+ }
+
+ /// Create a new `Location` for a given line and column
+ pub fn new(line: u64, column: u64) -> Self {
Self { line, column }
}
+ /// Create a new location for a given line and column
+ ///
+ /// Alias for [`Self::new`]
+ // TODO: remove / deprecate in favor of` `new` for consistency?
Review Comment:
I found the `Location::of` API kind of confusing -- I would expect that creating a new Rust object would be done via `::new()` so I added a `new()`
I am thinking perhaps we can deprecate `Location::of` maybe to help downstream users
##########
src/tokenizer.rs:
##########
@@ -422,13 +422,35 @@ impl fmt::Display for Whitespace {
}
/// Location in input string
+///
+/// # Create an "empty" (unknown) `Location`
+/// ```
+/// # use sqlparser::tokenizer::Location;
+/// let location = Location::empty();
+/// ```
+///
+/// # Create a `Location` from a line and column
+/// ```
+/// # use sqlparser::tokenizer::Location;
+/// let location = Location::new(1, 1);
+/// ```
+///
+/// # Create a `Location` from a pair
+/// ```
+/// # use sqlparser::tokenizer::Location;
+/// let location = Location::from((1, 1));
+/// ```
#[derive(Eq, PartialEq, Hash, Clone, Copy, Ord, PartialOrd)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
pub struct Location {
- /// Line number, starting from 1
+ /// Line number, starting from 1.
Review Comment:
`///` means these comments show up in docs
##########
src/ast/spans.rs:
##########
@@ -21,21 +21,51 @@ use super::{
/// Given an iterator of spans, return the [Span::union] of all spans.
fn union_spans<I: Iterator<Item = Span>>(iter: I) -> Span {
- iter.reduce(|acc, item| acc.union(&item))
- .unwrap_or(Span::empty())
+ Span::union_iter(iter)
Review Comment:
this was useful enough I felt making it a method in `Span` with more documentation and examples would make it easier to find
##########
src/tokenizer.rs:
##########
@@ -512,16 +567,61 @@ impl Span {
}
/// Same as [Span::union] for `Option<Span>`
+ ///
/// If `other` is `None`, `self` is returned
pub fn union_opt(&self, other: &Option<Span>) -> Span {
match other {
Some(other) => self.union(other),
None => *self,
}
}
+
+ /// Return the [Span::union] of all spans in the iterator
+ ///
+ /// If the iterator is empty, an empty span is returned
+ ///
+ /// # Example
+ /// ```
+ /// # use sqlparser::tokenizer::{Span, Location};
+ /// let spans = vec![
+ /// Span::new(Location::new(1, 1), Location::new(2, 5)),
+ /// Span::new(Location::new(2, 3), Location::new(3, 7)),
+ /// Span::new(Location::new(3, 1), Location::new(4, 2)),
+ /// ];
+ /// // line 1, column 1 -> line 4, column 2
+ /// assert_eq!(
+ /// Span::union_iter(spans),
+ /// Span::new(Location::new(1, 1), Location::new(4, 2))
+ /// );
+ pub fn union_iter<I: IntoIterator<Item = Span>>(iter: I) -> Span {
Review Comment:
This was a private function and I thought it was useful enough it should be its own API and have an example, so I pulled it into a method
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549#discussion_r1860722501
##########
src/lib.rs:
##########
@@ -61,13 +64,67 @@
//! // The original SQL text can be generated from the AST
//! assert_eq!(ast[0].to_string(), sql);
//! ```
-//!
//! [sqlparser crates.io page]: https://crates.io/crates/sqlparser
//! [`Parser::parse_sql`]: crate::parser::Parser::parse_sql
//! [`Parser::new`]: crate::parser::Parser::new
//! [`AST`]: crate::ast
//! [`ast`]: crate::ast
//! [`Dialect`]: crate::dialect::Dialect
+//!
+//! # Source Spans
+//!
+//! Starting with version `0.53.0` sqlparser introduced source spans to the
+//! AST. This feature provides source information for syntax errors enables
+//! better error messages. See [issue #1548] for more information and the
+//! [`Spanned`] traite to access the spans.
+//!
+//! [issue #1548]: https://github.com/apache/datafusion-sqlparser-rs/issues/1548
+//! [`Spanned`]: ast::Spanned
+//!
+//! ## Migration Guide
+//!
+//! For the next few releases, we will be incrementally adding source spans to the
+//! AST odes, trying to minimize the impact on existing users. Some breaking
Review Comment:
```suggestion
//! AST nodes, trying to minimize the impact on existing users. Some breaking
```
##########
src/ast/mod.rs:
##########
@@ -596,9 +596,21 @@ pub enum CeilFloorKind {
/// An SQL expression of any type.
///
+/// # Semantics / Type Checking
+///
/// The parser does not distinguish between expressions of different types
-/// (e.g. boolean vs string), so the caller must handle expressions of
-/// inappropriate type, like `WHERE 1` or `SELECT 1=1`, as necessary.
+/// (e.g. boolean vs string). The the caller is responsible for detecting and
Review Comment:
```suggestion
/// (e.g. boolean vs string). The caller is responsible for detecting and
```
##########
src/tokenizer.rs:
##########
@@ -448,10 +470,25 @@ impl fmt::Debug for Location {
}
impl Location {
- pub fn of(line: u64, column: u64) -> Self {
+ /// Return an "empty" / unknown location
+ pub fn empty() -> Self {
+ Self { line: 0, column: 0 }
+ }
+
+ /// Create a new `Location` for a given line and column
+ pub fn new(line: u64, column: u64) -> Self {
Self { line, column }
}
+ /// Create a new location for a given line and column
+ ///
+ /// Alias for [`Self::new`]
+ // TODO: remove / deprecate in favor of` `new` for consistency?
Review Comment:
That sounds reasonable to me!
##########
src/lib.rs:
##########
@@ -61,13 +64,67 @@
//! // The original SQL text can be generated from the AST
//! assert_eq!(ast[0].to_string(), sql);
//! ```
-//!
//! [sqlparser crates.io page]: https://crates.io/crates/sqlparser
//! [`Parser::parse_sql`]: crate::parser::Parser::parse_sql
//! [`Parser::new`]: crate::parser::Parser::new
//! [`AST`]: crate::ast
//! [`ast`]: crate::ast
//! [`Dialect`]: crate::dialect::Dialect
+//!
+//! # Source Spans
+//!
+//! Starting with version `0.53.0` sqlparser introduced source spans to the
+//! AST. This feature provides source information for syntax errors enables
Review Comment:
```suggestion
//! AST. This feature provides source information for syntax errors, enabling
```
not sure if the edit does justice to the intended sentence, the 'enables' part seemed to read off was why
##########
src/lib.rs:
##########
@@ -61,13 +64,67 @@
//! // The original SQL text can be generated from the AST
//! assert_eq!(ast[0].to_string(), sql);
//! ```
-//!
//! [sqlparser crates.io page]: https://crates.io/crates/sqlparser
//! [`Parser::parse_sql`]: crate::parser::Parser::parse_sql
//! [`Parser::new`]: crate::parser::Parser::new
//! [`AST`]: crate::ast
//! [`ast`]: crate::ast
//! [`Dialect`]: crate::dialect::Dialect
+//!
+//! # Source Spans
+//!
+//! Starting with version `0.53.0` sqlparser introduced source spans to the
+//! AST. This feature provides source information for syntax errors enables
+//! better error messages. See [issue #1548] for more information and the
+//! [`Spanned`] traite to access the spans.
Review Comment:
```suggestion
//! [`Spanned`] trait to access the spans.
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments / docs for `Spanned` [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1549:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549#issuecomment-2508957361
🚀
Let's get this in
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org