Skip to content

Conversation

@flyingsilverfin
Copy link
Member

Product change and motivation

We expose value parsing as a top-level function, and update the value literal definitions.

Comment on lines +564 to +568
date_literal = ${ date_fragment }
datetime_literal = ${ date_fragment ~ "T" ~ time }
datetime_tz_literal = ${ date_fragment ~ "T" ~ time ~ ( " " ~ iana_timezone | iso8601_timezone_offset ) }

duration_literal = ${ "P" ~ ( duration_weeks | duration_date ~ ( "T" ~ duration_time )? | "T" ~ duration_time ) ~ WB}
duration_literal = ${ "P" ~ ( duration_weeks | duration_date ~ ( "T" ~ duration_time )? | "T" ~ duration_time ) }
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need those WB at the end?

Copy link
Member

Choose a reason for hiding this comment

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

If it's put there intentionally, it seems likely that we do.

PEST doesn't seem to have a lexer/tokenization stage so this is likely to prevent a "higher priority" rule from matching a prefix that should actually match a different rule.
E.g. 2025-12-22T11:44:56Z might be greedily matched by DATE instead of DATETIME, leaving T11:44:56Z to fail the rest of the query.

Copy link
Member

@krishnangovindraj krishnangovindraj left a comment

Choose a reason for hiding this comment

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

The actual change looks fine. Please revert the WB, or establish we don't need it and remove everywhere in a separate PR.

Comment on lines +564 to +568
date_literal = ${ date_fragment }
datetime_literal = ${ date_fragment ~ "T" ~ time }
datetime_tz_literal = ${ date_fragment ~ "T" ~ time ~ ( " " ~ iana_timezone | iso8601_timezone_offset ) }

duration_literal = ${ "P" ~ ( duration_weeks | duration_date ~ ( "T" ~ duration_time )? | "T" ~ duration_time ) ~ WB}
duration_literal = ${ "P" ~ ( duration_weeks | duration_date ~ ( "T" ~ duration_time )? | "T" ~ duration_time ) }
Copy link
Member

Choose a reason for hiding this comment

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

If it's put there intentionally, it seems likely that we do.

PEST doesn't seem to have a lexer/tokenization stage so this is likely to prevent a "higher priority" rule from matching a prefix that should actually match a different rule.
E.g. 2025-12-22T11:44:56Z might be greedily matched by DATE instead of DATETIME, leaving T11:44:56Z to fail the rest of the query.

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