Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions tests/WP_SQLite_Driver_Translation_Tests.php
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,31 @@
);
}

/**
* Test for Query Error: ambiguous column name in ORDER BY clause

Check failure on line 97 in tests/WP_SQLite_Driver_Translation_Tests.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed

Check failure on line 97 in tests/WP_SQLite_Driver_Translation_Tests.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed
* @see https://github.com/wordpress/sqlite-database-integration/issues/228

Check failure on line 98 in tests/WP_SQLite_Driver_Translation_Tests.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed

Check failure on line 98 in tests/WP_SQLite_Driver_Translation_Tests.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed
*/
public function testSelectOrderByAmbiguousColumnResolution(): void {
// This query works in MySQL, but in SQLite `ORDER BY name` is ambiguous. Let's confirm
// the SQLite driver can handle it.
$this->assertQuery(
'SELECT `t1`.`name` FROM `t1` JOIN `t2` ON `t2`.`t1_id` = `t1`.`id` ORDER BY `t1`.`name`',
'SELECT t1.name FROM t1 JOIN t2 ON t2.t1_id = t1.id ORDER BY name'
);

// Test with explicit aliases - should resolve to alias name
$this->assertQuery(
'SELECT `t1`.`name` AS `t1_name` FROM `t1` JOIN `t2` ON `t2`.`t1_id` = `t1`.`id` ORDER BY `t1_name`',
'SELECT t1.name AS t1_name FROM t1 JOIN t2 ON t2.t1_id = t1.id ORDER BY `t1_name`'
);

// It should also work with multiple ambiguous columns.
$this->assertQuery(
'SELECT `t1`.`id` , `t1`.`name` FROM `t1` JOIN `t2` ON `t2`.`t1_id` = `t1`.`id` ORDER BY `t1`.`id`, `t1`.`name`',
'SELECT t1.id, t1.name FROM t1 JOIN t2 ON t2.t1_id = t1.id ORDER BY id, name'
);
}

