Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't reduce verbose SQL expressions #1586

Open
wants to merge 1 commit into
base: court/custom-offset6
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading