Skip to content

Add support for CURRENT_TIMESTAMP() calls with parentheses #151

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 12, 2024
Merged

Add support for CURRENT_TIMESTAMP() calls with parentheses #151

merged 2 commits into from
Aug 12, 2024

Conversation

JanJakes
Copy link
Collaborator

@JanJakes JanJakes commented Aug 8, 2024 •
edited
Loading

While SQLite does support CURRENT_TIMESTAMP function calls natively, it doesn't support calling the function with parentheses in the form of CURRENT_TIMESTAMP().

This pull request removes the parentheses after CURRENT_TIMESTAMP keyword for all types of queries.

The following types of queries will now work:

SELECT
    current_timestamp AS t1,
    CURRENT_TIMESTAMP AS t2,
    current_timestamp() AS t3,
    CURRENT_TIMESTAMP() AS t4;

INSERT INTO _dates (option_name, option_value) VALUES ('first', current_timestamp());

UPDATE _dates SET option_value = CURRENT_TIMESTAMP();

DELETE FROM _dates WHERE option_value = CURRENT_TIMESTAMP();

Closes #129.

@JanJakes
Add support for CURRENT_TIMESTAMP() with parentheses
4043e6d 
While SQLite does support CURRENT_TIMESTAMP function calls natively,
it doesn't support calling the function with parentheses in the form of
CURRENT_TIMESTAMP(), so we need to remove them.
@JanJakes JanJakes mentioned this pull request Aug 8, 2024
Closed
bgrgicak
bgrgicak reviewed Aug 8, 2024
View reviewed changes
wp-includes/sqlite/class-wp-sqlite-translator.php
Comment on lines +775 to +791
$tokens = ( new WP_SQLite_Lexer( $query ) )->tokens;

// SQLite does not support CURRENT_TIMESTAMP() calls with parentheses.
// Since CURRENT_TIMESTAMP() can appear in most types of SQL queries,
// let's remove the parentheses globally before further processing.
foreach ( $tokens as $i => $token ) {
if ( WP_SQLite_Token::TYPE_KEYWORD === $token->type && 'CURRENT_TIMESTAMP' === $token->keyword ) {
$paren_open = $tokens[ $i + 1 ] ?? null;
$paren_close = $tokens[ $i + 2 ] ?? null;
if ( WP_SQLite_Token::TYPE_OPERATOR === $paren_open->type && '(' === $paren_open->value
&& WP_SQLite_Token::TYPE_OPERATOR === $paren_close->type && ')' === $paren_close->value ) {
unset( $tokens[ $i + 1 ], $tokens[ $i + 2 ] );
}
}
}
$tokens = array_values( $tokens );

Copy link
Collaborator

Choose a reason for hiding this comment

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

This code works as expected, but this seems like the wrong place to put it.
Ideally, we should clean up the brackets in the Lexer, but I couldn't find a good place to do it there.

@adamziel @aristath do you have any suggestions on where to put this code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bgrgicak I tried to move it here: 43b628d

Does that make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This definitely better but it still feels like a "random" piece of code in that function.

I will let @adamziel and @aristath decide on this one as I don't know enough about the codebase to make this decision.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, I have the opposite feedback. This Lexer class is a direct port of the PHPMyAdmin Lexer and is meant to tokenize MySQL queries. parentheses are valid in MySQL and removing them in there sounds like an abstraction leak. Not that it matters with the AST Parser/Lexer explorations coming. I'm happy to get this change in as it is as I don't think we'll stick to the current lexer in the longer term, but if you were keen to relocate it then its original location in the Translator class seem fine to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamziel If you prefer the variant from the first commit, I can just drop the last one and repush, and we can go with that for now. I guess if the new approach replaces the old one, it can be still valuable to add the test case, and we can have the functionality earlier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please and agreed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamziel Thanks! Repushed.

@JanJakes JanJakes force-pushed the current-timestamp-parentheses branch from 12ef9ae to 43b628d Compare August 8, 2024 12:03
@JanJakes JanJakes requested a review from bgrgicak August 8, 2024 12:05
@JanJakes JanJakes mentioned this pull request Aug 8, 2024
Merged
13 tasks
@bgrgicak bgrgicak requested review from adamziel and aristath August 9, 2024 13:32
@JanJakes JanJakes force-pushed the current-timestamp-parentheses branch from 43b628d to 2b1111c Compare August 12, 2024 09:33
@adamziel adamziel merged commit dd6bd6b into WordPress:develop Aug 12, 2024
16 checks passed
@adamziel
Copy link
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgrgicak bgrgicak Awaiting requested review from bgrgicak

@adamziel adamziel Awaiting requested review from adamziel

@aristath aristath Awaiting requested review from aristath

Assignees
No one assigned
Labels
None yet
Projects
Playground Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JanJakes @adamziel @bgrgicak