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

Use phpmyadmin/sql-parser #313

Closed
wants to merge 2 commits into from
Closed

Use phpmyadmin/sql-parser #313

wants to merge 2 commits into from

Conversation

xPaw
Copy link
Contributor

@xPaw xPaw commented Feb 20, 2022

This is a proof of concept.

One of the 'problems' is if the query does not parse, parser will throw it, instead of sending the supposedly invalid query to the server.

@staabm
Copy link
Owner

staabm commented Feb 20, 2022

I really love the idea of exploring sql parser stuff.

IMO we should not have sql parser types within our interface.
Would it work we hide the sql parser stuff within query simulation?

another idea: introduce a ParsingReflector which works on the result of another QueryReflector and can use knowledge about the actual query and to refine the result the wrapped reflector returned.

sprinkled sql parser dependencies across the whole codebase should be prevented if possible

@xPaw
Copy link
Contributor Author

xPaw commented Feb 20, 2022

Would it work we hide the sql parser stuff within query simulation?

You could do that, but every time you pass around the query it needs to be re-parsed in relevant spots, e.g. if you wanted to use it for getQueryType too

At least this basic exploration should push the implementation in the right direction.

@staabm
Copy link
Owner

staabm commented Feb 20, 2022

but every time you pass around the query it needs to be re-parsed

would also be interessting whether the parsing is slow .. I guess the parsing will be faster then any sql query we send to the db, so the overhead might be not relevant

@xPaw
Copy link
Contributor Author

xPaw commented Feb 20, 2022

Perhaps there should be a Statement interface added? It would contain the original query, have a method to get a simulated version etc.

@staabm
Copy link
Owner

staabm commented Feb 20, 2022

Imo a prototype should at first explore what information we get/need for sql parsing based inference, e.g.

  • can parsing inference be built separately to the existing inference? (Like a 2 phase analysis)
  • How fast is the parsing of queries
  • getQueryType is not important.. it should not be mixed with the primary use case: sql parsing type inference
  • Check whatever approach we decide on, can utilize all/most of the already known use cases in Use proper sql parser #123

after we know the answer to this questions we can decide how we should at best integrate the existing api and the sql parser api

@xPaw
Copy link
Contributor Author

xPaw commented Feb 20, 2022

Not entirely sure what you mean by 2 phases, wouldn't you want to use the parser to build a proper query for simulation?

I reverted the interface change by the way, it's just parsing in the relevant methods for now.

@staabm
Copy link
Owner

staabm commented Feb 20, 2022

Not entirely sure what you mean by 2 phases, wouldn't you want to use the parser to build a proper query for simulation?

the already existing features work pretty good without a query parser. we can get better in edge cases, but as is most things I am fine with.

Primarily I would want to use the sql parser to allow type inference in cases, where its not technical possible with the current design (meaning by looking at the result-set types as we do atm).
I don't think we should re-do all the already existing features in a query reflector which understands the query because of query parsing, but instead let one of the already existing reflectors do its magic and after that do a kind of "optimization pass" with the knowledge we win when parsing the simulated query and re-fine with that knowledge the result-set based types.

at least thats the idea how I would do it currently - but I am open for other approaches in case we see a working prototype beforehand.

the PR here - as is - is fine, but solves a "different problem". its mostly a refactoring of already existing featues.

@xPaw
Copy link
Contributor Author

xPaw commented Feb 20, 2022

the PR here - as is - is fine, but solves a "different problem". its mostly a refactoring of already existing featues.

That's what I thought would be a good and simple way to bring the parser in, specifically to get rid of the regex based stuff for query manipulation.

@staabm
Copy link
Owner

staabm commented Feb 20, 2022

That's what I thought would be a good and simple way to bring the parser in, specifically to get rid of the regex based stuff for query manipulation.

yep thats fine. a "side effect" of having a parser, but not my primary motivation.

@xPaw
Copy link
Contributor Author

xPaw commented Feb 20, 2022

sql-parser does parse out expressions, so it is trivial to get the function (like COUNT), I pushed a basic example on how it could work (disregarding the place where I put it)

@staabm
Copy link
Owner

staabm commented Feb 20, 2022

Cool, thats a first step.

When thinking about a more involved case like min/max we would need something like

Parse the query to a expr like Min(colA, colB)
-> find the phpstan type for colA
-> find the phpstan type for colB
-> combine the 2 types and calculate the resulting phpstan type (e.g. let phpstan combine the integer range types)

The challenge would be on how to get all this information together

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.

2 participants