Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt proof_of_sql_parser::posql_time to sqlparser #351

Open
1 task
Tracked by #356
iajoiner opened this issue Nov 7, 2024 · 2 comments
Open
1 task
Tracked by #356

Adapt proof_of_sql_parser::posql_time to sqlparser #351

iajoiner opened this issue Nov 7, 2024 · 2 comments
Labels
refactor Code cleanup or reorganization

Comments

@iajoiner
Copy link
Contributor

iajoiner commented Nov 7, 2024

Background and Motivation

This issue is a subtask in #235. In short since we plan to add more SQL features we plan to switch to the sqlparser crate which is a feature-rich, no_std-compatible parser used by DataFusion, which is part of the Arrow ecosystem.

Right now we already have some code that can convert intermediate AST from proof-of-sql-parser we use to sqlparser AST. Now we need to systematically replace instances of proof-of-sql-parser constructs with their corresponding sqlparser ones.

Changes Required

  • Adapt proof_of_sql_parser::posql_time to sqlparser

This task requires further breakdowns. Please make sure to post your plan here and discuss with @Dustin-Ray , @JayWhite2357 or I before starting your work.

@varshith257
Copy link
Contributor

varshith257 commented Dec 31, 2024

Hi @Dustin-Ray, @JayWhite2357 and @iajoiner

Here's my proposed plan for adapting proof_of_sql_parser::posql_time to sqlparser. Please review and share your thoughts or suggestions.

  1. Mapping Constructs:
  • We can replace PoSQLTimestamp with sqlparser::Expr using Value::Timestamp for TIMESTAMP and Value::TimestampTz for TIMESTAMP WITH TIME ZONE.

  • And PoSQLTimeZone with sqlparser::ast::TimezoneInfo (None, WithTimeZone, WithoutTimeZone, Tz).

  1. Conversion Utilities

We need a utility to convert PoSQLTimestamp to Expr to parse RFC 3339 and Unix epoch timestamps and map PoSQLTimeZone to TimezoneInfo.

It could look like something like this(maybe a Pseudocode):

fn convert_timestamp_to_expr(timestamp: PoSQLTime) -> Expr {
    let timezone_info = match timestamp.timezone.offset() {
        0 => TimezoneInfo::None,
        _ if timestamp.timezone.offset() > 0 => TimezoneInfo::WithTimeZone,
        _ => TimezoneInfo::WithoutTimeZone,
    };

    Expr::Value(Value::Timestamp {
        value: timestamp.timestamp.to_string(),
        timezone: Some(timezone_info),
    })
}

Questions:

  1. How strict should the mapping be between PoSQLTimeZone and TimezoneInfo?

Should we replicate exact error scenarios for invalid inputs?

  1. Do you have any other scenarios you'd like to include?

Once we finalize the plan, I’ll proceed with the implementation.

Looking forward to your feedback!

@iajoiner
Copy link
Contributor Author

iajoiner commented Jan 2, 2025

@varshith257
Thanks for your proposal!
Here are some suggestions.

I don't think we really have Timestamp without timezone.

Other things look reasonable though I need to evaluate the situation in more details today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code cleanup or reorganization
Projects
None yet
Development

No branches or pull requests

2 participants