Skip to content

Conversation

niconoe-
Copy link
Contributor

@niconoe- niconoe- commented Feb 28, 2023

First commit contains the fix proposal, a new .in file about the partition by range, and a new .out file that was generated with current master status.

The second commit contains the update of the .out file regarding the changes brought to fix #377

From what I analyzed, the issue of #377 is that the PARTITION BY RANGE is not understood correctly by the parser as all partitions were written in the field property of the AlterOperation, which caused the syntax error on the linter on PhpMyAdmin. With this fix, the partitions are better managed in the partitions property of the AlterOperation, causing no issue with the linter.

Fixes: #377

@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (0ae405f) 97.05% compared to head (72c52dc) 97.05%.

Additional details and impacted files
@@            Coverage Diff            @@
##              5.8.x     #423   +/-   ##
=========================================
  Coverage     97.05%   97.05%           
- Complexity     2213     2217    +4     
=========================================
  Files            69       69           
  Lines          5092     5102   +10     
=========================================
+ Hits           4942     4952   +10     
  Misses          150      150           
Impacted Files Coverage Δ
src/Components/AlterOperation.php 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@williamdes williamdes changed the title Fix 377 Fix #377 - Feb 28, 2023
@williamdes williamdes changed the title Fix #377 - Fix #377 - PARTITION syntax errors Feb 28, 2023
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

This looks good, I am un sure about the whitespace skipping. @iifawzi what do you think?

@niconoe-
Copy link
Contributor Author

niconoe- commented Mar 1, 2023

This looks good, I am un sure about the whitespace skipping. @iifawzi what do you think?

Whitespaces are not skipped actually. The usage of getNext would skip them but that's why I reset the index to its previous value: to keep the whitespaces as they matter for the field assignation.
A good example of utility of this is when looking at the builder, with the unit tests PhpMyAdmin\SqlParser\Tests\Builder\AlterStatementTest::testBuilderPartitions:

  • input is ALTER TABLE t1 PARTITION BY HASH(id) PARTITIONS 8
  • output with reseting the index is ALTER TABLE t1 PARTITION BY HASH(id) PARTITIONS 8 (same as without my fix that uses the getNext) => RIGHT
  • output without reseting the indes is ALTER TABLE t1 PARTITION BY HASH(id)PARTITIONS8 => WRONG.

@williamdes williamdes added this to the 5.8.0 milestone Mar 9, 2023
@williamdes williamdes self-assigned this Mar 13, 2023
@williamdes williamdes changed the base branch from master to 5.8.x March 13, 2023 20:27
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Cool, I think something is missing for CREATE TABLE

williamdes added a commit that referenced this pull request Mar 13, 2023
Pull-request: #423

Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes williamdes merged commit f24aa16 into phpmyadmin:5.8.x Mar 13, 2023
@niconoe- niconoe- deleted the fix-377 branch June 21, 2023 13:19
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.

PARTITION syntax error Parser fails when using some keywords as column names
2 participants