Conversation
a901622 to
13fff24
Compare
…e original column name, not the underlying name. This ensures we generate correct GetField values for this column.
549a302 to
f667b86
Compare
zachmu
left a comment
There was a problem hiding this comment.
Clever approach, one suggestion to make it less error prone.
sql/planbuilder/builder.go
Outdated
| colId columnId | ||
| tabId sql.TableId | ||
| colId columnId | ||
| subqueryScope *scope |
There was a problem hiding this comment.
A simple way to make this significantly less hacky and prone to error would be to create a clone of the builder in resolveView, and return the scope it obtained during building as one of the results of resolveView. Then the main builder never has this field set to a value that's inappropriate to the context, e.g. when parsing two views in the same query. It also guards against the case of views that themselves contain views recursively (which you should make sure you have a test for). Also make sure to document the purpose of this field in the struct, as it's pretty mysterious otherwise. Also recommend calling it resolveViewScope or similar.
There was a problem hiding this comment.
I was able to remove the field entirely by passing the scope as a return value.
… name, not the underlying Alias. This is more correct but prevents a filter from being pushed into multiple views.
This is a partial fix for dolthub/dolt/issues/10297
When parsing a subquery alias, we create a new column id for each column in the SQA schema. The scope mapping is a dictionary on the SQA node that maps those column ids onto the expressions within the subquery that determine their values, and is used in some optimizations. For example, in order to push a filter into a subquery, we need to use the scope mapping to replace any GetFields that were pointing to the SQA with the expressions those fields map to. If for whatever reason the SQA doesn't have a scope mapping, we can't perform that optimization.
We parse views by recursively calling the parser on the view definition. This works but it means that the original parser doesn't have any references to the expressions inside the view, which prevents us from creating the scope mapping.
This PR attempts to fix this. Instead of defining the SQA columns in the original parser (where we no longer have access to the view's scope), we now create the columns while parsing the view, and attach them to the scope object for the view definition. Then we store that scope in a field on the Builder, so that the original parser can copy them into its own scope.
This feels hacky, but was the best way I could think of to generate the scope mappings and ensure they're visible outside the view.