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

Adding over keyword to the contexts and modify the Expression allowed keywords #360

Merged
merged 1 commit into from
Oct 30, 2021

Conversation

iifawzi
Copy link
Contributor

@iifawzi iifawzi commented Oct 29, 2021

Signed-off-by: Fawzi E. Abdulfattah iifawzie@gmail.com
Hi, This PR should fix phpmyadmin/phpmyadmin#17126.

Context references:
https://dev.mysql.com/doc/refman/8.0/en/keywords.html
https://mariadb.com/kb/en/reserved-words/

I've created the PR against the master branch, because some contexts are not exist on the QA branch.

@iifawzi iifawzi force-pushed the fix-alias-with-over branch 2 times, most recently from 7dd39fe to 9d7ee01 Compare October 29, 2021 01:44
Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #360 (53f44fa) into master (949fb85) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #360   +/-   ##
=========================================
  Coverage     95.49%   95.49%           
  Complexity     2023     2023           
=========================================
  Files            67       67           
  Lines          4332     4332           
=========================================
  Hits           4137     4137           
  Misses          195      195           
Impacted Files Coverage Δ
src/Components/Expression.php 100.00% <ø> (ø)

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 949fb85...53f44fa. Read the comment docs.

@iifawzi
Copy link
Contributor Author

iifawzi commented Oct 29, 2021

Regarding a test for this, I couldn't find a way to use a specific context while generating the parser's test output. This's needed because the keyword is not present in the default context being used (5.7.0), which will lead to parse it incorrectly, Is there a way @williamdes? I've tried changing the default context, and it worked, but the CI tests would fail then.

Anyway, I've made a fake integration with PhpMyAdmin and it also worked.

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.

💯

@williamdes williamdes self-assigned this Oct 29, 2021
@williamdes williamdes added this to the 5.5.0 milestone Oct 29, 2021
@iifawzi
Copy link
Contributor Author

iifawzi commented Oct 29, 2021

The PR would fix this issue too.
#197

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.

💯
Ready for a merge, tests can be added in another PR then

@williamdes williamdes linked an issue Oct 30, 2021 that may be closed by this pull request
@williamdes williamdes merged commit f4752dc into phpmyadmin:master Oct 30, 2021
williamdes added a commit that referenced this pull request Oct 30, 2021
Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes
Copy link
Member

I did some kind of adjustment to be able to load contexts depending on the file name, it looks like ANSI cases would be using a similar system

See: 79f555a
Feel free to improve this ;)

@iifawzi
Copy link
Contributor Author

iifawzi commented Oct 31, 2021

I did some kind of adjustment to be able to load contexts depending on the file name, it looks like ANSI cases would be using a similar system

See: 79f555a Feel free to improve this ;)

Looks so neat, I’m just thinking of also supporting specifying mysql context

Maybe:

$mysqlDbPos = strpos($input, '_mysqldb_');
elseif ($mysqlDbPos !== false) {
$mysqlDbVersion = (int) substr($name, $mysqlDbPos + 9, 6);
Context::load('MySqlDb' . $mysqlDbPos);
 }

@williamdes
Copy link
Member

I did some kind of adjustment to be able to load contexts depending on the file name, it looks like ANSI cases would be using a similar system
See: 79f555a Feel free to improve this ;)

Looks so neat, I’m just thinking of also supporting specifying mysql context

Maybe:

$mysqlDbPos = strpos($input, '_mysqldb_');
elseif ($mysqlDbPos !== false) {
$mysqlDbVersion = (int) substr($name, $mysqlDbPos + 9, 6);
Context::load('MySqlDb' . $mysqlDbPos);
 }

Sure, as soon as one test case uses it, yo avoid dead code

williamdes added a commit that referenced this pull request Mar 28, 2023
Signed-off-by: William Desportes <williamdes@wdes.fr>
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.

Bug- Export View Containing rank() Over() Window Function Linter warnings: OVER() with an alias (AS)
2 participants