Split column pruner into two phases #1501
Conversation
There was a problem hiding this comment.
Overall, this logic looks great!
I had a bit of trouble reading the code (sorry for the slow review - that's why!), but I think this was only due to the naming of some of the classes / variables / etc. I've left some suggestions to improve readability, and most all of them are just related to naming.
| f"SQL, but this is a bug and should be investigated." | ||
| ) | ||
| return node | ||
|
|
||
| pruned_select_columns = tuple( |
There was a problem hiding this comment.
This is tangential, but I've frequently read this code and found this variable name confusing (pruned_select_columns). We frequently refer to "pruned columns" when we mean the ones that have been removed, but in this case we mean the columns that have been kept. I think the word pruned can technically be used both ways, but it typically is used to refer to what has been removed. Can we change this to a more clear variable name?
There was a problem hiding this comment.
Sure, this could be renamed. To understand where you're coming from, you mention that [pruned] typically is used to refer to what has been removed. What examples were you thinking?
When I think of pruned, I think about an overgrown tree. Once I prune it, I would call it a pruned tree.
There was a problem hiding this comment.
I think that's true - the tree has been pruned. But if you're referring to the branches, I think the "pruned" branches would typically refer to the ones removed. In this case the tree is the SQL node and the columns are the branches.
I just did a quick search through the code to see how we use this word, and there are a couple places where we use the opposite meaning of prune:
metricflow/tests_metricflow/sql/optimizer/test_column_pruner.py
Lines 349 to 350 in d01cbea
metricflow/tests_metricflow/sql/optimizer/test_column_pruner.py
Lines 394 to 395 in d01cbea
And this is silly but I just did a quick google search for a gut check here and it does look like the pruned leaves are the ones that have been removed:
Not totally related to this PR so this isn't blocking! But if you don't update the naming here I probably will the next time I come across it.
There was a problem hiding this comment.
In that case, I'm thinking we use different terms like "removed" and "retained" then, but will have to handle in a follow up.
| self._column_alias_tagger = tagged_column_alias_set | ||
|
|
||
| def _search_for_expressions( | ||
| self, select_node: SqlSelectStatementNode, pruned_select_columns: Tuple[SqlSelectColumn, ...] |
There was a problem hiding this comment.
Same concern re: the name pruned_select_columns here
|
@courtneyholcomb Updated with naming changes + other alterations. I'm going to rename the files later as they cause conflicts in the stack. |
| f"SQL, but this is a bug and should be investigated." | ||
| ) | ||
| return node | ||
|
|
||
| pruned_select_columns = tuple( |
There was a problem hiding this comment.
I think that's true - the tree has been pruned. But if you're referring to the branches, I think the "pruned" branches would typically refer to the ones removed. In this case the tree is the SQL node and the columns are the branches.
I just did a quick search through the code to see how we use this word, and there are a couple places where we use the opposite meaning of prune:
metricflow/tests_metricflow/sql/optimizer/test_column_pruner.py
Lines 349 to 350 in d01cbea
metricflow/tests_metricflow/sql/optimizer/test_column_pruner.py
Lines 394 to 395 in d01cbea
And this is silly but I just did a quick google search for a gut check here and it does look like the pruned leaves are the ones that have been removed:
Not totally related to this PR so this isn't blocking! But if you don't update the naming here I probably will the next time I come across it.
Currently, the column pruner checks the columns that are needed in each `SELECT` statement and generates the pruned SQL in a single pass. For better readability and easier modification, this splits the column pruner into two phases. First, the SQL nodes are traversed to figure out which columns are required and which can be pruned. Then, the SQL nodes are rewritten with the pruned columns.
Currently, the column pruner checks the columns that are needed in each
SELECTstatement and generates the pruned SQL in a single pass. For better readability and easier modification, this splits the column pruner into two phases.First, the SQL nodes are traversed to figure out which columns are required and which can be pruned. Then, the SQL nodes are rewritten with the pruned columns.
The logic in
SqlTagRequiredColumnAliasesVisitorhas been copied from the original implementation.