Skip to content

Conversation

@MazterQyou
Copy link
Member

This PR introduces a change that allows EXTRACT expression to accept date/time fields as strings, e.g. EXTRACT('year' FROM column). PostgreSQL allows those kinds of strings.
Although it also adds a related test, the test suite is half-baked; see the comment in parse_pg_extract function.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3265165931

  • 40 of 44 (90.91%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.009%) to 85.183%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/sqlparser_postgres.rs 6 7 85.71%
src/parser.rs 34 37 91.89%
Totals Coverage Status
Change from base Build 3090366902: -0.009%
Covered Lines: 8779
Relevant Lines: 10306

💛 - Coveralls

pub fn parse_extract_expr(&mut self) -> Result<Expr, ParserError> {
self.expect_token(&Token::LParen)?;
let field = self.parse_date_time_field()?;
let field_parsable_as_string =
Copy link
Member

Choose a reason for hiding this comment

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

Why you cannot move this check into parse_date_time_field method?)

Copy link
Member Author

Choose a reason for hiding this comment

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

parse_date_time_field is also used with intervals, for instance, where such string behavior would be explicitly not allowed in Postgres. INTERVAL '1' MONTH is ok, but INTERVAL '1' 'MONTH' would be an error. If I were to move the check, such incorrect syntax would be parsed as valid.

Copy link
Member

@ovr ovr left a comment

Choose a reason for hiding this comment

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

LGTM

@MazterQyou MazterQyou merged commit ef389d8 into cubesql-v0.16.0 Oct 18, 2022
@MazterQyou MazterQyou deleted the extract-field-as-string branch October 18, 2022 13:57
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.

4 participants