Name resolution correctness tests #1850
Conversation
…-mysql-server into max/name-res-simple-queries-2
…-mysql-server into max/name-res-simple-queries-2
max-hoffman
changed the title
| func TestDerivedTableOuterScopeVisibility(t *testing.T) { | ||
| enginetest.TestDerivedTableOuterScopeVisibility(t, enginetest.NewDefaultMemoryHarness()) | ||
| } | ||
|
|
||
| func TestDerivedTableOuterScopeVisibility_Experimental(t *testing.T) { |
There was a problem hiding this comment.
Does Dolt run these tests yet? Should probably start
| @@ -37,18 +37,15 @@ func (g *FrameFactoryGen) genImports() { | |||
| } | |||
|
|
|||
| func (g *FrameFactoryGen) genNewFrameFactory() { | |||
| fmt.Fprintf(g.w, "func NewFrame(ctx *sql.Context, f *ast.Frame) (sql.WindowFrame, error) {\n") | |||
| fmt.Fprintf(g.w, "func (b *PlanBuilder) NewFrame(inScope *scope, f *ast.Frame) sql.WindowFrame {\n") | |||
There was a problem hiding this comment.
As these become more solidified, it might make sense to invest a bit in some kind of templating language to make them easier to read / modify
If we're committing to code gen to this extent, it should be as easy to manage as possible, i.e. not just a sequence of printf calls
| @@ -868,7 +868,7 @@ func checkForNonAggregatedColumnReferences(w *plan.Window) error { | |||
| if agg.Window() == nil { | |||
| index, gf := findFirstWindowAggregationColumnReference(w) | |||
|
|
|||
| if index > 0 { | |||
| if index >= 0 { | |||
There was a problem hiding this comment.
Should be a test that was previously passing in error that is now passing with this bug fix
There was a problem hiding this comment.
the new tests broke this
| └─ Table | ||
| ├─ name: xy | ||
| └─ columns: [x y z] | ||
| └─ Project |
There was a problem hiding this comment.
This looks like a redundant projection, is the idea this will get collapsed during analysis so it doesn't matter?
There was a problem hiding this comment.
And can we not do that? Not clear what's causing this behavior form this change without much closer inspection
There was a problem hiding this comment.
I tried a couple deadends, this only works if we pull projection pruning forward. Most direct way of doing that is after we start moving rules onto a new format. If someone hits a perf issue because of that I can do sooner.
This fixes many of the remaining correctness tests for
TestSimpleQueries,TestsJoinOps,TestJoinPlanning,TestColumnAliases,TestDerivedTableOuterScopeVisibility,TestAmbiguousColumnResolution,TestReadOnlyVersionedQuerieswith the new name resolution strategy.Many of the query plans are slightly different but mostly equivalent. Join rearrangements and un-nesting in particular are better after this change, because I needed the transform logic work for both. There are a variety of other bugs the slight plan differences exposed that are fixed now.
This does not fix every set of enginetests, there is still a lot to do. But I'm locking in compatibility for most of the core tests to prevent backsliding.
The next follow-up is probably replacing the old name resolution. I will need to figure out if triggers, procs, prepared statements need any sort of special treatment.