Improve typing during query planning #16310
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Andres Taylor <andres@planetscale.com>
systay
requested review from
harshit-gangal,
frouioui,
GuptaManan100 and
arthurschreiber
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
systay
added
Type: Enhancement
systay
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16310 +/- ##
==========================================
- Coverage 68.71% 68.71% -0.01%
==========================================
Files 1547 1547
Lines 198286 198338 +52
==========================================
+ Hits 136257 136281 +24
- Misses 62029 62057 +28 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Comment on lines
+273
to
+278
| // replaceAggrWithArg replaces aggregate functions with Arguments in the given expression. | ||
| // this is to prepare for sending the expression to the evalengine compiler to figure out the type | ||
| func (ctx *PlanningContext) replaceAggrWithArg(e sqlparser.Expr, cfg *evalengine.Config, env *evalengine.ExpressionEnv) (expr sqlparser.Expr, unknown bool) { | ||
| expr = sqlparser.CopyOnRewrite(e, nil, func(cursor *sqlparser.CopyOnWriteCursor) { | ||
| agg, ok := cursor.Node().(sqlparser.AggrFunc) | ||
| if !ok { |
There was a problem hiding this comment.
I see the TODO, do you want to keep this unused method before removing the TODO?
There was a problem hiding this comment.
yeah, that was my thought, but it's not important. if you think it should go, I can delete it
harshit-gangal
approved these changes
Jul 3, 2024
Signed-off-by: Andres Taylor <andres@planetscale.com>
frouioui
approved these changes
Jul 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We've long had pretty bad typing in the semantic analysis, and pretty great typing in the evalengine. This PR tries to connect the two.
The only typing during semantic analysis that we'll do is to copy information over from the VSchema, and fill the types of column names known to the vschema.
Actual typing will happen when the planner asks for type info, and not before that. The typing will be more expensive, but will only be done when it's really needed.
The PR contains TODOs about enabling the type logic for aggregate functions as well. At the moment we don't calculate these types correctly, so let's revisit once that is done.
Related Issue(s)
Checklist
Deployment Notes