-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Infer prepared statement parameter types #4701
Conversation
@alamb @andygrove @NGA-TRAN Please review! |
d877d9a
to
9a63823
Compare
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.
Thanks @avantgardnerio -- this is looking very nice. I left some suggestions -- hopefully they make sense!
Sounds like you're okay with the general approach then. I'll implement a bunch of other inference types and undraft the PR 👍 |
decbe5b
to
f3b9851
Compare
Filter & join were all I could find for our test dataset of SQL queries, and those both work now. I'm undrafting this and marking it ready for review. I'll do a follow-on one for insert, update, and delete 😨 |
I plan to review this tomorrow. |
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.
Thank you @avantgardnerio -- I think this is looking close to mergeable as is. I left a few suggestions.
One way to proceed might be:
- Do any cleanup on this PR you might want to do
- Merge this PR, and file follow on tickets / PRs for:
- a) Extract the "walk all exprs in a plan" code to its own function
- b) Cover the remaining LogicalPlan types in "walk all exprs in a plan"
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.
I think it looks good enough to go in to me @avantgardnerio
Thank you -- I do think it is worth filing tickets about the feature gaps so that other people can help / realize what is missing
Specifically, that this doens't walk all logical plan nodes yet and that it doesn't handle inference for use of parameters in anything other than binary expressions
I'll file a ticket for that.
What could it infer from except those? |
I was thinking stuff like |
4ce58b9
to
845a40d
Compare
I think these are done now. |
FWIW I plan to go over this once more hopefully later today or tomorrow and if it hasn't been merged yet do so. I think it is good enough to go in as is and we can iterate on it in subsequent PRs |
c00ddd7
to
bb89ece
Compare
Benchmark runs are scheduled for baseline = 64fa312 and contender = e6a0500. e6a0500 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #4683.
Rationale for this change
Described in issue.
What changes are included in this PR?
Are these changes tested?
[x]
Are there any user-facing changes?
FlightSQL implementers can respond with types.
Additional Info
I'm opening this as a draft PR to discuss general approach. If folks are on board with it, I'll add support for a bunch more statement types and more tests.