feat(duckdb): Addressed review comments

fivetran-amrutabhimsenayachit · fivetran-amrutabhimsenayachit · commit dba3bb85dc07 · 2025-11-26T12:06:17.000-05:00
diff --git a/sqlglot/dialects/bigquery.py b/sqlglot/dialects/bigquery.py
@@ -214,26 +214,34 @@ def _serialize_bq_datetime_diff_unit(self: BigQuery.Generator, expression: exp.E
 
     Canonical form -> BigQuery syntax:
     - Week(SUNDAY) -> WEEK (BigQuery's default)
-    - Week(MONDAY) -> WEEK(MONDAY)
-    - ISOWEEK -> ISOWEEK
+    - Week(MONDAY) -> WEEK(MONDAY) (preserved during round-trip)
+    - Var(ISOWEEK) -> ISOWEEK (preserved as-is)
+    - WeekStart -> preserved as-is
     - Week(other day) -> WEEK(day)
     - Other units -> use unit_to_var
 
     """
     from sqlglot.dialects.dialect import extract_week_unit_info
 
     unit = expression.args.get("unit")
-    day_name = extract_week_unit_info(unit, include_dow=False)
 
-    if day_name and isinstance(day_name, str):
+    # Preserve ISOWEEK/WEEKISO as-is (don't convert to WEEK(MONDAY))
+    if isinstance(unit, exp.Var):
+        unit_name = unit.this.upper() if isinstance(unit.this, str) else str(unit.this)
+        if unit_name in ("ISOWEEK", "WEEKISO"):
+            return self.sql(unit)
+
+    week_info = extract_week_unit_info(unit)
+
+    if week_info:
+        day_name, _ = week_info  # Extract day name, ignore DOW number
         if day_name == "SUNDAY":
             return self.sql(exp.var("WEEK"))
         elif day_name == "MONDAY":
-            # Preserve the original form: WEEK(MONDAY) vs ISOWEEK
             if isinstance(unit, exp.WeekStart):
                 return self.sql(unit)
             else:
-                return self.sql(exp.var("ISOWEEK"))
+                return self.sql(exp.Week(this=exp.var(day_name)))
         else:
             return self.sql(exp.Week(this=exp.var(day_name)))
 
diff --git a/sqlglot/dialects/dialect.py b/sqlglot/dialects/dialect.py
@@ -811,21 +811,23 @@ class Dialect(metaclass=_Dialect):
         "CENTURIES": "CENTURY",
     }
 
+    # Mapping of week unit names to (start_day, dow_number) tuples
+    # dow_number is the numeric day-of-week value (0=Sunday, 1=Monday, etc.)
     WEEK_UNIT_SEMANTICS = {
         "WEEK": ("SUNDAY", 0),
         "ISOWEEK": ("MONDAY", 1),
         "WEEKISO": ("MONDAY", 1),
     }
 
-    # Days of week to DOW numbers (ISO standard: Monday=1, Sunday=0)
+    # Days of week to DOW numbers (ISO standard: Sunday=0, Monday=1)
     WEEK_START_DAY_TO_DOW = {
+        "SUNDAY": 0,
         "MONDAY": 1,
         "TUESDAY": 2,
         "WEDNESDAY": 3,
         "THURSDAY": 4,
         "FRIDAY": 5,
         "SATURDAY": 6,
-        "SUNDAY": 0,
     }
 
     # Specifies what types a given type can be coerced into
@@ -1702,47 +1704,42 @@ def map_date_part(part, dialect: DialectType = Dialect):
 
 
 def extract_week_unit_info(
-    unit: t.Optional[exp.Expression], include_dow: bool = False
-) -> t.Optional[t.Union[str, t.Tuple[str, int]]]:
+    unit: t.Optional[exp.Expression], dialect: DialectType = Dialect
+) -> t.Optional[t.Tuple[str, int]]:
     """
     Extract week unit information from AST node.
 
     This helper provides a unified way to handle week units across dialects.
 
     Args:
         unit: The unit expression (Var, Week, or WeekStart)
-        include_dow: If True, return (day_name, dow_number) tuple for transformations
-                    If False, return just day_name string for serialization
+        dialect: Dialect class or instance for accessing WEEK_UNIT_SEMANTICS and WEEK_START_DAY_TO_DOW
 
     Returns:
-        - If include_dow=False: day_name (e.g., "SUNDAY")
-        - If include_dow=True: (day_name, dow_number) (e.g., ("SUNDAY", 0))
-        - None if not a week unit
+        - (day_name, dow_number) tuple (e.g., ("SUNDAY", 0) or ("MONDAY", 1))
+        - None if not a week unit or if day is dynamic (not a constant)
 
     """
+    dialect_instance = Dialect.get_or_raise(dialect)
+
     # Handle plain Var expressions for ISOWEEK/WEEKISO only
     # NOTE: Plain Var('WEEK') is NOT handled to avoid breaking other dialects
     if isinstance(unit, exp.Var):
         unit_name = unit.this.upper() if isinstance(unit.this, str) else str(unit.this)
         # Only handle ISOWEEK/WEEKISO variants, not plain WEEK
         if unit_name in ("ISOWEEK", "WEEKISO"):
-            week_info = Dialect.WEEK_UNIT_SEMANTICS.get(unit_name)
+            week_info = dialect_instance.WEEK_UNIT_SEMANTICS.get(unit_name)
             if week_info:
-                day_name, dow = week_info
-                return (day_name, dow) if include_dow else day_name
+                return week_info
         return None
 
     # Handle Week/WeekStart expressions with explicit day
     if isinstance(unit, (exp.Week, exp.WeekStart)):
         day_var = unit.this
         if isinstance(day_var, exp.Var):
             day_name = day_var.this.upper() if isinstance(day_var.this, str) else str(day_var.this)
-
-            if include_dow:
-                dow_value = Dialect.WEEK_START_DAY_TO_DOW.get(day_name)
-                return (day_name, dow_value) if dow_value is not None else None
-
-            return day_name
+            dow_value = dialect_instance.WEEK_START_DAY_TO_DOW.get(day_name)
+            return (day_name, dow_value) if dow_value is not None else None
 
     return None
 
diff --git a/sqlglot/dialects/duckdb.py b/sqlglot/dialects/duckdb.py
@@ -249,34 +249,14 @@ def _implicit_datetime_cast(
     return arg
 
 
-def _extract_week_start_day(unit: t.Optional[exp.Expression]) -> t.Optional[t.Tuple[str, int]]:
-    """
-    Extract week start day name and DOW number from a Week, WeekStart, or plain Var expression.
-
-    Uses extract_week_unit_info(include_dow=True) for uniform week unit handling.
-
-    Handles:
-    - Var('WEEK') -> ('SUNDAY', 0)  # BigQuery default
-    - Var('ISOWEEK') -> ('MONDAY', 1)
-    - Week(Var('day')) -> ('day', dow)
-    - WeekStart(Var('day')) -> ('day', dow)
-    """
-    from sqlglot.dialects.dialect import extract_week_unit_info
-
-    # Use shared helper with include_dow=True to get (day_name, dow_number)
-    result = extract_week_unit_info(unit, include_dow=True)
-    # When include_dow=True, result is either None or Tuple[str, int]
-    return result if result is None or isinstance(result, tuple) else None
-
-
 def _build_week_trunc_expression(date_expr: exp.Expression, start_dow: int) -> exp.Expression:
     """
     Build DATE_TRUNC expression for week boundaries with custom start day.
     Formula: shift = 1 - start_dow, then DATE_TRUNC('week', date + INTERVAL shift DAY)
     """
     if start_dow == 1:
         # No shift needed for Monday-based weeks (ISO standard)
-        return exp.Anonymous(this="DATE_TRUNC", expressions=[exp.Literal.string("week"), date_expr])
+        return exp.DateTrunc(unit=exp.var("week"), this=date_expr)
 
     # Shift date to align week boundaries with ISO Monday
     shift_days = 1 - start_dow
@@ -285,18 +265,24 @@ def _build_week_trunc_expression(date_expr: exp.Expression, start_dow: int) -> e
         expression=exp.Interval(this=exp.Literal.string(str(shift_days)), unit=exp.var("DAY")),
     )
 
-    return exp.Anonymous(this="DATE_TRUNC", expressions=[exp.Literal.string("week"), shifted_date])
+    return exp.DateTrunc(unit=exp.var("week"), this=shifted_date)
 
 
 def _date_diff_sql(self: DuckDB.Generator, expression: exp.DateDiff) -> str:
     """
     Generate DATE_DIFF SQL for DuckDB using DATE_TRUNC-based week alignment.
+
+    Note: When week start day is dynamic (from a column), week_start will be None
+    and we fall back to standard DATE_DIFF, since compile-time offsets are required.
     """
+    from sqlglot.dialects.dialect import extract_week_unit_info
+
     this = _implicit_datetime_cast(expression.this)
     expr = _implicit_datetime_cast(expression.expression)
     unit = expression.args.get("unit")
 
-    week_start = _extract_week_start_day(unit)
+    # Extract week start day; returns None if day is dynamic (column reference)
+    week_start = extract_week_unit_info(unit)
     if week_start and this and expr:
         _, start_dow = week_start
 
diff --git a/tests/dialects/test_bigquery.py b/tests/dialects/test_bigquery.py
@@ -3225,47 +3225,47 @@ def test_week(self):
             "DATE_DIFF('2024-01-15', '2024-01-08', WEEK(MONDAY))",
             write={
                 "bigquery": "DATE_DIFF('2024-01-15', '2024-01-08', WEEK(MONDAY))",
-                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('week', CAST('2024-01-08' AS DATE)), DATE_TRUNC('week', CAST('2024-01-15' AS DATE))) // 7",
+                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-08' AS DATE)), DATE_TRUNC('WEEK', CAST('2024-01-15' AS DATE))) // 7",
             },
         )
         # Test WEEK(SUNDAY) - Sunday-based week (normalized to WEEK)
         self.validate_all(
             "DATE_DIFF('2024-01-15', '2024-01-08', WEEK(SUNDAY))",
             write={
                 "bigquery": "DATE_DIFF('2024-01-15', '2024-01-08', WEEK)",
-                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('week', CAST('2024-01-08' AS DATE) + INTERVAL '1' DAY), DATE_TRUNC('week', CAST('2024-01-15' AS DATE) + INTERVAL '1' DAY)) // 7",
+                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-08' AS DATE) + INTERVAL '1' DAY), DATE_TRUNC('WEEK', CAST('2024-01-15' AS DATE) + INTERVAL '1' DAY)) // 7",
             },
         )
         # Test WEEK(SATURDAY) - Saturday-based week calculation
         self.validate_all(
             "DATE_DIFF('2024-01-15', '2024-01-08', WEEK(SATURDAY))",
             write={
                 "bigquery": "DATE_DIFF('2024-01-15', '2024-01-08', WEEK(SATURDAY))",
-                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('week', CAST('2024-01-08' AS DATE) + INTERVAL '-5' DAY), DATE_TRUNC('week', CAST('2024-01-15' AS DATE) + INTERVAL '-5' DAY)) // 7",
+                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-08' AS DATE) + INTERVAL '-5' DAY), DATE_TRUNC('WEEK', CAST('2024-01-15' AS DATE) + INTERVAL '-5' DAY)) // 7",
             },
         )
         # Test WEEK - Default Sunday-based week calculation
         self.validate_all(
             "DATE_DIFF('2024-01-15', '2024-01-08', WEEK)",
             write={
                 "bigquery": "DATE_DIFF('2024-01-15', '2024-01-08', WEEK)",
-                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('week', CAST('2024-01-08' AS DATE) + INTERVAL '1' DAY), DATE_TRUNC('week', CAST('2024-01-15' AS DATE) + INTERVAL '1' DAY)) // 7",
+                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-08' AS DATE) + INTERVAL '1' DAY), DATE_TRUNC('WEEK', CAST('2024-01-15' AS DATE) + INTERVAL '1' DAY)) // 7",
             },
         )
         # Test ISOWEEK - ISO 8601 Monday-based week calculation
         self.validate_all(
             "DATE_DIFF('2024-01-15', '2024-01-08', ISOWEEK)",
             write={
                 "bigquery": "DATE_DIFF('2024-01-15', '2024-01-08', ISOWEEK)",
-                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('week', CAST('2024-01-08' AS DATE)), DATE_TRUNC('week', CAST('2024-01-15' AS DATE))) // 7",
+                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-08' AS DATE)), DATE_TRUNC('WEEK', CAST('2024-01-15' AS DATE))) // 7",
             },
         )
         # Test with DATE literal - Explicit DATE casting
         self.validate_all(
             "DATE_DIFF(DATE '2024-01-15', DATE '2024-01-08', WEEK(MONDAY))",
             write={
                 "bigquery": "DATE_DIFF(CAST('2024-01-15' AS DATE), CAST('2024-01-08' AS DATE), WEEK(MONDAY))",
-                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('week', CAST('2024-01-08' AS DATE)), DATE_TRUNC('week', CAST('2024-01-15' AS DATE))) // 7",
+                "duckdb": "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-08' AS DATE)), DATE_TRUNC('WEEK', CAST('2024-01-15' AS DATE))) // 7",
             },
         )
 
diff --git a/tests/dialects/test_duckdb.py b/tests/dialects/test_duckdb.py
@@ -1983,24 +1983,6 @@ def test_extract_date_parts(self):
                 # All of these should remain as is, they don't have synonyms
                 self.validate_identity(f"EXTRACT({part} FROM foo)")
 
-    def test_date_diff_week(self):
-        # Test Monday-based week (ISO week) - no offset needed
-        self.validate_identity(
-            "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-08' AS DATE)), DATE_TRUNC('WEEK', CAST('2024-01-15' AS DATE))) // 7"
-        )
-        # Test Sunday-based week - shift by +1 day to align with ISO Monday
-        self.validate_identity(
-            "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-08' AS DATE) + INTERVAL '1' DAY), DATE_TRUNC('WEEK', CAST('2024-01-15' AS DATE) + INTERVAL '1' DAY)) // 7"
-        )
-        # Test Saturday-based week - shift by -5 days to align with ISO Monday
-        self.validate_identity(
-            "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-08' AS DATE) + INTERVAL '-5' DAY), DATE_TRUNC('WEEK', CAST('2024-01-15' AS DATE) + INTERVAL '-5' DAY)) // 7"
-        )
-        # Test zero weeks between dates in same week
-        self.validate_identity(
-            "DATE_DIFF('DAY', DATE_TRUNC('WEEK', CAST('2024-01-01' AS DATE)), DATE_TRUNC('WEEK', CAST('2024-01-01' AS DATE))) // 7"
-        )
-
     def test_set_item(self):
         self.validate_identity("SET memory_limit = '10GB'")
         self.validate_identity("SET SESSION default_collation = 'nocase'")
