Skip to content

Make Expr::Interval its own struct #869

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

Closed
aprimadi opened this issue May 7, 2023 · 1 comment · Fixed by #872
Closed

Make Expr::Interval its own struct #869

aprimadi opened this issue May 7, 2023 · 1 comment · Fixed by #872

Comments

@aprimadi
Copy link
Contributor

aprimadi commented May 7, 2023

Currently Expr::Interval is part of Expr enum. Instead, we should create its own struct because it has a lot of fields.

pub enum Expr {
    ...

    /// INTERVAL literals, roughly in the following format:
    /// `INTERVAL '<value>' [ <leading_field> [ (<leading_precision>) ] ]
    /// [ TO <last_field> [ (<fractional_seconds_precision>) ] ]`,
    /// e.g. `INTERVAL '123:45.67' MINUTE(3) TO SECOND(2)`.
    ///
    /// The parser does not validate the `<value>`, nor does it ensure
    /// that the `<leading_field>` units >= the units in `<last_field>`,
    /// so the user will have to reject intervals like `HOUR TO YEAR`.
    Interval {
        value: Box<Expr>,
        leading_field: Option<DateTimeField>,
        leading_precision: Option<u64>,
        last_field: Option<DateTimeField>,
        /// The seconds precision can be specified in SQL source as
        /// `INTERVAL '__' SECOND(_, x)` (in which case the `leading_field`
        /// will be `Second` and the `last_field` will be `None`),
        /// or as `__ TO SECOND(x)`.
        fractional_seconds_precision: Option<u64>,
    },

    ...
}

I think it is better this way:

pub enum Expr {
    ...

    /// INTERVAL expression
    Interval(Interval),

    ...
}
@alamb
Copy link
Contributor

alamb commented May 8, 2023

Makes sense to me -- thank you @aprimadi -- I plan to make a new release of sqlparser sometime over the next week or so #847 -- maybe we can get this into it too

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 a pull request may close this issue.

2 participants