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.
|
|
||
| // 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.
|
|
||
| // resolveColumnAsTable resolves a column as though it were a table, by searching the table space. This then returns all | ||
| // columns (in their index order) of the table. | ||
| func (s *scope) resolveColumnAsTable(db, table string) []scopeColumn { |
There was a problem hiding this comment.
Again probably belongs in the override, not declared here.
| // We only set the info if it's nil, as in the normal GMS workflow, this will only be set on the first query and | ||
| // will contain all of the information that we need. Only tests will supply a subset of the information, which we | ||
| // aren't concerned with. | ||
| if info := ctx.GetInfo(sql.ContextInfoID_Builder); info == nil { |
There was a problem hiding this comment.
This is really bad. Let's figure out a way to not do this.
| // NewFromContext creates a new Builder using information from the given context. This will preserve the parser, | ||
| // overrides, etc. as long as the context was created from the server handler. Otherwise, this will use defaults | ||
| // equivalent to calling New with nil options. | ||
| func NewFromContext(ctx *sql.Context, cat sql.Catalog) *Builder { |
There was a problem hiding this comment.
You don't need this if the catalog is aware of and can vend information about builder overrides. It's not ideal, but it's not terrible and definitely preferable to jamming this information into the context.
Related PRs: