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

refactor!: proof_of_sql_parser::intermediate_ast::PoSQLTimezone with sqlparser::ast::TimezoneInfo in the proof-of-sql crate #451

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

varshith257
Copy link
Contributor

@varshith257 varshith257 commented Jan 4, 2025

Please be sure to look over the pull request guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.

Please go through the following checklist

Rationale for this change

This PR addresses the need to replace the proof_of_sql_parser::PoSQLTime with the sqlparser::ast::TimezoneInfo in the proof-of-sql crate as part of a larger transition toward integrating the sqlparser .

This change is a subtask of issue #235, with the main goal of streamlining the repository by switching to the sqlparser crate and gradually replacing intermediary constructs like proof_of_sql_parser::intermediate_ast with sqlparser::ast.

What changes are included in this PR?

  • All instances of proof_of_sql_parser::PoSQLTimeZone have been replaced with sqlparser::ast::TimezoneInfo
  • Every usage of PoSQLTimeZone has been updated to maintain the original functionality, ensuring no changes to the logic or behavior.
  • The breaking change here is that TimeZoneInfo doesn't support offset and a trait has been added for it

Are these changes tested?

Yes

Part of #235
Part of #351

Note for Reviewers

  • PosqlTimeUnit has been kept intact due to sqlparser::ast not having related structures or enums to do so.
  • PosqlTimeStamp will be refactored in Literal -> ast::Expr PR as PosqlTimeStamp has been integrated with Literal::TimeStampTZ, which makes refactor redundant in this PR. I have included a util function for the refactor of TimeStamp

@varshith257 varshith257 changed the title refactor!: PosqlTimeZone to use ast::TimezoneInfo refactor!: proof_of_sql_parser::intermediate_ast::PoSQLTimezone with sqlparser::ast::TimezoneInfo in the proof-of-sql crate Jan 4, 2025
@varshith257 varshith257 marked this pull request as ready for review January 4, 2025 14:29
.gitattributes Outdated Show resolved Hide resolved
.offset(),
None => PoSQLTimeZone::utc().offset(),
},
_ => panic!("Offsets are not applicable for WithoutTimeZone or Tz variants."),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithoutTimeZone should be converted to UTC, right? Tz should be treated as WithTimeZone.

@iajoiner iajoiner requested a review from Dustin-Ray January 6, 2025 19:19
Copy link
Contributor

@Dustin-Ray Dustin-Ray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait sorry what is the point of this PR? Is it that sqlparser has a requirement for timestamps without timezone information? Also, Im not sure if this branch should merge without being deployed and smoke tested in the ProofsService crate first as this may end up causing breaking changes, but Im not really sure without trying it.

Lastly, Im not at all against removing the TimeUnit if it isnt supported by sqlparser. Removing the TimeUnit and just defaulting into nanoseconds would vastly simplify the codebase in many places, but maybe the trait introduced here mitigates that issue.

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

Successfully merging this pull request may close these issues.

3 participants