Fixes 21482: only compare a column against '' when the type can hold it

ingestion/src/metadata/profiler/metrics/static/null_missing_count.py

@@ -16,12 +16,18 @@
 
 from typing import TYPE_CHECKING, Optional
 
-from sqlalchemy import case, column
+from sqlalchemy import Boolean, Interval, case, column
+from sqlalchemy import Enum as SqlEnum
+from sqlalchemy.types import TypeEngine
 
 from metadata.generated.schema.configuration.profilerConfiguration import MetricType
 from metadata.profiler.metrics.core import StaticMetric, _label
 from metadata.profiler.metrics.pandas_metric_protocol import PandasComputation
 from metadata.profiler.orm.functions.sum import SumFn
+from metadata.profiler.orm.registry import is_date_time, is_quantifiable
+from metadata.profiler.orm.types.custom_ip import CustomIP
+from metadata.profiler.orm.types.custom_time import CustomTime
+from metadata.profiler.orm.types.uuid import UUIDString
 from metadata.utils.logger import profiler_logger
 
 if TYPE_CHECKING:
@@ -32,6 +38,26 @@
 logger = profiler_logger()
 
 
+# Types that cannot hold '', beyond the quantifiable/date-time families. Postgres
+# and ClickHouse raise on the comparison; MySQL/MariaDB coerce instead and count
+# real values as missing ('' matches 0, false and, on MariaDB TIME, '00:00:00').
+# CustomTime is listed here rather than in registry.is_date_time because that
+# helper also drives min/max/mean/stddev and columnValuesToBeBetween, which we are
+# not changing here. TODO(#21482): close that registry gap separately.
+NON_EMPTY_STRING_TYPES = (Boolean, UUIDString, CustomIP, CustomTime, Interval, SqlEnum)
+
+
+def can_hold_empty_string(col_type: TypeEngine) -> bool:
+    """Whether comparing a column of this type against '' is meaningful.
+
+    Types we don't recognize fall through to True so dialect-specific string types
+    keep counting empty values.
+    """
+    if is_quantifiable(col_type) or is_date_time(col_type):
+        return False
+    return not isinstance(col_type, NON_EMPTY_STRING_TYPES)
+
+
 class NullMissingCount(StaticMetric):
     """
     NULL + Empty COUNT Metric
@@ -68,13 +94,12 @@
         """
         Returns the SQLAlchemy function for calculating the metric.
         """
-        return SumFn(
-            case(
-                (column(self.col.name, self.col.type).is_(None), 1),
-                (column(self.col.name, self.col.type).__eq__(""), 1),
-                else_=0,
-            )
-        )
+        conditions = [(column(self.col.name, self.col.type).is_(None), 1)]
+
+        if can_hold_empty_string(self.col.type):
+            conditions.append((column(self.col.name, self.col.type).__eq__(""), 1))
+
+        return SumFn(case(*conditions, else_=0))
 
     def df_fn(self, dfs: Optional["PandasRunner"] = None):
         """pandas function"""
@@ -104,6 +129,11 @@
 
         Maintains a single count value. Adds chunk's null and empty string count
         to the current total and returns the sum.
+
+        Unlike the SQL path this needs no type guard: pandas comparisons never
+        coerce, so `== ""` is simply False on a numeric/temporal column. It also
+        stays correct for an object-dtype datalake column that OM typed as INT but
+        which really does carry "" for missing values.
         """
         chunk_null_count = df[column.name].isnull().sum()
         chunk_empty_count = (df[column.name] == "").sum()

ingestion/tests/integration/profiler/test_null_missing_count_metric.py

