Explicit Analyzer/Builder Overrides #3303
Conversation
|
Also, right now tests are going to fail since they don't include the |
zachmu
left a comment
zachmu
left a comment
There was a problem hiding this comment.
General idea is sound, but let's avoid introducing more globals if we can help it
Hydrocharged
force-pushed
the
daylon/hooks
branch
from
7830530 to
97ca0b8
Compare
zachmu
left a comment
zachmu
left a comment
There was a problem hiding this comment.
My main comment is don't put this new information in the context in order to get it to where it needs to go in non-mainline parse contexts. The catalog is a better target for that.
Otherwise I think this plan seems sound.
I think the hooks stuff is harmless enough to leave in with this batch of changes.
sql/planbuilder/builder.go
Outdated
|
|
||
| // BuilderOverrides contains functions and variables that can replace, supplement, or override functionality within the | ||
| // builder. | ||
| type BuilderOverrides struct { |
There was a problem hiding this comment.
No reason for this to be a struct type, just declare an interface with this method
| err := sql.ErrTableColumnNotFound.New(tblName, colName) | ||
| b.handleErr(err) | ||
| } else if b.overrides.ParseTableAsColumn != nil && inScope.hasTable(colName) { | ||
| scopeTableCols := inScope.resolveColumnAsTable(dbName, colName) |
There was a problem hiding this comment.
This logic should probably be entirely in the override handler. Would require exporting some methods from scope and other objects but that's fine.
There was a problem hiding this comment.
Going to address this on a future pass?
Hydrocharged
force-pushed
the
daylon/hooks
branch
from
97ca0b8 to
843c878
Compare
zachmu
left a comment
zachmu
left a comment
There was a problem hiding this comment.
Looking a lot better, thanks for the changes.
The only additional structural change you should make is to make ExtensionHook into an interface rather than separate structs with func fields.
| ctx, span := s.tracer.Start(ctx, "query") | ||
|
|
||
| context := s.ctxFactory( | ||
| createdCtx := s.ctxFactory( |
| } | ||
|
|
||
| // CreateTable contains hooks related to CREATE TABLE statements. These will take a *plan.CreateTable. | ||
| type CreateTable struct { |
There was a problem hiding this comment.
These are pretty redundant after the latest refactoring. Seems like an ExecutionHook interface with these two methods would satisfy all these without the repetition. And in general, whenever you func fields in a struct, that's a good sign it should be an interface already.
| // It defaults to MysqlParser. | ||
| var GlobalParser Parser = NewMysqlParser() | ||
| // DefaultMySQLParser is the default MySQL parser. | ||
| var DefaultMySQLParser Parser = NewMysqlParser() |
There was a problem hiding this comment.
Aren't these still DefaultParser and DefaultSchemaFormatter? Or are they only referenced in situations where it's clear they're the MySQL one?
|
|
||
| // New takes ctx, catalog, event scheduler, and parser. If the parser is nil, then the default parser is used (which | ||
| // will be the MySQL parser unless modified). | ||
| func New(ctx *sql.Context, cat sql.Catalog, es sql.EventScheduler) *Builder { |
There was a problem hiding this comment.
Comment needs an update. Also it seems like the event scheduler should probably live on the catalog too at this point but that's for another day.
| return nil, fmt.Errorf("DefaultStringToExpression expected *sqlparser.AliasedExpr but received %T", parserSelect.SelectExprs[0]) | ||
| } | ||
| proj, _, err := Parse(ctx, nil, fmt.Sprintf("SELECT %s", aliasedExpr.Expr)) | ||
| // TODO: this needs to take a catalog |
There was a problem hiding this comment.
Apparently this doesn't break Doltgres? Where is it even used?
| err := sql.ErrTableColumnNotFound.New(tblName, colName) | ||
| b.handleErr(err) | ||
| } else if b.overrides.ParseTableAsColumn != nil && inScope.hasTable(colName) { | ||
| scopeTableCols := inScope.resolveColumnAsTable(dbName, colName) |
There was a problem hiding this comment.
Going to address this on a future pass?
| // and column definition statements in order and the collation and character set names for the table | ||
| func GenerateCreateTableStatement(tblName string, colStmts []string, temp, autoInc, tblCharsetName, tblCollName, comment string) string { | ||
| return GlobalSchemaFormatter.GenerateCreateTableStatement(tblName, colStmts, temp, autoInc, tblCharsetName, tblCollName, comment) | ||
| return "" |
There was a problem hiding this comment.
Did you mean to leave this like it is?
Either delete if they're now unused, or revert
Related PRs: