Skip to content

Commit

Permalink
Don't reduce verbose SQL expressions
Browse files Browse the repository at this point in the history
  • Loading branch information
courtneyholcomb committed Dec 19, 2024
1 parent d260a66 commit b53a655
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
16 changes: 16 additions & 0 deletions metricflow-semantics/metricflow_semantics/sql/sql_exprs.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ def as_window_function_expression(self) -> Optional[SqlWindowFunctionExpression]
"""If this is a window function expression, return self."""
return None

@property
def is_verbose(self) -> bool:
"""Denotes if the statement is typically verbose, and therefore can be hard to read when optimized.
This is helpful in determining if statements will be harder to read when collapsed.
"""
return False

@abstractmethod
def rewrite(
self,
Expand Down Expand Up @@ -1197,6 +1205,10 @@ def matches(self, other: SqlExpressionNode) -> bool: # noqa: D102
and self.sql_function_args == other.sql_function_args
)

@property
def is_verbose(self) -> bool: # noqa: D102
return True


@dataclass(frozen=True, eq=False)
class SqlNullExpression(SqlExpressionNode):
Expand Down Expand Up @@ -1884,6 +1896,10 @@ def matches(self, other: SqlExpressionNode) -> bool: # noqa: D102
return False
return self.when_to_then_exprs == other.when_to_then_exprs and self.else_expr == other.else_expr

@property
def is_verbose(self) -> bool: # noqa: D102
return True


class SqlArithmeticOperator(Enum):
"""Arithmetic operator used to do math in a SQL expression."""
Expand Down
10 changes: 9 additions & 1 deletion metricflow/sql/optimizer/rewriting_sub_query_reducer.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ def _current_node_can_be_reduced(self, node: SqlSelectStatementNode) -> bool:
if len(from_source_node_as_select_node.group_bys) > 0 and len(node.group_bys) > 0:
return False

# Skip this case for readability.
if any(col.expr.is_verbose for col in from_source_node_as_select_node.select_columns):
return False

# If there is a column in the parent group by that is not used in the current select statement, don't reduce or it
# would leave an unselected column in the group by and change the meaning of the query. For example, in the SQL
# below, reducing would remove the `is_instant` from the select statement.
Expand Down Expand Up @@ -497,7 +501,11 @@ def _rewrite_node_with_join(self, node: SqlSelectStatementNode) -> SqlSelectStat
join_select_node = join_desc.right_source.as_select_node

# Verifying that it's simple makes it easier to reason about the logic.
if not join_select_node or not SqlRewritingSubQueryReducerVisitor._is_simple_source(join_select_node):
if (
not join_select_node
or not SqlRewritingSubQueryReducerVisitor._is_simple_source(join_select_node)
or any(col.expr.is_verbose for col in join_select_node.select_columns)
):
new_join_descs.append(join_desc)
continue

Expand Down

0 comments on commit b53a655

Please sign in to comment.