Skip to content
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.

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

ALTER TABLE … RENAME KEY … TO … fails to be parsed, and subsequent issues about it #580

Open
niconoe- opened this issue Sep 4, 2024 · 0 comments

Comments

@niconoe-
Copy link
Contributor

niconoe- commented Sep 4, 2024

If you want to parse

ALTER TABLE acme RENAME INDEX my_index TO my_key;

you'll get no trouble.

But if you want to parse

ALTER TABLE acme RENAME KEY my_key TO my_index;

you'll have the error

Parse errors: 
Missing comma before start of a new alter operation.

while both of those operations are analoguous (see https://dev.mysql.com/doc/refman/8.4/en/alter-table.html).

After a short session of digging into the sources, I found out this comes from the fact that, on AlterOperation.php, the TABLE_OPTIONS "KEY" is not allowing a variable by "INDEX" is. Therefore, I tried to allow a variable for "KEY" as well, but some unit tests were not passing because it changed some behavior beyond the variable being integrated in the options of the ADD alter operation: the field property became null.

                             'options' => PhpMyAdmin\SqlParser\Components\OptionsArray Object (
                                 'options' => Array (
                                     1 => 'ADD'
-                                    2 => 'KEY'
+                                    2 => Array (...)
                                 )
                             )
-                            'field' => PhpMyAdmin\SqlParser\Components\Expression Object (...)
+                            'field' => null
                             'partitions' => null
                             'unknown' => Array (...)
                         )

As this is the behavior we have for the "INDEX" keyword usage for similar operation, I wonder if this shouldn't be fixed this way, but I also am afraid of BCBreak if some consumers of this package are expecting to have a "field" property set in such kind of operation.

@williamdes what do you think?

Annexes

Here are different queries and, the actual status of the alter operation running in the parsed statement, with, to me, the expected status

ALTER TABLE acme ADD INDEX idx (col)

Actual

  • options: [1 => "ADD", 2 => ['name' => "INDEX", 'equals' => false, 'expr' => "idx", 'value' => "idx"]]
  • field: NULL
  • unknown: [Token "(", Token "col", Token ")"]

Expected: ✔️ same as actual.

ALTER TABLE acme ADD KEY idx (col)

Actual

  • options: [1 => "ADD", 2 => "KEY"]
  • field: Expression(column: "idx", expr: "idx")
  • unknown: [Token "(", Token "col", Token ")"]

Expected:

  • options: [1 => "ADD", 2 => ['name' => "KEY", 'equals' => false, 'expr' => "idx", 'value' => "idx"]]
  • field: NULL
  • unknown: [Token "(", Token "col", Token ")"]
ALTER TABLE acme RENAME INDEX idx TO my_index

Actual

  • options: [1 => "RENAME", 2 => ['name' => "INDEX", 'equals' => false, 'expr' => "idx", 'value' => "idx"], 3 => ['name' => "TO", 'equals' => false, 'expr' => "my_index", 'value' => "my_index"]]
  • field: NULL
  • unknown: []

Expected: ✔️ same as actual.

ALTER TABLE acme RENAME KEY idx TO my_index

Actual
Parse error

Expected:

  • options: [1 => "RENAME", 2 => ['name' => "KEY", 'equals' => false, 'expr' => "idx", 'value' => "idx"], 3 => ['name' => "TO", 'equals' => false, 'expr' => "my_index", 'value' => "my_index"]]
  • field: NULL
  • unknown: []
niconoe- added a commit to niconoe-/sql-parser that referenced this issue Sep 4, 2024
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

No branches or pull requests

1 participant