public function testInsert(): void {
$this->assertQuery(
'INSERT INTO `t` ( `c` ) VALUES ( 1 )',
Expand Down
205 changes: 204 additions & 1 deletion wp-includes/sqlite-ast/class-wp-sqlite-driver.php
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,24 @@
'%y' => '%y',
);

/**
* A stack of column maps for resolving unqualified ORDER BY names to select-item expressions.
*

Check failure on line 231 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed

Check failure on line 231 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed
* Every query is a tree consisting of zero or more sub-SELECT queries. This stack keeps track
* of selected columns per SELECT. This map is then used to resolve ambiguities in the ORDER BY

Check failure on line 233 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed

Check failure on line 233 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed
* clause, e.g.
*
* SELECT name from t1 JOIN t2 ORDER BY name
*

Check failure on line 237 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed

Check failure on line 237 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed
* Becomes
*

Check failure on line 239 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed

Check failure on line 239 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed
* SELECT name from t1 JOIN t2 ORDER BY t1.name
*

Check failure on line 241 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed

Check failure on line 241 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Tabs must be used to indent lines; spaces are not allowed
* @see https://github.com/wordpress/sqlite-database-integration/issues/228
* @var array<int, array<string, string|null>> Stack of frames: name(lowercase) → expression|null
*/
private $select_output_name_to_ordinal_stack = array();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like this name. Maybe $selected_columns_stack?


/**
* A map of MySQL data types to implicit default values for non-strict mode.
*
Expand Down Expand Up @@ -2513,6 +2531,11 @@
$rule_name = $node->rule_name;
switch ( $rule_name ) {
case 'querySpecification':
// Start a new SELECT-output frame so ORDER BY can resolve names against its related
// SELECT column list (and not, e.g., an unrelated subquery).
// We defer popping until the enclosing queryExpression finishes to ensure ORDER BY sees it.
$this->select_output_name_to_ordinal_stack[] = array();

// Translate "HAVING ..." without "GROUP BY ..." to "GROUP BY 1 HAVING ...".
if ( $node->has_child_node( 'havingClause' ) && ! $node->has_child_node( 'groupByClause' ) ) {
$parts = array();
Expand All @@ -2528,6 +2551,17 @@
return implode( ' ', $parts );
}
return $this->translate_sequence( $node->get_children() );
case 'queryExpression':
// Pop the recorded SELECT column names to avoid using them when translating a higher-level
// ORDER BY clause.
$depth_before = count( $this->select_output_name_to_ordinal_stack );
$result = $this->translate_sequence( $node->get_children() );

Check warning on line 2558 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 7 spaces but found 1 space

Check warning on line 2558 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 7 spaces but found 1 space
while ( count( $this->select_output_name_to_ordinal_stack ) > $depth_before ) {
array_pop( $this->select_output_name_to_ordinal_stack );
}
return $result;
case 'orderClause':
return $this->translate_order_clause_with_select_alias_resolution( $node );
case 'qualifiedIdentifier':
case 'tableRefWithWildcard':
$parts = $node->get_descendant_nodes( 'identifier' );
Expand Down Expand Up @@ -2926,6 +2960,159 @@
return $this->translate_sequence( $node->get_children() );
}

/**
* Record a SELECT columns list to resolve ambiguous ORDER BY terms.
*
* @see https://github.com/WordPress/sqlite-database-integration/issues/228
* @param string $output_name Output column name (alias, inferred name).
* @param string|null $full_column_expression Translated expression that produces the column value
* If null, defaults to quoting the name as an identifier.
* @return void
*/
private function record_select_column_name_for_ambiguous_column_resolution( string $output_name, ?string $full_column_expression = null ): void {
if ( empty( $this->select_output_name_to_ordinal_stack ) ) {
return;
}
$normalized = strtolower( $output_name );

Check warning on line 2976 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 2 spaces but found 1 space

Check warning on line 2976 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 2 spaces but found 1 space
$frame_index = count( $this->select_output_name_to_ordinal_stack ) - 1;
$frame = $this->select_output_name_to_ordinal_stack[ $frame_index ];

Check warning on line 2978 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 7 spaces but found 1 space

Check warning on line 2978 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 7 spaces but found 1 space
if ( array_key_exists( $normalized, $frame ) ) {
$frame[ $normalized ] = null; // ambiguous
} else {
// Store the translated expression used in the SELECT list.
$frame[ $normalized ] = $full_column_expression ?? ('`' . $output_name . '`');

Check failure on line 2983 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Expected 1 space before close parenthesis; 0 found

Check failure on line 2983 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Expected 1 space after open parenthesis; 0 found

Check failure on line 2983 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Expected 1 space before close parenthesis; 0 found

Check failure on line 2983 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Expected 1 space after open parenthesis; 0 found
}
$this->select_output_name_to_ordinal_stack[ $frame_index ] = $frame;
}

/**
* Translate ORDER BY and resolve ambiguous unqualified names against the current SELECT list.
*
* ## Problem
*

Check failure on line 2992 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Whitespace found at end of line

Check failure on line 2992 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Whitespace found at end of line
* MySQL allows ORDER BY to reference output columns by their column names (or aliases)
* without qualification (e.g., ORDER BY name), and it resolves the reference against
* the SELECT list when unambiguous. SQLite requires the term to be either a positional
* index or a resolvable expression (e.g., t1.name) and will error on ambiguity.
*
* For example, given the following tables and data:
*
* ```sql
* CREATE TABLE t1 (id INT, name TEXT);
* CREATE TABLE t2 (t1_id INT, name TEXT);
*
* INSERT INTO t1 (id, name) VALUES (1, "T1 A");
* INSERT INTO t1 (id, name) VALUES (2, "T1 B");
* INSERT INTO t2 (t1_id, name) VALUES (1, "T2 B");
* INSERT INTO t2 (t1_id, name) VALUES (2, "T2 A");
* ```
*
* The following queries **error in SQLite but not in MySQL**:
*
* ```sql
* SELECT t1.name -- name column used for ORDER BY in MySQL
* FROM t1
* JOIN t2 ON t2.t1_id = t1.id
* ORDER BY name;
* -- [MySQL] T1 A, T1 B
* -- [SQLite] Query Error: ambiguous column name: name
*
* SELECT t2.name -- name column used for ORDER BY in MySQL
* FROM t1
* JOIN t2 ON t2.t1_id = t1.id
* ORDER BY name;
* -- [MySQL] T2 A, T2 B
* -- [SQLite] Query Error: ambiguous column name: name
* ```
*
* ## Solution
*
* When we see a non-fully qualified column in an `ORDER BY` clause and the same column
* is fully qualified and non-ambiguous in the `SELECT` list, we can use the same full
* qualifier in the `ORDER BY` clause.
*
* For example, the above queries are rewritten as:
*
* ```sql
* SELECT t1.name
* FROM t1
* JOIN t2 ON t2.t1_id = t1.id
* ORDER BY t1.name;
*
* SELECT t2.name
* FROM t1
* JOIN t2 ON t2.t1_id = t1.id
* ORDER BY t2.name;
* -- [MySQL] T2 A, T2 B
* -- [SQLite] T2 A, T2 B
* ```
*
* ## Limitations
*
* This solution is limited to simple unqualified ORDER BY terms. Complex computed
* expressions, wildcards, and other non-simple terms are not supported.
*
* @param WP_Parser_Node $order_clause The orderClause node.
* @return string The translated ORDER BY clause.
*/
private function translate_order_clause_with_select_alias_resolution( WP_Parser_Node $order_clause ): string {
$order_list = $order_clause->get_first_child_node( 'orderList' );
if ( null === $order_list ) {
return $this->translate_sequence( $order_clause->get_children() );
}
$parts = array( 'ORDER BY' );

Check warning on line 3063 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 7 spaces but found 1 space

Check warning on line 3063 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 7 spaces but found 1 space
$order_items = array();
foreach ( $order_list->get_child_nodes( 'orderExpression' ) as $order_expr ) {
$expr_nodes = $order_expr->get_children();
$expr = $this->translate( $expr_nodes[0] );

Check warning on line 3067 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 7 spaces but found 1 space

Check warning on line 3067 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 7 spaces but found 1 space
$direction = null;

Check warning on line 3068 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 2 spaces but found 1 space

Check warning on line 3068 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 2 spaces but found 1 space
if ( isset( $expr_nodes[1] ) ) {
$direction = $this->translate( $expr_nodes[1] );
}

$resolved = $this->maybe_resolve_unqualified_order_term_to_select_expression( $expr );
if ( null !== $resolved ) {
$expr = (string) $resolved;
}
$order_items[] = trim( $expr . ( $direction ? ( ' ' . $direction ) : '' ) );
}
$parts[] = implode( ', ', $order_items );
return implode( ' ', $parts );
}

/**
* Try to resolve an ORDER BY term like `name` to a select-item ordinal when uniquely present.
*
* @param string $translated_expr The already-translated expression string for the term.
* @return int|null Ordinal (1-based) if resolved; null otherwise.
*/
private function maybe_resolve_unqualified_order_term_to_select_expression( string $translated_expr ): ?string {
if ( empty( $this->select_output_name_to_ordinal_stack ) ) {
return null;
}
$frame = $this->select_output_name_to_ordinal_stack[ count( $this->select_output_name_to_ordinal_stack ) - 1 ];
// Only consider simple unqualified identifiers: `name` or name
$trimmed = trim( $translated_expr );
// Remove backticks if present for lookup purposes.
if ( strlen( $trimmed ) >= 2 && $trimmed[0] === '`' && substr( $trimmed, -1 ) === '`' ) {
$key = strtolower( substr( $trimmed, 1, -1 ) );
} else {
$key = strtolower( $trimmed );
}
// If expression contains a dot, function call, parentheses, or spaces, do not resolve.
if ( strpbrk( $key, ".() ") !== false ) {
return null;
}
if ( ! array_key_exists( $key, $frame ) ) {
return null;
}
$select_expr = $frame[ $key ];
if ( null === $select_expr ) {
return null; // ambiguous
}
return (string) $select_expr;
}

/**
* Translate a MySQL LIKE expression to SQLite.
*
Expand Down Expand Up @@ -3219,6 +3406,11 @@
// When an explicit alias is provided, we can use it as is.
$alias = $node->get_first_child_node( 'selectAlias' );
if ( $alias ) {
// Record explicit alias in the current select output map.
$this->record_select_column_name_for_ambiguous_column_resolution(
$this->unquote_sqlite_identifier( $this->translate( $alias->get_first_child() ) ),
$item
);
return $item;
}

Expand All @@ -3233,7 +3425,17 @@
* In this case, SQLite uses the same logic as MySQL, so using the value
* as is without adding an explicit alias will produce the correct result.
*/
$column_ref = $node->get_first_descendant_node( 'columnRef' );
$column_ref = $node->get_first_descendant_node( 'columnRef' );
if ( $column_ref ) {
// Record inferred output name from column reference (the final column name part).
$identifiers = $column_ref->get_descendant_nodes( 'identifier' );
if ( ! empty( $identifiers ) ) {
// For qualified references like t1.name, the last identifier is the column name.
$last_identifier = end( $identifiers );
$column_name = $this->unquote_sqlite_identifier( $this->translate( $last_identifier ) );

Check warning on line 3435 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 5 spaces but found 1 space

Check warning on line 3435 in wp-includes/sqlite-ast/class-wp-sqlite-driver.php

View workflow job for this annotation

GitHub Actions / Check code style

Equals sign not aligned with surrounding assignments; expected 5 spaces but found 1 space
$this->record_select_column_name_for_ambiguous_column_resolution( $column_name, $item );
}
}
$is_column_ref = $column_ref && $item === $this->translate( $column_ref );
if ( $is_column_ref ) {
return $item;
Expand All @@ -3256,6 +3458,7 @@
// let's avoid unnecessary aliases ("SELECT `id` AS `id` FROM t").
return $item;
}
$this->record_select_column_name_for_ambiguous_column_resolution( $raw_alias );
return sprintf( '%s AS %s', $item, $alias );
}

Expand Down
Loading