fix(profiler): sanitize BigQuery uniqueCount SQL for TIMESTAMP/DATE and STRUCT-subfield columns (#30152) - #30538
Conversation
…nd STRUCT-subfield columns (open-metadata#30152)
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi team 👋 this PR seems blocked because #30152 is on Roadmap, not Shipping. |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
Pull request overview
This PR fixes BigQuery-specific SQL generation for the uniqueCount profiler metric (and the related uniqueness data-quality validation) by making the outer COUNTIF compare against a stable, integer-typed alias instead of reusing the source column’s name/type—resolving failures for TIMESTAMP/DATE columns and nested STRUCT subfield paths.
Changes:
- Introduces a shared
VALUE_COUNT_ALIAS = "value_count"and updates BigQueryUniqueCount.query()to usecountif(column(value_count, Integer()) == 1). - Aligns the grouped subquery/CTE projections in
SQAProfilerInterfaceandColumnValuesToBeUniqueValidatorto label the grouped count asvalue_count. - Adds BigQuery SQL-compilation unit tests covering
TIMESTAMP,DATE, and dottedSTRUCT-subfield column cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ingestion/src/metadata/profiler/metrics/static/unique_count.py | Adds VALUE_COUNT_ALIAS and updates BigQuery COUNTIF to reference an integer-typed alias. |
| ingestion/src/metadata/profiler/interface/sqlalchemy/profiler_interface.py | Ensures the grouped subquery feeding BigQuery uniqueCount labels the count output with VALUE_COUNT_ALIAS. |
| ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeUnique.py | Labels grouped CTE counts with VALUE_COUNT_ALIAS and sums the correct CTE column to match the BigQuery uniqueCount expression. |
| ingestion/tests/unit/observability/profiler/sqlalchemy/bigquery/test_unique_count_bigquery.py | Adds BigQuery dialect SQL-compilation tests for temporal types and dotted STRUCT subfields. |
| from unittest.mock import Mock, patch | ||
|
|
||
| from sqlalchemy import Column, Date, DateTime, Integer, String, create_engine | ||
| from sqlalchemy.orm import DeclarativeBase, Session | ||
| from sqlalchemy_bigquery.base import BigQueryDialect | ||
|
|
…my-bigquery not installed
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ ApprovedSanitizes BigQuery uniqueCount SQL for TIMESTAMP, DATE, and STRUCT-subfield columns by using an untyped integer column alias. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
ingestion/tests/unit/observability/profiler/sqlalchemy/bigquery/test_unique_count_bigquery.py:38
pytest.importorskip(...)is executed before several module-level imports, so the subsequent imports are no longer at the top of the file and will trigger Ruff/flake8 E402 (see similar pattern in ingestion/tests/unit/topology/database/test_vertica_type_mapping.py:16-24, which adds# noqa: E402afterimportorskip). Consider moving theimportorskipblock to just before the optionalsqlalchemy_bigqueryimport so only that import needs# noqa: E402.
import pytest
pytest.importorskip(
"sqlalchemy_bigquery",
reason="sqlalchemy-bigquery not installed — skipping BigQuery SQL compilation tests",
)
from unittest.mock import Mock, patch
from sqlalchemy import Column, Date, DateTime, Integer, String, create_engine
from sqlalchemy.orm import DeclarativeBase, Session
from sqlalchemy_bigquery.base import BigQueryDialect
Description
Fixes #30152.
On BigQuery, the
uniqueCountmetric generated invalid SQL and failed for:TIMESTAMP,DATE).STRUCTsubfield columns (e.g.,customer.email).This is distinct from #27256, which fixed a separate BYTES/binary-column type
mismatch in the same function. That fix did not resolve the TIMESTAMP/DATE or
STRUCT-subfield cases described here — confirmed still present on
mainas ofthe issue being filed.
Root Cause
TIMESTAMP/DATE):UniqueCount.querybuilt the outerCOUNTIFusingcolumn(col.name, col.type). This caused SQLAlchemy to bind parameter1matching the column's nativeTIMESTAMP/DATEtype, triggering BigQuery type conversion errors.STRUCTsubfields): The outerCOUNTIFused raw dotted identifiers (e.g.customer.email) while the subquery grouped and labeled using sanitized identifiers, resulting in unresolved column errors (400 Unrecognized name: customer) and invalid parameter names (customer.email_1).Solution
VALUE_COUNT_ALIAS = "value_count".UniqueCount.query()to constructfunc.countif(column(VALUE_COUNT_ALIAS, Integer()) == 1).profiler_interface.py(_compute_query_metrics) andcolumnValuesToBeUnique.py(_run_results) to label grouped CTE subqueries withVALUE_COUNT_ALIAS.BigQueryDialectforTIMESTAMP,DATE, andSTRUCTcolumns.Affected Modules
ingestion/src/metadata/profiler/metrics/static/unique_count.pyingestion/src/metadata/profiler/interface/sqlalchemy/profiler_interface.pyingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeUnique.pyingestion/tests/unit/observability/profiler/sqlalchemy/bigquery/test_unique_count_bigquery.pyTesting
Ran unit tests with
pytest:Greptile Summary
Updates BigQuery unique-count query generation to use a shared integer count alias.
value_count.COUNTIFreference that alias instead of the original typed or dotted column identifier.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller as Profiler / Validator participant CTE as Grouped subquery participant BQ as BigQuery Caller->>CTE: COUNT(source_column) AS value_count CTE->>BQ: Group by source_column Caller->>BQ: "COUNTIF(value_count = 1)" BQ-->>Caller: uniqueCountReviews (5): Last reviewed commit: "fix(profiler): gracefully skip BigQuery ..." | Re-trigger Greptile