Migrate most enginetests for new name resolution #1869
Conversation
max-hoffman
changed the title
sql/analyzer/experimental_rules.go
Outdated
| a.aliases[alias.Name()] = alias.Child | ||
| } | ||
|
|
||
| func (a *aliasScope) isOuterRef(name string) (bool, sql.Expression) { |
There was a problem hiding this comment.
Same comment about lower casing (or use method doc to make clear args need to be lower cased before calling this)
There was a problem hiding this comment.
I think we let aliases play as they lie, after name resolution. We have a few tests that hit that case.
|
|
||
| l.Destination = children[0] | ||
| return l, nil | ||
| ret := *l |
There was a problem hiding this comment.
You an avoid this rigmarole by declaring the receiver to a non-pointer type
There was a problem hiding this comment.
the bugs i run into are still usually because we were using a value instead of pointer type, if we overcorrect at some point we can always mass codegen all nodes as value types
| if len(exprs) != 1 { | ||
| return nil, sql.ErrInvalidChildrenNumber.New(sv, len(exprs), 1) | ||
| } | ||
| ret := *sv |
There was a problem hiding this comment.
Same comment about receiver type
sql/planbuilder/scope.go
Outdated
| @@ -42,6 +41,8 @@ type scope struct { | |||
| windowDefs map[string]*sql.WindowDefinition | |||
| // exprs collects unique expression ids for reference | |||
| exprs map[string]columnId | |||
| // subqueries is a workaround for execution limitations | |||
| subqueries map[string]scopeColumn | |||
There was a problem hiding this comment.
Name is not great, unclear what a field called subqueries with this type could be. Missing a noun or two.
There was a problem hiding this comment.
deleting, this was from a discarded prototype
| // getExpr returns a columnId if the given expression has | ||
| // been built. | ||
| func (s *scope) getExpr(name string) (columnId, bool) { | ||
| func (s *scope) getExpr(name string, checkCte bool) (columnId, bool) { |
There was a problem hiding this comment.
Probably better to have a getExprInCte method than add this extra param
There was a problem hiding this comment.
There can be CTEs in higher scopes whose naming precedence is interleaved
This doubles most of the enginetests to add versions with new name resolution. As a result testing takes ~2x as long, temporarily. Gets majority of those enginetests working with a couple bigger exceptions:
Other suites:
A couple other discoveries:
Dolt PR: dolthub/dolt#6414