fix applying bindvars to filters and table functions #1230
Conversation
sql/analyzer/analyzer.go
Outdated
| @@ -387,6 +387,7 @@ func prePrepareRuleSelector(id RuleId) bool { | |||
| case | |||
| // OnceBeforeDefault | |||
| reresolveTablesId, | |||
| validatePriviledgesId, | |||
There was a problem hiding this comment.
we're skipping privilege checks in pre and post?
There was a problem hiding this comment.
Meant to just skip in pre, will add it back in post.
I did this because the analyzer would call CheckPrivileges on an unresolved TableFunction in pre, and give panics.
| @@ -49,6 +49,18 @@ func resolveTableFunctions(ctx *sql.Context, a *Analyzer, n sql.Node, _ *Scope, | |||
| database = privilegedDatabase.Unwrap() | |||
| } | |||
|
|
|||
| var hasBindVarArgs bool | |||
There was a problem hiding this comment.
Ideally we would do at least some of the work upfront to frontload/prepare analyzer logic. This makes this rule more expensive and we run it a whole second time. Not the end of the world but we can't find and swap table functions in ApplyBindings? Or were error tests failing because of invalid argument types? In which case the table function resolve and semantic validation should maybe be split into different rules.
There was a problem hiding this comment.
I'm not sure what you mean by swap table functions?
The tests were failing because of invalid argument types (i think); the BindVars never get replaced, so we try to evaluate them during pre, leading to panics.
There was a problem hiding this comment.
whoops meant arguments, can we not go and replace the arguments in ApplyBindings
There was a problem hiding this comment.
I do in the code I added in plan/bindvar.go
I think this does the same thing as the DeferredTable; the first pass leaves it unresolved, and it should work normally in the second pass.
sql/plan/bindvar.go
Outdated
| @@ -92,6 +92,31 @@ func applyBindingsHelper(n sql.Node, bindings map[string]sql.Expression) (sql.No | |||
| } | |||
| ne, _, err := transform.NodeExprs(n.WithSource(newSource), fixBindingsTransform) | |||
| return ne, transform.NewTree, err | |||
| case *ResolvedTable: | |||
There was a problem hiding this comment.
Seems like calling these ResolvedTable is misleading, like they will panic if you try to read them. I know we talked about something like DeferredFilteredTable, is that not an easy add to avoid the confusion?
There was a problem hiding this comment.
It's definitely possible, but I think it is definitely a bit harder.
DeferredTable is created in resolveTable (in BeforeDefault), but during that time filters haven't yet been evaluated, so I can't save them there.
I could add a new rule right before pushdownFilters that prevents the filters from being pusheddown? and then applyBindings might just work?
or I could add a new rule/modify pushdownFilters (in AfterDefault), to create a DeferredTable there?
There was a problem hiding this comment.
WithFilters has one use in pushdown. I think a helper function should look for a BindVar in the WithFilters argument, and we switch on the result to make either a ResolvedTable or DeferredFiltersTable. Deferred table will implemented UnresolvedTable and look a lot like DeferredAsOf, and will be a target for bindvar replacement.
sql/plan/delete.go
Outdated
| @@ -42,6 +42,8 @@ func GetDeletable(node sql.Node) (sql.DeletableTable, error) { | |||
| return GetDeletable(node.ResolvedTable) | |||
| case *ResolvedTable: | |||
| return getDeletableTable(node.Table) | |||
| //case *DeferredFilteredTable: | |||
sql/plan/bindvar.go
Outdated
| @@ -70,7 +70,7 @@ func applyBindingsHelper(n sql.Node, bindings map[string]sql.Expression) (sql.No | |||
| fixBindingsTransform := func(e sql.Expression) (sql.Expression, transform.TreeIdentity, error) { | |||
| return fixBindings(e, bindings) | |||
| } | |||
| return transform.NodeWithOpaque(n, func(node sql.Node) (sql.Node, transform.TreeIdentity, error) { | |||
| node, same, err := transform.NodeWithOpaque(n, func(node sql.Node) (sql.Node, transform.TreeIdentity, error) { | |||
There was a problem hiding this comment.
probably reset back to the return?
sql/analyzer/pushdown.go
Outdated
| return pushdownFiltersAtNode(ctx, a, n, scope, sel) | ||
| node, same, err := pushdownFiltersAtNode(ctx, a, n, scope, sel) | ||
| var hasBindVars bool | ||
| transform.Inspect(n, func(node sql.Node) bool { |
There was a problem hiding this comment.
you use this elsewhere, maybe a good candidate for a helper method filtersHasBindvar, or something similar
|
|
||
| // OnceAfterDefault | ||
| pushdownFiltersId, |
jycor
changed the title
Changes:
DeferredFilteredTablenodeTest PR: