Table time travel clause support, add `visit_table_factor` to Visitor #951

gruuya · 2023-08-18T14:34:36Z

This PR introduces support for temporal querying in BigQuery and MSSQL for now. It does so by extending the TableFactor::Table variant to record and propagate an optional version specification (atm only a timestamp, though we don't validate them).

Related to apache/datafusion#7292: a visitor method is also derived for TableFactor in order to facilitate walking/parsing of table versions used in a given statement, so that adequate datafusion::datasource::TableProvider implementations can be loaded prior to planning/execution.

gruuya · 2023-08-18T14:37:05Z

src/ast/query.rs

+impl Display for TableVersion {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            TableVersion::Timestamp(timestamp) => {
+                write!(f, " FOR SYSTEM_TIME AS OF '{timestamp}'")?
+            }
+        }
+        Ok(())
+    }
+}


This may be a slight problem—some other systems have completely different syntaxes here but we're missing the info on which particular dialect the table version was parsed for.

For instance, Snowflake uses AT, so if we decided to extend the support to it we wouldn't know which time travel clause to re-construct here.

I think what has happened in the past for such changes is that we would add some new variant of TableVersion that represents At perhaps

pub enum TableVersion { AsOf(...) At (..) } ...

Which would be backwards incompatible change, but then basically all changes to the AST are so it seems to be fine

One thing I wonder is whether the AsOf variant should be named explicitly as ForSystemTimeAsOf, given that e.g. CockroachDB also has an AS OF clause with a somewhat different syntax. Arguably that dialect is not supported now but if someone decides to implement it it would be a misfortunate conflict.

alamb

Thank you @gruuya -- this looks very nice. I had some naming suggestions, and to change the type from String to Expr, but otherwise I think this is very nearly ready to go

alamb · 2023-08-21T14:22:39Z

src/ast/query.rs

+impl Display for TableVersion {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            TableVersion::Timestamp(timestamp) => {
+                write!(f, " FOR SYSTEM_TIME AS OF '{timestamp}'")?
+            }
+        }
+        Ok(())
+    }
+}


I think what has happened in the past for such changes is that we would add some new variant of TableVersion that represents At perhaps

pub enum TableVersion { AsOf(...) At (..) } ...

Which would be backwards incompatible change, but then basically all changes to the AST are so it seems to be fine

alamb · 2023-08-21T14:32:55Z

src/ast/query.rs

+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum TableVersion {
+    Timestamp(String),


I recommend you make this an Expr so that it can parse expressions like

FOR SYSTEM_TIME AS OF now() - '5 minutes'

That also seems to be supported by
https://cloud.google.com/bigquery/docs/access-historical-data

FOR SYSTEM_TIME AS OF TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL 1 HOUR);

Perhaps something like

Suggested change

Timestamp(String),

AsOf(Expr),

I looked at MsSQL and it seems to allow several different phrases like BETWEEN and CONTAINED

https://learn.microsoft.com/en-us/sql/relational-databases/tables/querying-data-in-a-system-versioned-temporal-table?view=sql-server-ver16

I don't think we need to add support for any of this syntax now in this PR, but your choice of enum to represent this structure I think is prescient

Good idea, will revise it like that.

Ok, I've renamed AsOf to ForSystemTimeAsOf, and made it wrap a generic Expr instead of a String. Arguably this makes the clause more versatile then is supported by the actual external system, but that's probably for the better.

alamb · 2023-08-21T17:15:03Z

src/ast/query.rs

@@ -661,6 +662,9 @@ pub enum TableFactor {
        args: Option<Vec<FunctionArg>>,
        /// MSSQL-specific `WITH (...)` hints such as NOLOCK.
        with_hints: Vec<Expr>,
+        /// Optional version qualifier to facilitate table time-travel, as
+        /// supported by BigQuery and MSSQL.
+        version: Option<TableVersion>,


given it is so specific maybe this could be called system_time: Option<SystemTime> or something? I don't feel strongly

Ideally, I imagine this field would evolve into something capable of capturing other qualifiers beyond time, e.g. something like explicit version numbers as supported by delta-rs. To that end, I'd vote to keep it more generic than system_time.

Perhaps I could rename it to temporal_qualifier: Option<TemporalQualifier>,? Feels more SQL-y, and at the same time sufficiently generic such that it could also store explicit version number references (besides timestamps and intervals).

coveralls · 2023-08-21T17:19:01Z

Pull Request Test Coverage Report for Build 5903878906

100 of 119 (84.03%) changed or added relevant lines in 13 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.02%) to 87.329%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ast/query.rs	7	8	87.5%
src/parser/mod.rs	10	12	83.33%
src/ast/visitor.rs	0	16	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/ast/query.rs	1	85.5%

Totals
Change from base Build 5893081161:	-0.02%
Covered Lines:	16183
Relevant Lines:	18531

💛 - Coveralls

alamb · 2023-08-21T21:29:17Z

It seems like the docs CI failure is real -- specifically https://github.com/sqlparser-rs/sqlparser-rs/actions/runs/5903878906/job/16077037912?pr=951

It is pointing out the lack of a visit_table_factors function, in the same way there is a visit_statements function. Did you mean to add visit_table_factors too?

Also rename the current enum variant to ForSystemTimeAsOf to make it more explicit.

gruuya · 2023-08-22T07:28:02Z

Did you mean to add visit_table_factors too?

No, removed that line from the docs now, thanks.

alamb

Thank you @gruuya

…apache#951)

Add support for temporal querying in BigQuery and MSSQL

57e73c6

gruuya commented Aug 18, 2023

View reviewed changes

alamb changed the title ~~Table time travel clause support~~ Table time travel clause support, add visit_table_factor to Visitor Aug 21, 2023

alamb mentioned this pull request Aug 21, 2023

Table time travel support apache/datafusion#7292

Closed

alamb reviewed Aug 21, 2023

View reviewed changes

Enable arbitrary expression support in the temporal qualifier

7e5df30

Also rename the current enum variant to ForSystemTimeAsOf to make it more explicit.

alamb approved these changes Aug 22, 2023

View reviewed changes

alamb merged commit 1ea8858 into apache:main Aug 22, 2023

gruuya deleted the time-travel-support branch August 22, 2023 10:07

alamb mentioned this pull request Aug 22, 2023

Question: why is the Visitor trait limited to statements, relations & expressions? #934

Open

serprex pushed a commit to serprex/sqlparser-rs that referenced this pull request Nov 6, 2023

Table time travel clause support, add visit_table_factor to Visitor (…

e7c6d55

…apache#951)

gruuya mentioned this pull request Nov 20, 2023

Remove custom sqlparser expr visitor in favour of the native one splitgraph/seafowl#466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table time travel clause support, add `visit_table_factor` to Visitor #951

Table time travel clause support, add `visit_table_factor` to Visitor #951

gruuya commented Aug 18, 2023 •

edited

Loading

gruuya Aug 18, 2023 •

edited

Loading

alamb Aug 21, 2023

gruuya Aug 21, 2023 •

edited

Loading

alamb left a comment

alamb Aug 21, 2023

alamb Aug 21, 2023

alamb Aug 21, 2023

gruuya Aug 21, 2023

gruuya Aug 22, 2023 •

edited

Loading

alamb Aug 21, 2023

gruuya Aug 21, 2023

gruuya Aug 21, 2023

coveralls commented Aug 21, 2023

alamb commented Aug 21, 2023

gruuya commented Aug 22, 2023

alamb left a comment •

edited

Loading

Table time travel clause support, add visit_table_factor to Visitor #951

Table time travel clause support, add visit_table_factor to Visitor #951

Conversation

gruuya commented Aug 18, 2023 • edited Loading

gruuya Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gruuya Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gruuya Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 21, 2023

Pull Request Test Coverage Report for Build 5903878906

💛 - Coveralls

alamb commented Aug 21, 2023

gruuya commented Aug 22, 2023

alamb left a comment • edited Loading

Choose a reason for hiding this comment

Table time travel clause support, add `visit_table_factor` to Visitor #951

Table time travel clause support, add `visit_table_factor` to Visitor #951

gruuya commented Aug 18, 2023 •

edited

Loading

gruuya Aug 18, 2023 •

edited

Loading

gruuya Aug 21, 2023 •

edited

Loading

gruuya Aug 22, 2023 •

edited

Loading

alamb left a comment •

edited

Loading