PR feedback: refactor conversion to bool for is_metadata

Author: georgesittas

sqlmesh/core/model/common.py

@@ -54,7 +54,7 @@ def make_python_env(
     variables = variables or {}
     blueprint_variables = blueprint_variables or {}
 
-    used_macros: t.Dict[str, t.Tuple[MacroCallable, t.Optional[bool]]] = {}
+    used_macros: t.Dict[str, t.Tuple[MacroCallable, bool]] = {}
     used_variable_referenced_in_metadata_expression = dict.fromkeys(used_variables or set(), False)
 
     # For an expression like @foo(@v1, @bar(@v1, @v2), @v3), the following mapping would be:
@@ -66,7 +66,7 @@ def make_python_env(
         if isinstance(expression_metadata, tuple):
             expression, is_metadata = expression_metadata
         else:
-            expression, is_metadata = expression_metadata, None
+            expression, is_metadata = expression_metadata, False
 
         if isinstance(expression, d.Jinja):
             continue
@@ -98,7 +98,7 @@ def make_python_env(
                     var_name = args[0].this.lower()
                     used_variable_referenced_in_metadata_expression[var_name] = (
                         used_variable_referenced_in_metadata_expression.get(var_name, True)
-                        and bool(is_metadata)
+                        and is_metadata
                     )
                 else:
                     for var_ref in _extract_macro_func_variable_references(macro_func_or_var):
@@ -114,7 +114,7 @@ def make_python_env(
                 elif name in variables or name in blueprint_variables:
                     used_variable_referenced_in_metadata_expression[name] = (
                         used_variable_referenced_in_metadata_expression.get(name, True)
-                        and bool(is_metadata)
+                        and is_metadata
                     )
             elif (
                 isinstance(macro_func_or_var, (exp.Identifier, d.MacroStrReplace, d.MacroSQL))
@@ -126,12 +126,12 @@ def make_python_env(
                     if var_name in variables or var_name in blueprint_variables:
                         used_variable_referenced_in_metadata_expression[var_name] = (
                             used_variable_referenced_in_metadata_expression.get(var_name, True)
-                            and bool(is_metadata)
+                            and is_metadata
                         )
 
     for macro_ref in jinja_macro_references or set():
         if macro_ref.package is None and macro_ref.name in macros:
-            used_macros[macro_ref.name] = (macros[macro_ref.name], None)
+            used_macros[macro_ref.name] = (macros[macro_ref.name], False)
 
     for name, (used_macro, is_metadata) in used_macros.items():
         if isinstance(used_macro, Executable):

sqlmesh/utils/metaprogramming.py

@@ -283,7 +283,7 @@ def build_env(
     env: t.Dict[str, t.Tuple[t.Any, t.Optional[bool]]],
     name: str,
     path: Path,
-    is_metadata_obj: t.Optional[bool] = None,
+    is_metadata_obj: bool = False,
 ) -> None:
     """Fills in env dictionary with all globals needed to execute the object.
 
@@ -299,7 +299,7 @@ def build_env(
     # We don't rely on `env` to keep track of visited objects, because it's populated in post-order
     visited: t.Set[str] = set()
 
-    def walk(obj: t.Any, name: str, is_metadata: t.Optional[bool] = None) -> None:
+    def walk(obj: t.Any, name: str, is_metadata: bool = False) -> None:
         obj_module = inspect.getmodule(obj)
         if obj_module and obj_module.__name__ == "builtins":
             return
@@ -320,7 +320,7 @@ def walk(obj: t.Any, name: str, is_metadata: t.Optional[bool] = None) -> None:
                 # The existing object in the env is "metadata only" but we're walking it again as a
                 # non-"metadata only" dependency, so we update this flag to ensure all transitive
                 # dependencies are also not marked as "metadata only"
-                is_metadata = None
+                is_metadata = False
 
             if hasattr(obj, c.SQLMESH_MACRO):
                 # We only need to add the undecorated code of @macro() functions in env, which
@@ -380,7 +380,7 @@ def walk(obj: t.Any, name: str, is_metadata: t.Optional[bool] = None) -> None:
             )
 
     # The "metadata only" annotation of the object is transitive
-    walk(obj, name, is_metadata_obj or getattr(obj, c.SQLMESH_METADATA, None))
+    walk(obj, name, is_metadata_obj or getattr(obj, c.SQLMESH_METADATA, False))
 
 
 @dataclass
@@ -432,7 +432,11 @@ def value(
         cls, v: t.Any, is_metadata: t.Optional[bool] = None, sort_root_dict: bool = False
     ) -> Executable:
         payload = _dict_sort(v) if sort_root_dict else repr(v)
-        return Executable(payload=payload, kind=ExecutableKind.VALUE, is_metadata=is_metadata)
+        return Executable(
+            payload=payload,
+            kind=ExecutableKind.VALUE,
+            is_metadata=is_metadata or None,
+        )
 
 
 def serialize_env(env: t.Dict[str, t.Any], path: Path) -> t.Dict[str, Executable]:
@@ -447,6 +451,9 @@ def serialize_env(env: t.Dict[str, t.Any], path: Path) -> t.Dict[str, Executable
     serialized = {}
 
     for k, (v, is_metadata) in env.items():
+        # We don't store `False` for `is_metadata` to reduce the pydantic model's payload size
+        is_metadata = is_metadata or None
+
         if isinstance(v, LITERALS) or v is None:
             serialized[k] = Executable.value(v, is_metadata=is_metadata)
         elif inspect.ismodule(v):

tests/utils/test_metaprogramming.py

@@ -406,7 +406,7 @@ def function_with_custom_decorator():
         "SQLGLOT_META": Executable.value("sqlglot.meta"),
     }
 
-    assert all(is_metadata is None for (_, is_metadata) in env.values())
+    assert all(not is_metadata for (_, is_metadata) in env.values())
     assert serialized_env == expected_env
 
     # Annotate the entrypoint as "metadata only" to show how it propagates