@@ -0,0 +1,129 @@
+#  Copyright 2025 Collate
+#  Licensed under the Collate Community License, Version 1.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#  https://github.com/open-metadata/OpenMetadata/blob/main/ingestion/LICENSE
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+"""
+Integration tests for the NullMissingCount metric against real engines.
+
+The metric backs the `columnValuesMissingCount` data quality test. Comparing a
+non-string column against '' fails on PostgreSQL and returns the wrong count on
+MySQL, so both engines are exercised here.
+"""
+
+import datetime
+
+import pytest
+from sqlalchemy import Boolean, Column, Date, Integer, String, create_engine, select
+from sqlalchemy.orm import DeclarativeBase
+from testcontainers.mysql import MySqlContainer
+from testcontainers.postgres import PostgresContainer
+
+from metadata.ingestion.connections.session import create_and_bind_session
+from metadata.profiler.metrics.registry import Metrics
+
+
+class Base(DeclarativeBase):
+    pass
+
+
+class MissingCountTestTable(Base):
+    __tablename__ = "missing_count_test"
+    row_id = Column(Integer, primary_key=True)
+    int_col = Column(Integer)
+    date_col = Column(Date)
+    bool_col = Column(Boolean)
+    str_col = Column(String(64))
+
+
+# 3 NULLs per column, plus the values MySQL would coerce '' into: 2 zeros in
+# int_col and 2 false in bool_col. Only the NULLs are missing for those columns;
+# str_col also counts its 2 empty strings.
+ROWS = [
+    (1, 0, datetime.date(2024, 1, 1), False, ""),
+    (2, 0, datetime.date(2024, 1, 2), False, ""),
+    (3, 7, datetime.date(2024, 1, 3), True, "a value"),
+    (4, None, None, None, None),
+    (5, None, None, None, None),
+    (6, None, None, None, None),
+]
+
+EXPECTED_NULL_COUNT = 3
+EXPECTED_NULL_AND_EMPTY_COUNT = 5
+
+
+def _seed(engine):
+    Base.metadata.create_all(bind=engine)
+    session = create_and_bind_session(engine)
+    session.add_all(
+        [
+            MissingCountTestTable(
+                row_id=row_id,
+                int_col=int_col,
+                date_col=date_col,
+                bool_col=bool_col,
+                str_col=str_col,
+            )
+            for row_id, int_col, date_col, bool_col, str_col in ROWS
+        ]
+    )
+    session.commit()
+    return session
+
+
+@pytest.fixture(scope="module")
+def postgres_session():
+    with PostgresContainer("postgres:15") as container:
+        engine = create_engine(container.get_connection_url())
+        session = _seed(engine)
+        yield session
+        session.close()
+        engine.dispose()
+
+
+@pytest.fixture(scope="module")
+def mysql_session():
+    with MySqlContainer(image="mysql:8.4.5", dbname="test_missing_count") as container:
+        engine = create_engine(container.get_connection_url())
+        session = _seed(engine)
+        yield session
+        session.close()
+        engine.dispose()
+
+
+@pytest.fixture(params=["postgres", "mysql"])
+def session(request):
+    session = request.getfixturevalue(f"{request.param}_session")
+    yield session
+    # Postgres refuses every statement after a failed one until the transaction is
+    # rolled back, so one real failure would cascade into the other tests.
+    session.rollback()
+
+
+def run_null_missing_count(session, column) -> int:
+    metric_fn = Metrics.nullMissingCount(column).fn()
+    return int(session.execute(select(metric_fn).select_from(MissingCountTestTable.__table__)).scalar())
+
+
+def test_integer_column_counts_nulls_only(session):
+    """Zeros are not missing values: on MySQL '' would coerce to 0 and count them"""
+    assert run_null_missing_count(session, MissingCountTestTable.int_col) == EXPECTED_NULL_COUNT
+
+
+def test_date_column_counts_nulls_only(session):
+    assert run_null_missing_count(session, MissingCountTestTable.date_col) == EXPECTED_NULL_COUNT
+
+
+def test_boolean_column_counts_nulls_only(session):
+    """False is not a missing value: on MySQL '' would coerce to 0 and count them"""
+    assert run_null_missing_count(session, MissingCountTestTable.bool_col) == EXPECTED_NULL_COUNT
+
+
+def test_string_column_counts_nulls_and_empty_strings(session):
+    assert run_null_missing_count(session, MissingCountTestTable.str_col) == EXPECTED_NULL_AND_EMPTY_COUNT

ingestion/tests/unit/observability/profiler/sqlalchemy/test_null_missing_count_metric.py

