Building statement with FORCE INDEX results in wrong SQL

[SteveTherrien]

Hello,

I have query that's not parsed or built properly when FORCE INDEX is used:

SELECT a.id, a.name, b.order_id, b.total
FROM customers a
INNER JOIN orders b FORCE INDEX (idx_customer_id)
    ON a.id = b.customer_id
WHERE a.status = 'active'

The query becomes:

SELECT a.id, a.name, b.order_id, b.total
FROM customers AS `a` FORCE INDEX (idx_customer_id)
INNER JOIN orders AS `b`
WHERE a.status = 'active'

• FORCE INDEX got moved from table orders onto customers.
• The a.id = b.customer_id join condition was removed.

I see the same linting errors as #497, so this is probably related.

Example code

<?php
require __DIR__ . '/vendor/autoload.php';

$in = <<<SQL
SELECT a.id, a.name, b.order_id, b.total
FROM customers a
INNER JOIN orders b FORCE INDEX (idx_customer_id)
    ON a.id = b.customer_id
WHERE a.status = 'active'
SQL;

$parser = new PhpMyAdmin\SqlParser\Parser($in);
$statement = $parser->statements[0];
echo $statement->build();

[williamdes]

Thank you for reporting this bug !
@niconoe- this one may be interesting for you

[niconoe-]

Yeah, this one might be something I could get involved into. I do think that #497 is very similar and fixing this will fix both.
Due to the holiday period, I won't be available until Jan. 06 at least, and I'll probably have profressional chores to do first before I could take a deeper look at this. But I'll try to investigate and propose something as soon as I can 😉 .

[williamdes]

Yeah, this one might be something I could get involved into. I do think that #497 is very similar and fixing this will fix both. Due to the holiday period, I won't be available until Jan. 06 at least, and I'll probably have profressional chores to do first before I could take a deeper look at this. But I'll try to investigate and propose something as soon as I can 😉 .

Sure, thank you for all your help !

[niconoe-]

Based on the MySQL documentation about the SELECT statement and the JOIN clause, it seems that the FORCE INDEX expression is attached to a JOIN clause, not a SELECT statement.

SELECT
    …
    [FROM table_references …]
    …

table_references:
    escaped_table_reference [, escaped_table_reference] ...

escaped_table_reference: {
    table_reference
  | { OJ table_reference }
}

table_reference: {
    table_factor
  | joined_table
}

table_factor: {
    tbl_name [PARTITION (partition_names)]
        [[AS] alias] [index_hint_list]
  | [LATERAL] table_subquery [AS] alias [(col_list)]
  | ( table_references )
}

joined_table: {
    table_reference {[INNER | CROSS] JOIN | STRAIGHT_JOIN} table_factor [join_specification]
  | table_reference {LEFT|RIGHT} [OUTER] JOIN table_reference join_specification
  | table_reference NATURAL [INNER | {LEFT|RIGHT} [OUTER]] JOIN table_factor
}

(we're talking about index_hint_list here).

That means the "index_hints" should be stored on the expected item of the "join" property of the select statement, rather than being stored in the select statement directly, as it's currently the case.

The "index_hints" property from the SelectStatement class should be removed, and rather added in the JoinKeyword class. From this step, we could be able to rebuild the query with FORCE INDEX keywords at the expected location.

Unfortunately, this looks like a lot to a BC Break to me, so I don't know if you want me to go through this, @williamdes . And if that's the case, which branch should I start from?

[williamdes]

Unfortunately, this looks like a lot to a BC Break to me, so I don't know if you want me to go through this, @williamdes . And if that's the case, which branch should I start from?

Well, it really seems like a breaking change. The master branch should be the right one for such a change

[niconoe-]

Hi @williamdes

So, after a closer look, fixing this is not a BC Break as the index hint part can also be on the FROM clause, which makes sense to keep into the SelectStatement class.

Therefore, I provided you #614 which fixes (according to unit tests I added) this issue + #497, ready to be merged into 5.10.x (and can be applied to 5.11.x too).

As you can see, I also provided you #613 as I started to work on master. This version relies on new features and new classes that are only available on amster right now, so IMO, you should better consider the 2 PR as 2 differnet things.

Please tell me if something more is to do about it.
Regards,