address comments

dbt-labs · Nov 29, 2022 · f4f71b2 · f4f71b2
1 parent b4889ff
commit f4f71b2
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 12 deletions.
diff --git a/metricflow/plan_conversion/dataflow_to_sql.py b/metricflow/plan_conversion/dataflow_to_sql.py
@@ -96,7 +96,7 @@
     SqlAggregateFunctionExpression,
     SqlWindowFunction,
     SqlWindowFunctionExpression,
-    SqlWindowOrderByArg,
+    SqlWindowOrderByArgument,
 )
 from metricflow.sql.sql_plan import (
     SqlQueryPlan,
@@ -1287,7 +1287,7 @@ def visit_append_row_number_column_node(self, node: AppendRowNumberColumnNode) -
         row_number_select_column = SqlSelectColumn(
             expr=SqlWindowFunctionExpression(
                 sql_function=SqlWindowFunction.ROW_NUMBER,
-                order_by_args=[SqlWindowOrderByArg(expr=x) for x in input_sql_column_references],
+                order_by_args=[SqlWindowOrderByArgument(expr=x) for x in input_sql_column_references],
             ),
             column_alias=row_number_spec_column_name.column_name,
         )

diff --git a/metricflow/sql/sql_exprs.py b/metricflow/sql/sql_exprs.py
@@ -792,7 +792,7 @@ class SqlWindowFunction(Enum):
 
 
 @dataclass(frozen=True)
-class SqlWindowOrderByArg:
+class SqlWindowOrderByArgument:
     """In window functions, the ORDER BY clause can accept an expr, ordering, null ranking."""
 
     expr: SqlExpressionNode
@@ -818,7 +818,7 @@ def __init__(
         sql_function: SqlWindowFunction,
         sql_function_args: Optional[List[SqlExpressionNode]] = None,
         partition_by_args: Optional[List[SqlExpressionNode]] = None,
-        order_by_args: Optional[List[SqlWindowOrderByArg]] = None,
+        order_by_args: Optional[List[SqlWindowOrderByArgument]] = None,
     ) -> None:
         """Constructor.
 
@@ -881,7 +881,7 @@ def partition_by_args(self) -> List[SqlExpressionNode]:  # noqa: D
         return self._partition_by_args or []
 
     @property
-    def order_by_args(self) -> List[SqlWindowOrderByArg]:  # noqa: D
+    def order_by_args(self) -> List[SqlWindowOrderByArgument]:  # noqa: D
         return self._order_by_args or []
 
     def __repr__(self) -> str:  # noqa: D
@@ -901,7 +901,7 @@ def rewrite(  # noqa: D
                 x.rewrite(column_replacements, should_render_table_alias) for x in self.partition_by_args
             ],
             order_by_args=[
-                SqlWindowOrderByArg(
+                SqlWindowOrderByArgument(
                     expr=x.expr.rewrite(column_replacements, should_render_table_alias),
                     descending=x.descending,
                     nulls_last=x.nulls_last,
@@ -919,8 +919,11 @@ def lineage(self) -> SqlExpressionTreeLineage:  # noqa: D
     def matches(self, other: SqlExpressionNode) -> bool:  # noqa: D
         if not isinstance(other, SqlWindowFunctionExpression):
             return False
-        order_by_matches = all(x == y for x, y in itertools.zip_longest(self.order_by_args, other.order_by_args))
-        return self.sql_function == other.sql_function and order_by_matches and self._parents_match(other)
+        return (
+            self.sql_function == other.sql_function
+            and self.order_by_args == other.order_by_args
+            and self._parents_match(other)
+        )
 
 
 class SqlNullExpression(SqlExpressionNode):
@@ -1247,7 +1250,7 @@ class SqlRatioComputationExpression(SqlExpressionNode):
     """Node for expressing Ratio metrics to allow for appropriate casting to float/double in each engine
 
     In future we might wish to break this up into a set of nodes, e.g., SqlCastExpression and SqlMathExpression
-    or even add CAST to SqlAggregateFunctionExpression. However, at this time the only mathematical operation we encode
+    or even add CAST to SqlFunctionExpression. However, at this time the only mathematical operation we encode
     is division, and we only use that for ratios. Similarly, the only times we do typecasting are when we are
     coercing timestamps (already handled) or computing ratio metrics.
     """

diff --git a/metricflow/test/sql/test_sql_expr_render.py b/metricflow/test/sql/test_sql_expr_render.py
@@ -24,7 +24,7 @@
     SqlBetweenExpression,
     SqlWindowFunctionExpression,
     SqlWindowFunction,
-    SqlWindowOrderByArg,
+    SqlWindowOrderByArgument,
 )
 from metricflow.time.time_granularity import TimeGranularity
 
@@ -251,12 +251,12 @@ def test_window_function_expr(default_expr_renderer: DefaultSqlExpressionRendere
                 SqlColumnReferenceExpression(SqlColumnReference("b", "col1")),
             ],
             order_by_args=[
-                SqlWindowOrderByArg(
+                SqlWindowOrderByArgument(
                     expr=SqlColumnReferenceExpression(SqlColumnReference("a", "col0")),
                     descending=True,
                     nulls_last=False,
                 ),
-                SqlWindowOrderByArg(
+                SqlWindowOrderByArgument(
                     expr=SqlColumnReferenceExpression(SqlColumnReference("b", "col0")),
                     descending=False,
                     nulls_last=True,