Update comments / docs for `Spanned` by alamb · Pull Request #1549 · apache/datafusion-sqlparser-rs

alamb · 2024-11-25T12:24:29Z

I took a pass over the code in #1435 and found some places
I thought adding some more documentation would be helpful.

Changes

Updates rustdocs, and adds a bunch of examples
Adds links to [EPIC] Complete Span (source location) information / feature #1548

Follow on work (tickets)

alamb · 2024-11-25T15:48:17Z

docs/source_spans.md

@@ -1,52 +0,0 @@
-


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:

docs in lib.rs for stuff that is relevant to users

The [EPIC] Complete Span (source location) information / feature #1548 for things that are related to contributors

I think that will make it more likely the documentation can be discovered

alamb · 2024-11-26T18:44:44Z

README.md

-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


drive by clarification

alamb · 2024-11-26T18:45:48Z

src/ast/helpers/attached_token.rs

    }
 }
+
+impl From<AttachedToken> for TokenWithLocation {


this seemed to be an obvious gap so I just added it to avoid a papercut

alamb · 2024-11-26T18:47:35Z

src/ast/spans.rs

+///
+/// [this ticket]: https://github.com/apache/datafusion-sqlparser-rs/issues/1548
+///
+/// # Example


I felt an example will help people understand how to use the Spans

alamb · 2024-11-26T18:47:56Z

src/lib.rs

 //! [`ast`]: crate::ast
 //! [`Dialect`]: crate::dialect::Dialect
+//!
+//! # Source Spans


This content is largely migrated from docs/source_spans.md

alamb · 2024-11-26T18:48:16Z

src/tokenizer.rs

 #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
 pub struct Location {
-    /// Line number, starting from 1
+    /// Line number, starting from 1.


/// means these comments show up in docs

alamb · 2024-11-26T18:49:28Z

src/tokenizer.rs

+    /// Create a new location for a given line and column
+    ///
+    /// Alias for [`Self::new`]
+    // TODO: remove / deprecate in favor of` `new` for consistency?


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

That sounds reasonable to me!

alamb · 2024-11-26T18:50:39Z

src/ast/spans.rs

 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)


this was useful enough I felt making it a method in Span with more documentation and examples would make it easier to find

alamb · 2024-11-26T18:50:46Z

src/tokenizer.rs

+    ///   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 {


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

iffyio

LGTM! Thanks for the updates, the docs look great!

src/ast/mod.rs

src/lib.rs

iffyio · 2024-11-27T14:09:59Z

src/tokenizer.rs

+    /// Create a new location for a given line and column
+    ///
+    /// Alias for [`Self::new`]
+    // TODO: remove / deprecate in favor of` `new` for consistency?


That sounds reasonable to me!

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

alamb · 2024-11-27T16:39:49Z

Thanks @iffyio and @lovasoa -- I'll plan to leave this open for a few more days in case anyone else wants a chance to comment

alamb · 2024-11-30T13:08:53Z

🚀
Let's get this in

alamb · 2024-11-30T13:09:00Z

Thanks again @iffyio and @lovasoa

into update * tag 'v0.53.0' of https://github.com/apache/datafusion-sqlparser-rs: (66 commits) Run cargo fmt in derive crate Add Apache license header to spans.rs Update version to 0.53.0 and add release notes (apache#1592) Add support for ODBC functions (apache#1585) Parse `INSERT` with subquery when lacking column names (apache#1586) Support INSERT OVERWRITE INTO syntax (apache#1584) Snowflake ALTER TABLE clustering options (apache#1579) Parse Snowflake USE ROLE and USE SECONDARY ROLES (apache#1578) Add support of the ENUM8|ENUM16 for ClickHouse dialect (apache#1574) Fix displaying WORK or TRANSACTION after BEGIN (apache#1565) Support parsing optional nulls handling for unique constraint (apache#1567) Support BIT column types (apache#1577) Encapsulate CreateFunction (apache#1573) Support Databricks struct literal (apache#1542) Update comments / docs for `Spanned` (apache#1549) Support snowflake double dot notation for object name (apache#1540) `json_object('k' VALUE 'v')` in postgres (apache#1547) Increase version of sqlparser_derive from 0.2.2 to 0.3.0 (apache#1571) Support MySQL size variants for BLOB and TEXT columns (apache#1564) Rename `TokenWithLocation` to `TokenWithSpan`, in backwards compatible way (apache#1562) ...

… into sqlitedollar * 'update' of github.com:AikidoSec/datafusion-sqlparser-rs: (66 commits) Run cargo fmt in derive crate Add Apache license header to spans.rs Update version to 0.53.0 and add release notes (apache#1592) Add support for ODBC functions (apache#1585) Parse `INSERT` with subquery when lacking column names (apache#1586) Support INSERT OVERWRITE INTO syntax (apache#1584) Snowflake ALTER TABLE clustering options (apache#1579) Parse Snowflake USE ROLE and USE SECONDARY ROLES (apache#1578) Add support of the ENUM8|ENUM16 for ClickHouse dialect (apache#1574) Fix displaying WORK or TRANSACTION after BEGIN (apache#1565) Support parsing optional nulls handling for unique constraint (apache#1567) Support BIT column types (apache#1577) Encapsulate CreateFunction (apache#1573) Support Databricks struct literal (apache#1542) Update comments / docs for `Spanned` (apache#1549) Support snowflake double dot notation for object name (apache#1540) `json_object('k' VALUE 'v')` in postgres (apache#1547) Increase version of sqlparser_derive from 0.2.2 to 0.3.0 (apache#1571) Support MySQL size variants for BLOB and TEXT columns (apache#1564) Rename `TokenWithLocation` to `TokenWithSpan`, in backwards compatible way (apache#1562) ...

alamb marked this pull request as draft November 25, 2024 12:24

alamb force-pushed the alamb/doc_sql_parser branch from 2092351 to bb7ad71 Compare November 25, 2024 15:18

alamb commented Nov 25, 2024

View reviewed changes

Update README.md, add docs + examples

ec24d26

alamb force-pushed the alamb/doc_sql_parser branch from d08e6ec to ec24d26 Compare November 26, 2024 18:43

alamb commented Nov 26, 2024

View reviewed changes

alamb marked this pull request as ready for review November 26, 2024 18:50

This was referenced Nov 26, 2024

Implement Spanned to retrieve source locations on AST nodes #1435

Merged

Rename TokenWithLocation to TokenWithSpan, in backwards compatible way #1562

Merged

Create test pattern for Spans #1563

Open

lovasoa approved these changes Nov 26, 2024

View reviewed changes

iffyio approved these changes Nov 27, 2024

View reviewed changes

Apply suggestions from code review

018afff

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

alamb added 2 commits November 30, 2024 08:00

Merge remote-tracking branch 'apache/main' into alamb/doc_sql_parser

19a9278

Merge remote-tracking branch 'apache/main' into alamb/doc_sql_parser

dd9cfee

alamb merged commit 4ab3ab9 into apache:main Nov 30, 2024

alamb deleted the alamb/doc_sql_parser branch November 30, 2024 13:09

Conversation

alamb commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iffyio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Nov 30, 2024

Uh oh!

alamb commented Nov 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb commented Nov 25, 2024 •

edited

Loading

alamb Nov 25, 2024 •

edited

Loading

alamb commented Nov 27, 2024 •

edited

Loading