@@ -0,0 +1,158 @@
+#  Copyright 2025 Collate
+#  Licensed under the Collate Community License, Version 1.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#  https://github.com/open-metadata/OpenMetadata/blob/main/ingestion/LICENSE
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+"""
+Test that the NullMissingCount metric only compares a column against the empty
+string when the column can actually hold one.
+
+Comparing a numeric/temporal/boolean/uuid/enum/interval/ip column with '' is a
+runtime error on strict engines (Postgres, ClickHouse) and silently coerces on
+MySQL and MariaDB, counting real values as missing.
+"""
+
+import pytest
+from sqlalchemy import (
+    BIGINT,
+    CHAR,
+    DECIMAL,
+    NVARCHAR,
+    TEXT,
+    VARCHAR,
+    Boolean,
+    Column,
+    Date,
+    DateTime,
+    Enum,
+    Float,
+    Integer,
+    Interval,
+    MetaData,
+    Numeric,
+    SmallInteger,
+    String,
+    Table,
+    Time,
+    select,
+)
+from sqlalchemy.dialects import mysql, postgresql
+
+from metadata.generated.schema.entity.data.table import DataType
+from metadata.profiler.metrics.registry import Metrics
+from metadata.profiler.orm.converter.common import CommonMapTypes
+from metadata.profiler.orm.types.custom_ip import CustomIP
+from metadata.profiler.orm.types.custom_time import CustomTime
+from metadata.profiler.orm.types.custom_timestamp import CustomTimestamp
+from metadata.profiler.orm.types.undetermined_type import UndeterminedType
+from metadata.profiler.orm.types.uuid import UUIDString
+
+EMPTY_STRING_COMPARISON = "= ''"
+
+DIALECTS = [postgresql.dialect(), mysql.dialect()]
+
+# OM types listed as supported by columnValuesMissingCount that no engine can
+# store an empty string in. Kept as DataType so the mapping in CommonMapTypes is
+# exercised too: a converter change that reintroduces the bug fails here.
+NON_EMPTY_STRING_DATA_TYPES = [
+    DataType.NUMBER,
+    DataType.TINYINT,
+    DataType.SMALLINT,
+    DataType.INT,
+    DataType.BIGINT,
+    DataType.BYTEINT,
+    DataType.FLOAT,
+    DataType.DOUBLE,
+    DataType.DECIMAL,
+    DataType.NUMERIC,
+    DataType.TIMESTAMP,
+    DataType.TIME,
+    DataType.DATE,
+    DataType.DATETIME,
+    DataType.INTERVAL,
+    DataType.BOOLEAN,
+    DataType.ENUM,
+    DataType.UUID,
+]
+
+
+def compile_null_missing_count(sqa_type, dialect) -> str:
+    """Compile the nullMissingCount expression for a column of the given type"""
+    table = Table("a_table", MetaData(), Column("a_column", sqa_type))
+    metric_fn = Metrics.nullMissingCount(table.c.a_column).fn()
+    return str(
+        select(metric_fn).compile(
+            dialect=dialect,
+            compile_kwargs={"literal_binds": True},
+        )
+    )
+
+
+@pytest.mark.parametrize("dialect", DIALECTS, ids=lambda d: d.name)
+@pytest.mark.parametrize(
+    "sqa_type",
+    [
+        Integer(),
+        SmallInteger(),
+        BIGINT(),
+        Numeric(),
+        DECIMAL(),
+        Float(),
+        Date(),
+        DateTime(),
+        Time(),
+        CustomTimestamp(),
+        CustomTime(),
+        Boolean(),
+        UUIDString(),
+        Interval(),
+        Enum("a", "b", name="an_enum"),
+        CustomIP(),
+    ],
+    ids=lambda t: type(t).__name__,
+)
+def test_no_empty_string_comparison_for_non_string_types(sqa_type, dialect):
+    """A column that cannot hold '' is only checked for NULL"""
+    query = compile_null_missing_count(sqa_type, dialect)
+
+    assert "IS NULL" in query
+    assert EMPTY_STRING_COMPARISON not in query
+
+
+@pytest.mark.parametrize("dialect", DIALECTS, ids=lambda d: d.name)
+@pytest.mark.parametrize(
+    "sqa_type",
+    [
+        String(),
+        VARCHAR(256),
+        TEXT(),
+        CHAR(8),
+        NVARCHAR(256),
+        # Fail-open contract: a type we don't recognize keeps the comparison
+        UndeterminedType(),
+    ],
+    ids=lambda t: type(t).__name__,
+)
+def test_empty_string_comparison_for_string_types(sqa_type, dialect):
+    """String-like and unrecognized types keep counting '' as missing"""
+    query = compile_null_missing_count(sqa_type, dialect)
+
+    assert "IS NULL" in query
+    assert EMPTY_STRING_COMPARISON in query
+
+
+@pytest.mark.parametrize("dialect", DIALECTS, ids=lambda d: d.name)
+@pytest.mark.parametrize("data_type", NON_EMPTY_STRING_DATA_TYPES, ids=lambda dt: dt.value)
+def test_supported_om_types_that_cannot_hold_empty_string(data_type, dialect):
+    """Guard the OM DataType -> ORM type mapping, not just the ORM types"""
+    # Reaching into _TYPE_MAP on purpose: the mapping itself is what we're pinning
+    sqa_type = CommonMapTypes._TYPE_MAP[data_type]
+    query = compile_null_missing_count(sqa_type(), dialect)
+
+    assert EMPTY_STRING_COMPARISON not in query