Add WITH support #334

soyuka · 2021-03-11T11:06:21Z

Fixes: #165
Fixes: #331

codecov · 2021-03-11T11:18:33Z

Codecov Report

Merging #334 (602217e) into master (8d8336d) will increase coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head 602217e differs from pull request most recent head 4216402. Consider uploading reports for the commit 4216402 to get more accurate results

@@             Coverage Diff              @@
##             master     #334      +/-   ##
============================================
+ Coverage     95.45%   95.53%   +0.08%     
- Complexity     1969     2009      +40     
============================================
  Files            65       67       +2     
  Lines          4203     4303     +100     
============================================
+ Hits           4012     4111      +99     
- Misses          191      192       +1

Impacted Files	Coverage Δ	Complexity Δ
src/Parser.php	`100.00% <ø> (ø)`	`32.00 <0.00> (+1.00)`
src/Components/OptionsArray.php	`100.00% <100.00%> (ø)`	`61.00 <0.00> (+1.00)`
src/Components/WithKeyword.php	`100.00% <100.00%> (ø)`	`6.00 <6.00> (?)`
src/Statements/WithStatement.php	`100.00% <100.00%> (ø)`	`24.00 <24.00> (?)`
src/TokensList.php	`100.00% <100.00%> (+2.04%)`	`27.00 <0.00> (ø)`
src/Statement.php	`100.00% <0.00%> (ø)`	`82.00% <0.00%> (+2.00%)`
src/Components/Array2d.php	`100.00% <0.00%> (ø)`	`15.00% <0.00%> (ø%)`
src/Components/ArrayObj.php	`100.00% <0.00%> (ø)`	`26.00% <0.00%> (ø%)`
src/Tools/TestGenerator.php	`0.00% <0.00%> (ø)`	`24.00% <0.00%> (ø%)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d8336d...4216402. Read the comment docs.

src/Statements/WithStatement.php

tests/Parser/WithStatementTest.php

williamdes

Parfait !

src/Components/WithKeyword.php

src/Statements/WithStatement.php

Surfoo · 2021-05-10T19:09:23Z

Hello,
When do you think it will be merged?

williamdes · 2021-05-10T20:13:11Z

Hello,
When do you think it will be merged?

Hi @phpmyadmin/developers
Could someone with some expertise on the sql-parser review this PR please ?

src/Components/WithKeyword.php

ismail1432 · 2021-05-11T13:42:30Z

src/Components/WithKeyword.php

+    public static function build($component, array $options = [])
+    {
+        if (! $component instanceof WithKeyword) {
+            throw new RuntimeException('Can not build a component that is not a WithKeyword');


What about LogicException ?

LogicException are more often used outside of running code, at compile time, here as we would hit that during the "runtime" it's more appropriate. See also https://stackoverflow.com/questions/5586404/logicexception-vs-runtimeexception.

I hesitated here, and I even wanted to pick an exception of the package but ParseException wasn't a good fit.

ismail1432 · 2021-05-11T13:53:45Z

src/TokensList.php

-        }
-
-        $this->count = count($tokens);
+        $this->count = $count === -1 ? count($tokens) : $count;


You change the behavior, are you sure ? 😃

Before $this->count was equal to 0 as we return earlier

Yes, there was a bug in the parser actually. count here should always be the value of the array's length.

ismail1432 · 2021-05-11T13:57:35Z

src/Components/WithKeyword.php

+
+        $str .= ' AS (';
+
+        foreach ($component->statement->statements as $statement) {


maybe check that statement->statements is an iterable, on line 45 you just check that it's isset

As component->statement should be typed to Parser and that Parser has a public $statements = []; we're fine :).

src/Components/WithKeyword.php

src/Statements/WithStatement.php

williamdes

Thank you for the new reviews !!

Did you manually test the binaries in ./bin/ ?

soyuka · 2021-05-12T08:03:54Z

bin/lint-query outputs nothing, error code 0

bin/tokenize-query works as well

williamdes · 2021-05-12T08:55:59Z

Thank you :)
I will merge this pull-request this week-end or monday to get a chance to have a team member review it

devenbansod

Thanks for working on this @soyuka! LGTM 👍 💯

Pull-request: #334 Signed-off-by: William Desportes <williamdes@wdes.fr>

iifawzi · 2021-12-04T11:18:38Z

Hi, while working on other issues related to the WITH statements, I walked through this implementation and I noticed that it's parsing the statement based on this definition https://dev.mysql.com/doc/refman/8.0/en/with.html

    WITH [RECURSIVE]
        cte_name [(col_name [, col_name] ...)] AS (subquery)
        [, cte_name [(col_name [, col_name] ...)] AS (subquery)] ...

I'm wondering, shouldn't we also parse the part that follows the WITH statements? which will always be either SELECT (mostly), UPDATE or DELETE, instead of depending on other statements to parse them? this would help in not duplicating the code as well as throwing an appropriate errors. I think we would only need to parse the tokens remained after the WITH tokens based on the statement type (select, update, or delete). And imo it makes sense because the WITH statements wouldn't work without them.

For now, the parser will not throw any error for such queries:

WITH cte (col1, col2) AS ( SELECT 1, 2 UNION ALL SELECT 3, 4 ) FROM cte;

As i'm currently working on making the InsertStatement aware of the WITH support, I'd also need to parse the tokens remained after the WITH statement, in order to the insertStatement to behave correctly. The same should be done if I wanted to make other statements aware of it too. So why not handling it in the WithStatement from the first place? #353

What do you think? I might be missing something though.

williamdes · 2021-12-05T08:44:30Z

It seems to make sense to handle this in WithStatement and not in another statement
Would it be difficult to implement?
The version will be released at the end of the week by the way

iifawzi · 2021-12-05T12:16:51Z

Cool, I don't think it would be that difficult to implement, I will work on it. Hopefully I can manage to finish it along with the other issues related to it, before the end of the week.

soyuka force-pushed the feat/with branch from 8d5f51f to 8c5637a Compare March 11, 2021 11:17

soyuka force-pushed the feat/with branch from 8c5637a to f545e32 Compare March 11, 2021 11:24

williamdes reviewed Mar 11, 2021

View reviewed changes

src/Statements/WithStatement.php Outdated Show resolved Hide resolved

williamdes reviewed Mar 11, 2021

View reviewed changes

tests/Parser/WithStatementTest.php Outdated Show resolved Hide resolved

williamdes reviewed Mar 11, 2021

View reviewed changes

tests/Parser/WithStatementTest.php Outdated Show resolved Hide resolved

williamdes approved these changes Mar 11, 2021

View reviewed changes

williamdes added this to the 5.4.3 milestone Mar 11, 2021

williamdes requested review from MauricioFauth and devenbansod March 11, 2021 18:42

soyuka force-pushed the feat/with branch 2 times, most recently from 91d2de5 to 77ffd9d Compare March 11, 2021 19:39

soyuka commented Mar 11, 2021

View reviewed changes

soyuka force-pushed the feat/with branch from 77ffd9d to 1e1a7ff Compare March 12, 2021 07:32

williamdes force-pushed the feat/with branch 2 times, most recently from 2bee1c8 to a22dc44 Compare May 10, 2021 20:10

dunglas reviewed May 11, 2021

View reviewed changes

src/Components/WithKeyword.php Outdated Show resolved Hide resolved

ismail1432 reviewed May 11, 2021

View reviewed changes

williamdes approved these changes May 11, 2021

View reviewed changes

devenbansod approved these changes May 16, 2021

View reviewed changes

Add WITH support

4216402

williamdes force-pushed the feat/with branch from 602217e to 4216402 Compare May 16, 2021 13:06

williamdes removed the request for review from MauricioFauth May 16, 2021 13:07

williamdes self-assigned this May 16, 2021

williamdes merged commit 1b330ca into phpmyadmin:master May 16, 2021

williamdes added a commit that referenced this pull request May 16, 2021

Add a CHANGELOG entry for #165, #331

ff76ee6

Pull-request: #334 Signed-off-by: William Desportes <williamdes@wdes.fr>

This was referenced May 16, 2021

Add support for WITH clause (enhancement for syntax highlighter) #331

Closed

WITH keyword #165

Closed

Missing mysql 8 statements #336

Open

iifawzi mentioned this pull request Dec 5, 2021

Completing, refactoring and improving the WITH statements parser #363

Merged

iifawzi mentioned this pull request Apr 4, 2022

WITH...AS parse errors #373

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WITH support #334

Add WITH support #334

soyuka commented Mar 11, 2021 •

edited by williamdes

Loading

codecov bot commented Mar 11, 2021 •

edited

Loading

williamdes left a comment

Surfoo commented May 10, 2021

williamdes commented May 10, 2021

ismail1432 May 11, 2021

soyuka May 11, 2021

ismail1432 May 11, 2021

soyuka May 11, 2021

ismail1432 May 11, 2021

soyuka May 11, 2021

williamdes left a comment

soyuka commented May 12, 2021

williamdes commented May 12, 2021

devenbansod left a comment

iifawzi commented Dec 4, 2021 •

edited

Loading

williamdes commented Dec 5, 2021

iifawzi commented Dec 5, 2021


		$str .= ' AS (';

		foreach ($component->statement->statements as $statement) {

Add WITH support #334

Add WITH support #334

Conversation

soyuka commented Mar 11, 2021 • edited by williamdes Loading

codecov bot commented Mar 11, 2021 • edited Loading

Codecov Report

williamdes left a comment

Choose a reason for hiding this comment

Surfoo commented May 10, 2021

williamdes commented May 10, 2021

ismail1432 May 11, 2021

Choose a reason for hiding this comment

soyuka May 11, 2021

Choose a reason for hiding this comment

ismail1432 May 11, 2021

Choose a reason for hiding this comment

soyuka May 11, 2021

Choose a reason for hiding this comment

ismail1432 May 11, 2021

Choose a reason for hiding this comment

soyuka May 11, 2021

Choose a reason for hiding this comment

williamdes left a comment

Choose a reason for hiding this comment

soyuka commented May 12, 2021

williamdes commented May 12, 2021

devenbansod left a comment

Choose a reason for hiding this comment

iifawzi commented Dec 4, 2021 • edited Loading

williamdes commented Dec 5, 2021

iifawzi commented Dec 5, 2021

soyuka commented Mar 11, 2021 •

edited by williamdes

Loading

codecov bot commented Mar 11, 2021 •

edited

Loading

iifawzi commented Dec 4, 2021 •

edited

Loading