Fix merging of optional config parameters

metric_config_parser/data_source.py

@@ -117,12 +117,12 @@ class DataSourceDefinition:
     """Describes the interface for defining a data source in configuration."""
 
     name: str  # implicit in configuration
-    from_expression: str
+    from_expression: Optional[str]
     experiments_column_type: Optional[str] = None
-    client_id_column: Optional[str] = "client_id"
-    submission_date_column: Optional[str] = "submission_date"
+    client_id_column: Optional[str] = None
+    submission_date_column: Optional[str] = None
     default_dataset: Optional[str] = None
-    build_id_column: str = "SAFE.SUBSTR(application.build_id, 0, 8)"
+    build_id_column: Optional[str] = None
 
     def resolve(self, spec: "DefinitionSpecSub") -> DataSource:
         params: Dict[str, Any] = {"name": self.name, "from_expression": self.from_expression}
@@ -147,6 +147,11 @@ def resolve(self, spec: "DefinitionSpecSub") -> DataSource:
             params["experiments_column_type"] = None
         return DataSource(**params)
 
+    def merge(self, other: "DataSourceDefinition"):
+        """Merge with another data source definition."""
+        for key in attr.fields_dict(type(self)):
+            setattr(self, key, getattr(other, key) or getattr(self, key))
+
 
 @attr.s(auto_attribs=True)
 class DataSourcesSpec:
@@ -172,7 +177,14 @@ def merge(self, other: "DataSourcesSpec"):
         Merge another datasource spec into the current one.
         The `other` DataSourcesSpec overwrites existing keys.
         """
-        self.definitions.update(other.definitions)
+        seen = []
+        for key, _ in self.definitions.items():
+            if key in other.definitions:
+                self.definitions[key].merge(other.definitions[key])
+            seen.append(key)
+        for key, definition in other.definitions.items():
+            if key not in seen:
+                self.definitions[key] = definition
 
 
 converter.register_structure_hook(

metric_config_parser/dimension.py

@@ -44,6 +44,11 @@ def resolve(
             description=self.description,
         )
 
+    def merge(self, other: "DimensionDefinition"):
+        """Merge with another dimension definition."""
+        for key in attr.fields_dict(type(self)):
+            setattr(self, key, getattr(other, key) or getattr(self, key))
+
 
 @attr.s(auto_attribs=True)
 class DimensionsSpec:
@@ -67,7 +72,14 @@ def merge(self, other: "DimensionsSpec"):
 
         The `other` DimensionsSpec overwrites existing keys.
         """
-        self.definitions.update(other.definitions)
+        seen = []
+        for key, _ in self.definitions.items():
+            if key in other.definitions:
+                self.definitions[key].merge(other.definitions[key])
+            seen.append(key)
+        for key, definition in other.definitions.items():
+            if key not in seen:
+                self.definitions[key] = definition
 
 
 converter.register_structure_hook(DimensionsSpec, lambda obj, _type: DimensionsSpec.from_dict(obj))

metric_config_parser/tests/test_monitoring.py

@@ -205,6 +205,52 @@ def test_merge_statistic(self, config_collection):
         assert test.metric.type == "histogram"
         assert test.statistic.name == "sum"
 
+    def test_merge_data_source(self, config_collection):
+        """Test merging configs with data sources"""
+        config_str = dedent(
+            """
+            [metrics]
+            [metrics.test]
+            select_expression = "SELECT 1"
+            data_source = "foo"
+            type = "histogram"
+
+            [metrics.test.statistics]
+            sum = {}
+
+            [data_sources]
+            [data_sources.foo]
+            from_expression = "test"
+            build_id_column = "test"
+            """
+        )
+        spec = MonitoringSpec.from_definition_spec(DefinitionSpec.from_dict(toml.loads(config_str)))
+
+        config_str = dedent(
+            """
+            [project]
+            name = "foo"
+            metrics = ["test"]
+
+            [data_sources]
+            [data_sources.foo]
+            from_expression = "foo"
+            """
+        )
+
+        spec2 = MonitoringSpec.from_dict(toml.loads(config_str))
+        spec.merge(spec2)
+
+        assert spec.data_sources.definitions["foo"].name == "foo"
+        assert spec.data_sources.definitions["foo"].from_expression == "foo"
+        assert spec.data_sources.definitions["foo"].build_id_column == "test"
+
+        cfg = spec.resolve(experiment=None, configs=config_collection)
+
+        test = [p for p in cfg.metrics if p.metric.name == "test"][0]
+        assert test.metric.data_source.name == "foo"
+        assert test.metric.data_source.from_expression == "foo"
+
     def test_unknown_metric_failure(self, config_collection):
         config_str = dedent(
             """

setup.py

@@ -59,5 +59,5 @@ def text_from_file(path):
     long_description=text_from_file("README.md"),
     long_description_content_type="text/markdown",
     python_requires=">=3.6",
-    version="2022.10.6",
+    version="2022.10.7",
 )