From 415db2b6bee0b4c50e63ea2dedffc4dff4f967f5 Mon Sep 17 00:00:00 2001 From: treff7es Date: Thu, 2 Jul 2026 17:28:08 +0200 Subject: [PATCH 1/2] fix(ingestion/redshift): handle COPY CREDENTIALS in query fingerprinting Redshift `COPY ... CREDENTIALS ''` crashed query fingerprinting with `TypeError: 'Placeholder' object is not iterable`, which aborted usage extraction. Two defects combined to make it fatal: 1. generalize_query replaced the credentials Literal with a Placeholder. sqlglot's credentials_sql generator dispatches on isinstance(child, Literal) to pick the Redshift scalar form over the Snowflake key=value list; a Placeholder flips it onto the list path, which iterates the node and raises. Redact the credentials literal to a constant string instead: serialization stays on the scalar path, the secret never lands in the generalized query text, and fingerprints stay independent of the credential value. 2. get_query_fingerprint_debug only caught ValueError/SqlglotError, so the TypeError propagated and killed the pipeline. Widen the catch to include TypeError so fingerprinting degrades to the raw-text fallback instead. --- .../src/datahub/sql_parsing/sqlglot_utils.py | 20 ++++++++- .../unit/sql_parsing/test_sqlglot_utils.py | 45 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/metadata-ingestion/src/datahub/sql_parsing/sqlglot_utils.py b/metadata-ingestion/src/datahub/sql_parsing/sqlglot_utils.py index 339bbf4851b9..d934b7ec2a6b 100644 --- a/metadata-ingestion/src/datahub/sql_parsing/sqlglot_utils.py +++ b/metadata-ingestion/src/datahub/sql_parsing/sqlglot_utils.py @@ -342,6 +342,20 @@ def _strip_expression( if isinstance(node, (sqlglot.exp.In, sqlglot.exp.Values)): _simplify_node_expressions(node) elif isinstance(node, sqlglot.exp.Literal): + # A Redshift `COPY ... CREDENTIALS ''` parses the credentials + # as a Literal directly under a Credentials node. sqlglot's + # credentials_sql generator dispatches on isinstance(child, Literal) + # to choose the Redshift scalar form over the Snowflake key=value + # list; a Placeholder there routes into the list path that iterates + # the node and raises "TypeError: 'Placeholder' object is not + # iterable". Redact to a constant literal instead: serialization + # stays on the scalar path, the secret never lands in the generalized + # query text, and fingerprints stay independent of the credentials. + if ( + isinstance(node.parent, sqlglot.exp.Credentials) + and node.arg_key == "credentials" + ): + return sqlglot.exp.Literal.string("**REDACTED**") return sqlglot.exp.Placeholder() return node @@ -367,7 +381,11 @@ def get_query_fingerprint_debug( ) else: expression_sql = generalize_query_fast(expression, dialect=platform) - except (ValueError, sqlglot.errors.SqlglotError) as e: + except (ValueError, TypeError, sqlglot.errors.SqlglotError) as e: + # TypeError guards against sqlglot generator bugs that fail to serialize + # an otherwise-parseable statement (e.g. exotic COPY / credentials + # forms). Fingerprinting is best-effort with a raw-text fallback, so it + # must never propagate and abort ingestion. if not isinstance(expression, str): raise diff --git a/metadata-ingestion/tests/unit/sql_parsing/test_sqlglot_utils.py b/metadata-ingestion/tests/unit/sql_parsing/test_sqlglot_utils.py index 73aef98dcd9a..f1cfaf407cef 100644 --- a/metadata-ingestion/tests/unit/sql_parsing/test_sqlglot_utils.py +++ b/metadata-ingestion/tests/unit/sql_parsing/test_sqlglot_utils.py @@ -215,6 +215,51 @@ def test_redshift_query_fingerprint(): ) +def test_redshift_copy_credentials_generalization(): + # Regression: `COPY ... CREDENTIALS ''` used to crash generalization + # with `TypeError: 'Placeholder' object is not iterable`. The credentials + # literal was replaced with a Placeholder, which flips sqlglot's + # credentials_sql dispatch onto the Snowflake key=value path that iterates + # the node. + copy_sql = ( + "COPY my_schema.my_table FROM 's3://my-bucket/data' " + "CREDENTIALS 'aws_access_key_id=EXAMPLE;aws_secret_access_key=EXAMPLESECRET' " + "FORMAT AS PARQUET" + ) + + generalized = generalize_query(copy_sql, dialect="redshift") + + # The secret must never leak into the generalized (stored) query text. + assert "EXAMPLESECRET" not in generalized + assert "CREDENTIALS" in generalized + + # Fingerprinting must succeed and be independent of the actual secret, so + # two COPYs differing only in credentials share a fingerprint. + other_creds_sql = ( + "COPY my_schema.my_table FROM 's3://my-bucket/data' " + "CREDENTIALS 'aws_access_key_id=OTHER;aws_secret_access_key=OTHERSECRET' " + "FORMAT AS PARQUET" + ) + assert get_query_fingerprint(copy_sql, "redshift") == get_query_fingerprint( + other_creds_sql, "redshift" + ) + + +def test_get_query_fingerprint_survives_generalization_error(monkeypatch): + # Defense in depth: if generalization raises an unexpected error (e.g. a + # sqlglot generator bug on an exotic statement), fingerprinting must fall + # back to the raw text rather than propagating and killing the pipeline. + import datahub.sql_parsing.sqlglot_utils as sqlglot_utils + + def _boom(*args: object, **kwargs: object) -> str: + raise TypeError("simulated sqlglot generator failure") + + monkeypatch.setattr(sqlglot_utils, "generalize_query", _boom) + + fingerprint = get_query_fingerprint("SELECT * FROM my_table", "redshift") + assert fingerprint + + def test_query_fingerprint_with_secondary_id(): query = "SELECT * FROM users WHERE id = 123" From 4c8105d6c96e4869dddac7b4ccfde50339b36752 Mon Sep 17 00:00:00 2001 From: treff7es Date: Fri, 3 Jul 2026 13:57:35 +0200 Subject: [PATCH 2/2] =?UTF-8?q?test(ingestion/redshift):=20address=20revie?= =?UTF-8?q?w=20=E2=80=94=20hoist=20import,=20pin=20fingerprint=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move the sqlglot_utils module import to the top-level import block (OSS convention flags late imports in test bodies); monkeypatching still works since generalize_query is resolved via the module global at call time. - Pin the generalization-error fallback test to generate_hash(raw_query) instead of a bare truthiness check, so it proves the raw-text fallback path executed rather than just that a hash was returned. --- .../tests/unit/sql_parsing/test_sqlglot_utils.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/metadata-ingestion/tests/unit/sql_parsing/test_sqlglot_utils.py b/metadata-ingestion/tests/unit/sql_parsing/test_sqlglot_utils.py index f1cfaf407cef..2317862be948 100644 --- a/metadata-ingestion/tests/unit/sql_parsing/test_sqlglot_utils.py +++ b/metadata-ingestion/tests/unit/sql_parsing/test_sqlglot_utils.py @@ -5,6 +5,8 @@ import pytest import sqlglot +from datahub.sql_parsing import sqlglot_utils +from datahub.sql_parsing.fingerprint_utils import generate_hash from datahub.sql_parsing.query_types import get_query_type_of_sql from datahub.sql_parsing.schema_resolver import SchemaResolver from datahub.sql_parsing.sql_parsing_common import QueryType @@ -249,15 +251,16 @@ def test_get_query_fingerprint_survives_generalization_error(monkeypatch): # Defense in depth: if generalization raises an unexpected error (e.g. a # sqlglot generator bug on an exotic statement), fingerprinting must fall # back to the raw text rather than propagating and killing the pipeline. - import datahub.sql_parsing.sqlglot_utils as sqlglot_utils - def _boom(*args: object, **kwargs: object) -> str: raise TypeError("simulated sqlglot generator failure") monkeypatch.setattr(sqlglot_utils, "generalize_query", _boom) - fingerprint = get_query_fingerprint("SELECT * FROM my_table", "redshift") - assert fingerprint + raw_query = "SELECT * FROM my_table" + fingerprint = get_query_fingerprint(raw_query, "redshift") + # Pin the fallback path: the fingerprint must be the hash of the raw text, + # proving generalization was bypassed rather than merely "didn't raise". + assert fingerprint == generate_hash(raw_query) def test_query_fingerprint_with_secondary_id():