-
-
Notifications
You must be signed in to change notification settings - Fork 110
Completing, refactoring and improving the WITH statements parser
#363
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
Completing, refactoring and improving the WITH statements parser
#363
Conversation
…g other statements aware of it Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
e979de9 to
a615674
Compare
Codecov Report
@@ Coverage Diff @@
## master #363 +/- ##
============================================
+ Coverage 95.41% 95.46% +0.05%
- Complexity 2024 2058 +34
============================================
Files 67 67
Lines 4336 4410 +74
============================================
+ Hits 4137 4210 +73
- Misses 199 200 +1
Continue to review full report at Codecov.
|
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
|
working on fixing the tests issues. |
a615674 to
08ec3fd
Compare
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
08ec3fd to
75436a9
Compare
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
427b343 to
2961130
Compare
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
3e93824 to
85a6461
Compare
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
2c1d0b4 to
199ba66
Compare
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
| SQL; | ||
| // phpcs:enable | ||
| $this->assertEquals($expected, $parser->statements[0]->build()); | ||
| $this->assertEquals('SELECT * FROM categories', $parser->statements[1]->build()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this needed to be removed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, This's removed because, previously the Parser was dealing with the CTE expression as a separate statement, that's why we had two statements. Now, the CTE is included within the WITH statement, so it should be a single statement that contain both.
That's being said, the CTE statement should exist in the with statement, which isn't happening.
$expected = <<<SQL
WITH categories(identifier, name, parent_id) AS (SELECT c.identifier, c.name, c.parent_id FROM category AS `c` WHERE c.identifier = 'a' UNION ALL SELECT c.identifier, c.name, c.parent_id FROM categories, category AS `c` WHERE c.identifier = categories.parent_id), foo AS (SELECT * FROM test)
SQL;I will work on it. I'd need to change the build functionality of the withStatement a little bit.
6b1a67b to
de90e71
Compare
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
de90e71 to
f1585a9
Compare
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
| } elseif ($this->options->has('VIEW')) { | ||
| $builtStatement = ''; | ||
| if ($this->select !== null) { | ||
| $builtStatement = $this->select->build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know why it is reported non covered ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After 2 hours of investigation, It seems like it was always reporting false positive. it's A separate bug not related to the changes proposed in this PR, but it's now shown as uncovered because previously it wasn't in a separate branch:
sql-parser/src/Statements/CreateStatement.php
Lines 468 to 471 in ed0b4ba
| return 'CREATE ' | |
| . OptionsArray::build($this->options) . ' ' | |
| . Expression::build($this->name) . ' ' | |
| . $fields . ' AS ' . ($this->select ? $this->select->build() : '') |
The bug seems to be coming from this portion of code:
sql-parser/src/Statements/CreateStatement.php
Lines 699 to 718 in ed0b4ba
| $token = $list->getNext(); // Skipping whitespaces and comments. | |
| // Parsing columns list. | |
| if (($token->type === Token::TYPE_OPERATOR) && ($token->value === '(')) { | |
| --$list->idx; // getNext() also goes forward one field. | |
| $this->fields = ArrayObj::parse($parser, $list); | |
| ++$list->idx; // Skipping last token from the array. | |
| $list->getNext(); | |
| } | |
| // Parsing the SELECT expression if the view started with it. | |
| if ( | |
| $token->type === Token::TYPE_KEYWORD | |
| && $token->keyword === 'AS' | |
| && $list->tokens[$nextidx]->type === Token::TYPE_KEYWORD | |
| && $list->tokens[$nextidx]->value === 'SELECT' | |
| ) { | |
| $list->idx = $nextidx; | |
| $this->select = new SelectStatement($parser, $list); | |
| } |
You will notice that after parsing the array options if exists, we update the $list->idx by these two lines of code:
++$list->idx; // Skipping last token from the array.
$list->getNext(); and that's totally fine, the issue is that in the code that follows, we're checking the keywords using the $token and the $nextidx but their old state, which was set before updating the idx after parsing the options.
sql-parser/src/Statements/CreateStatement.php
Lines 710 to 715 in ed0b4ba
| if ( | |
| $token->type === Token::TYPE_KEYWORD | |
| && $token->keyword === 'AS' | |
| && $list->tokens[$nextidx]->type === Token::TYPE_KEYWORD | |
| && $list->tokens[$nextidx]->value === 'SELECT' | |
| ) { |
so, we would need to update the $token and the $nextidx to their new values corresponding to the update that occured after parsing the options.
I will try to find sometime to fix it in the next weeks, but it has nothing to do with this PR.
williamdes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really awesome ! 💯
Let me know when this is finished for a final review :)
Signed-off-by: Fawzi E. Abdulfattah <[email protected]>
|
I've just noticed a test that I've duplicated by mistake, removed. |
|
Merged, thanks for your contribution! |
Signed-off-by: Maurício Meneghini Fauth <[email protected]>
Hi, this PR is completing #334 (comment) and fixing #353.
I've targeted the Master branch, because I couldn't find the
WithStatementparser in the QA branch, which's weird.Anyway, I've re-designed the
WITHparser to be able to catch and throw more appropriate errors, and made theCreateStatementandInsertStatementaware of it.I've Added a bunch of comments, because there're some complex operations/decisions have been taken, hopefully comments are clear, let me know if there's anything need to be more clearified. I've also added as much tests as I could.