Skip to content

feat: support datetime_field as expr for bigquery #1971

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
wants to merge 1 commit into from

Conversation

chenkovsky
Copy link
Contributor

bigquery supports

SELECT DATETIME_TRUNC(CURRENT_DATETIME, WEEK(MONDAY))

WEEK(MONDAY) should be parsed to datetime_field

Comment on lines +2572 to +2577
let stmt = bigquery().verified_stmt(concat!(
"SELECT ",
"DATE_TRUNC(CURRENT_DATE, DAY), ",
"DATE_TRUNC(CURRENT_DATE, WEEK(MONDAY)) ",
"FROM my_table",
));
Copy link
Contributor

Choose a reason for hiding this comment

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

double checking the intent of the changes: my understanding is that currently given SELECT DATE_TRUNC(CURRENT_DATE, WEEK(MONDAY), WEEK(MONDAY) should be parsed as a function with name WEEK and having a single identifier MONDAY as argument? Is that indeed the case (vs e.g the parser not being able to handle such a statement)

Copy link
Contributor Author

@chenkovsky chenkovsky Jul 23, 2025

Choose a reason for hiding this comment

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

currently, DAY will be parsed as ident, week will be parsed as function. I think this is flawed.

  1. when transpiling bigquery sql to another dialect, DATE_TRUNC(CURRENT_DATE, DAY) should be transpiled to DATE_TRUNC(CURRENT_DATE,'day'), but currently, it will be transpiled into DATE_TRUNC(CURRENT_DATE, DAY).
  2. when compiling sql to logical plan, planner will try to find column called DAY, MONDAY, and function called WEEK. we should tell planner this is not function or column. I think we should convert it into str when planning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, I think the current behavior of having it parsed as a function is preferable since the construct is syntatically a function - there's quite a lot of such special scenarios across dialects and it would likely be unwieldy to cover them consistently and I think out of scope for the parser as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. if it's out of scope, i will close this pr. it can also be solved with a postprocessor.

@chenkovsky chenkovsky closed this Jul 25, 2025
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.

2 participants