[DRAFT] Table as column draft #3312
Conversation
zachmu
left a comment
zachmu
left a comment
There was a problem hiding this comment.
General idea seems fine, but I think cramming this in Hooks is the wrong abstraction to use. Doltgres needs to plug into the column resolution logic during plan building, and this should be a new interface. See comments.
| } | ||
| fieldArgs[i] = b.buildScalar(inScope, &astArg) | ||
| } | ||
| return expression.NewDoltgresHookExpression(fieldArgs...) |
There was a problem hiding this comment.
I don't think you want to use a hook as outlined in the other PR for this functionality. Those are limited to lifecycle events for schema objects, and I think you want to keep them limited to that. Throwing in planning is a really odd expansion of responsibility there that I don't think is warranted.
What you want here is for the builder object itself to have a ColumnResolver or similar, and the logic here can be totally opaque to the GMS code, implemented entirely in Doltgres. Realistically for now you would need to use function pointer swaps to get this integration, but in the longer term you could specify the details of planbuilder construction during engine initialization.
There was a problem hiding this comment.
Would need to export the Scope type for this to work.
As should be fairly obvious, this PR is to allow for using a table's name as though it were a column, which is required for:
This will be integrated with the following PR after comments are made here, but this is separate just to make it a little easier to look over.
DoltgresFunctionis a stand-in for a regular Doltgres function, andDoltgresHookExpressionis a stand-in for an expression that will be returned by the hooks interface via Doltgres' integration.I tried a few different methods of achieving this, but this seems to be the most straightforward (and most compatible) method I tried. There may be failure cases with this approach, but none seemed obvious from test cases. It's worth noting that this "fully works", in that debugging by stepping through shows that everything is where it needs to be for the hooks integration, so besides general cleanup and all the locations that I've marked can be removed, this can be treated with relatively high scrutiny.