Skip to content

Conversation

JanJakes
Copy link
Member

@JanJakes JanJakes commented Jul 24, 2025

This pull request implements better support for MySQL SET statements.

It also implements additional related functionality and resolves some related issues:

  • Depth-first traversal: All node querying APIs use depth-first pre-order traversal. This traversal produces a natural ordering that corresponds to the original parsed input. It also aligns with CSS selector behavior. This is useful in situations when we need to reconstruct or retrieve a fragment of the original input with tokens in the correct order. It's used to preserve correct returned MySQL column names (see the next item).
  • Correct result column names: When no explicit column alias is provided in a select item, the driver will now ensure that the returned column name is equivalent to what MySQL infers from the input. For example, if we translate CONCAT('a', 'b') to ('a' || 'b'), we need to use the original CONCAT('a', 'b') string as the column name. In other words, SELECT CONCAT('a', 'b') will be translated to SELECT ('a' || 'b') AS ’CONCAT('a', 'b')’. This is also needed when selecting user and system variables (see the items below).
  • SET NAMES/CHARSET:SET NAMES, SET CHARSET, and SET CHARACTER SET are a no-op now, with a TODO to implement some validation logic in Charset and collation support #192.
  • User variables: Reading and writing of user variables is now implemented (SET @my_var = ..., SELECT @my_var).
  • System variables: Reading and writing of session system variables is now implemented (SET time_zone = ..., SET @@time_zone = ..., , SET @@session.time_zone = ..., SELECT @@time_zone, SELECT @@session.time_zone, etc.). The variables are not yet interpreted in any way, and they are not merged with global system variables at the moment.

@JanJakes JanJakes marked this pull request as ready for review July 24, 2025 15:42
@JanJakes JanJakes requested a review from adamziel July 25, 2025 11:13
&& WP_MySQL_Lexer::NAMES_SYMBOL === $part->id
) {
// "SET NAMES ..." is a no-op for now.
// TODO: Validate charset compatibility with UTF-8.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we call _doing_it_wrong to log a warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel We'll need to validate it somehow, but I think that should be part of #192. If we do it properly (getting a list of all commonly supported charsets and, for each one of them evaluating whether they are UTF-8 compatible or not), then maybe we can even throw an error. Or at least a warning.

We can't use directly _doing_it_wrong as that's WP-specific, but we want the driver to be more universal.

$value = str_replace( "''", "'", $value );
$value = substr( $value, 1, -1 );
/*
* Some MySQL system variables values can be set using an unquoted pure
Copy link
Collaborator

Choose a reason for hiding this comment

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

tricky! TIL

/*
* Handle ON/OFF values. They are accepted as both strings and keywords.
*
* @TODO: This is actually variable-specific and depends on the its type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

tricky! oh, MySQL 🤦

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel Yeah... So to support the system variables better in the future, we'll need a list of them, their types, etc. And for some of them, we should implement whatever behavior they modify.

$name = $this->unquote_sqlite_identifier(
$this->translate( $user_variable->get_first_child() )
);
$name = strtolower( substr( $name, 1 ) ); // Remove '@', normalize case.
Copy link
Collaborator

Choose a reason for hiding this comment

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

any chance we get a double @@ here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel I think that should be excluded by the grammar — single @ is user variable, double @ is system variable.

* @return mixed The value of the expression.
*/
public function evaluate_expression( WP_Parser_Node $node ) {
// To support expressions, we'll use a SQLite query.
Copy link
Collaborator

Choose a reason for hiding this comment

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

smart!

Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

Great work, as always @JanJakes – thank you!

Co-authored-by: Adam Zieliński <[email protected]>
@JanJakes JanJakes merged commit d2c99e7 into develop Jul 25, 2025
12 checks passed
@JanJakes JanJakes deleted the set-statement branch July 25, 2025 16:54
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