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

Reduce shift-reduce conflicts in the grammar by using Precedence #10053

Merged
merged 2 commits into from
Apr 7, 2022

Conversation

GuptaManan100
Copy link
Member

Description

With ongoing work to support more and more functions in parsing, the number of shift-reduce conflicts is increasing by a lot.

This PR removes the shift-reduce conflicts occurring due to addition of special grammar rules for functions whose names are non-reserved keywords using precedence rules.

I am, in general, not a fan of precedence rules, and think that they should only be used if it is absolutely required in the grammar, since they make it much more complex and harder to read and debug. But, with each additional function's parsing being added, the shift-reduce conflicts are increasing by one. We have already reached 20 now. Having these many shift-reduce conflicts is not ideal either, since it becomes much harder to track what conflicts we had in the first place. This PR, resolves these conflicts using precedence rules by using the docs - https://www.ibm.com/docs/en/zos/2.4.0?topic=ambiguities-resolving-conflicts-by-precedence and https://www.ibm.com/docs/en/zos/2.4.0?topic=section-precedence-associativity

Eventually, when we have parsing for all the functions in MySQL and we can remove the function_call_generic grammar rules, we can also remove this precedence symbol.

Related Issue(s)

Checklist

  • Should this PR be backported? No
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@GuptaManan100
Copy link
Member Author

This PR is based on top of #9971 and should be merged after it.

@K-Kumar-01
Copy link
Contributor

LGTM 🎉

…flicts

Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

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

awesome!

Copy link
Collaborator

@systay systay left a comment

Choose a reason for hiding this comment

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

Nice stuff.

However:

Eventually, when we have parsing for all the functions in MySQL and we can remove the function_call_generic grammar rules, we can also remove this precedence symbol.

Since you can in MySQL declare your own functions, won't we forever be stuck with a function_call_generic for functions that are not native?

@harshit-gangal
Copy link
Member

Nice stuff.

However:

Eventually, when we have parsing for all the functions in MySQL and we can remove the function_call_generic grammar rules, we can also remove this precedence symbol.

Since you can in MySQL declare your own functions, won't we forever be stuck with a function_call_generic for functions that are not native?

how can you do that? just curious

@GuptaManan100
Copy link
Member Author

This is the doc for it - https://dev.mysql.com/doc/refman/8.0/en/create-procedure.html. That's a really good point @systay. So I guess, this change is here to stay then!

@GuptaManan100 GuptaManan100 merged commit eeb05bf into vitessio:main Apr 7, 2022
@GuptaManan100 GuptaManan100 deleted the reduce-conflicts branch April 7, 2022 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants