feat(ingest/bigquery): profiler core rewrite + BigQueryProfilingConfig - #19486
feat(ingest/bigquery): profiler core rewrite + BigQueryProfilingConfig#19486acrylJonny wants to merge 5 commits into
Conversation
Rewrite the BigQuery profiler to support partition-aware batch kwargs, custom-SQL profiling, row sampling, staleness skipping, date-window pruning, and a per-dataset partition-metadata cache. Introduce BigQueryProfilingConfig (subclass of GEProfilingConfig) carrying the new profiling knobs and add query_executor.py (timeout-bounded, injection- guarded query execution) as a profiler dependency. Defaults are opt-in: skip_stale_tables defaults to false and partition_datetime_window_days defaults to None, so profiling behaviour is unchanged on upgrade unless explicitly enabled. updating-datahub.md documents these as new opt-in options. External-table profiling logic ships here but stays inert behind profile_external_tables (default false); it is wired up in the next PR. Part of the PR #12825 split (8/9). Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
ingestion
PR SummaryOverview Behavior / defaults: Docs add release notes for the new profiling knobs; unit tests for the profiler, security, and partition flows are greatly expanded. Reviewed by Cursor Bugbot for commit 85aac26. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 6 files
Not reviewed (too large): metadata-ingestion/tests/unit/bigquery/test_bigquery_profiler.py (~3,293 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/bigquery_config.py">
<violation number="1" location="metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/bigquery_config.py:96">
P3: With this default now `False`, `BigqueryProfiler.get_profile_request` still documents the option as defaulting to `True`. Update that comment so the opt-in behavior is not misleading to maintainers.</violation>
</file>
<file name="metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/query_executor.py">
<violation number="1" location="metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/query_executor.py:21">
P2: When a string/Hive partition or fallback value contains `data:`, `execute_query_safely` rejects the valid SELECT because `validate_sql_structure` scans quoted literals as raw SQL. Make the structural checks quote-aware so safe partition values are not skipped.</violation>
<violation number="2" location="metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/query_executor.py:43">
P3: When structural validation raises, `execute_query_safely` exits before the `try` block, so the promised DEBUG failure log and execution context are missing. Move `_validate_query_security(query)` inside the `try` block.</violation>
</file>
<file name="metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/profiler.py">
<violation number="1" location="metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/profiler.py:543">
P2: When discovery produces a non-equality predicate, `_single_partition_label` can label a multi-partition scan with `max_partition_id`. Require a recognized equality predicate before returning the single-partition label.</violation>
<violation number="2" location="metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/profiler.py:681">
P2: When deferred external discovery skips or fails a table, profiling statistics still report it as accepted because `_log_profiling_statistics` runs before the workers. Record selection statistics from successful deferred results, or update them after deferred discovery completes.
(Based on your team's feedback about Defer External Table Accounting.)</violation>
<violation number="3" location="metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/profiler.py:803">
P1: When partition discovery raises a non-`GoogleAPICallError` query failure, `get_profile_request` lets it escape and aborts the entire project’s profiling loop. Catch and report the full discovery failure set at this boundary so one transient table query does not terminate profiling for all remaining tables.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| return request | ||
|
|
||
| except ( |
There was a problem hiding this comment.
P1: When partition discovery raises a non-GoogleAPICallError query failure, get_profile_request lets it escape and aborts the entire project’s profiling loop. Catch and report the full discovery failure set at this boundary so one transient table query does not terminate profiling for all remaining tables.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/profiler.py, line 803:
<comment>When partition discovery raises a non-`GoogleAPICallError` query failure, `get_profile_request` lets it escape and aborts the entire project’s profiling loop. Catch and report the full discovery failure set at this boundary so one transient table query does not terminate profiling for all remaining tables.</comment>
<file context>
@@ -85,245 +175,885 @@ def get_profiler_instance(
+
+ return request
+
+ except (
+ ValueError,
+ AttributeError,
</file context>
| def _validate_query_security(self, query: str) -> None: | ||
| # validate_sql_structure returns False (rather than raising) for an empty or | ||
| # non-string query; treat that as a rejection instead of letting it through. | ||
| if not validate_sql_structure(query): |
There was a problem hiding this comment.
P2: When a string/Hive partition or fallback value contains data:, execute_query_safely rejects the valid SELECT because validate_sql_structure scans quoted literals as raw SQL. Make the structural checks quote-aware so safe partition values are not skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/query_executor.py, line 21:
<comment>When a string/Hive partition or fallback value contains `data:`, `execute_query_safely` rejects the valid SELECT because `validate_sql_structure` scans quoted literals as raw SQL. Make the structural checks quote-aware so safe partition values are not skipped.</comment>
<file context>
@@ -0,0 +1,67 @@
+ def _validate_query_security(self, query: str) -> None:
+ # validate_sql_structure returns False (rather than raising) for an empty or
+ # non-string query; treat that as a rejection instead of letting it through.
+ if not validate_sql_structure(query):
+ raise ValueError("Query failed structural validation (empty or malformed)")
+
</file context>
| ) | ||
|
|
||
| if len(profile_requests) == 0: | ||
| eligible_tables = len(profile_requests) + len(deferred_external) |
There was a problem hiding this comment.
P2: When deferred external discovery skips or fails a table, profiling statistics still report it as accepted because _log_profiling_statistics runs before the workers. Record selection statistics from successful deferred results, or update them after deferred discovery completes.
(Based on your team's feedback about Defer External Table Accounting.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/profiler.py, line 681:
<comment>When deferred external discovery skips or fails a table, profiling statistics still report it as accepted because `_log_profiling_statistics` runs before the workers. Record selection statistics from successful deferred results, or update them after deferred discovery completes.
(Based on your team's feedback about Defer External Table Accounting.) </comment>
<file context>
@@ -85,245 +175,885 @@ def get_profiler_instance(
)
- if len(profile_requests) == 0:
+ eligible_tables = len(profile_requests) + len(deferred_external)
+ self._log_profiling_statistics(project_id, total_tables, eligible_tables)
+
</file context>
| @staticmethod | ||
| def _predicate_scans_single_partition(partition_where: str) -> bool: | ||
| # Pure equality predicates each pin one partition value. | ||
| if not PARTITION_RANGE_OPERATOR_RE.search(partition_where): |
There was a problem hiding this comment.
P2: When discovery produces a non-equality predicate, _single_partition_label can label a multi-partition scan with max_partition_id. Require a recognized equality predicate before returning the single-partition label.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/profiler.py, line 543:
<comment>When discovery produces a non-equality predicate, `_single_partition_label` can label a multi-partition scan with `max_partition_id`. Require a recognized equality predicate before returning the single-partition label.</comment>
<file context>
@@ -85,245 +175,885 @@ def get_profiler_instance(
+ @staticmethod
+ def _predicate_scans_single_partition(partition_where: str) -> bool:
+ # Pure equality predicates each pin one partition value.
+ if not PARTITION_RANGE_OPERATOR_RE.search(partition_where):
+ return True
+ # A range that collapses to a single value per column (a zero-day window's
</file context>
| skip_stale_tables: bool = Field( | ||
| default=True, | ||
| description="Skip profiling for tables not modified within `staleness_threshold_days` " | ||
| default=False, |
There was a problem hiding this comment.
P3: With this default now False, BigqueryProfiler.get_profile_request still documents the option as defaulting to True. Update that comment so the opt-in behavior is not misleading to maintainers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/bigquery_config.py, line 96:
<comment>With this default now `False`, `BigqueryProfiler.get_profile_request` still documents the option as defaulting to `True`. Update that comment so the opt-in behavior is not misleading to maintainers.</comment>
<file context>
@@ -93,10 +93,11 @@ class BigQueryProfilingConfig(GEProfilingConfig):
skip_stale_tables: bool = Field(
- default=True,
- description="Skip profiling for tables not modified within `staleness_threshold_days` "
+ default=False,
+ description="Opt-in: skip profiling for tables not modified within `staleness_threshold_days` "
"(default 365). Uses last_altered (BigQuery's last_modified_time) for both regular and "
</file context>
| # Failures are logged at DEBUG and re-raised, never swallowed: the | ||
| # partition-detection probe relies on the exception, and the caller holds the | ||
| # report and decides whether a genuine failure warrants a report warning. | ||
| self._validate_query_security(query) |
There was a problem hiding this comment.
P3: When structural validation raises, execute_query_safely exits before the try block, so the promised DEBUG failure log and execution context are missing. Move _validate_query_security(query) inside the try block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/profiling/query_executor.py, line 43:
<comment>When structural validation raises, `execute_query_safely` exits before the `try` block, so the promised DEBUG failure log and execution context are missing. Move `_validate_query_security(query)` inside the `try` block.</comment>
<file context>
@@ -0,0 +1,67 @@
+ # Failures are logged at DEBUG and re-raised, never swallowed: the
+ # partition-detection probe relies on the exception, and the caller holds the
+ # report and decides whether a genuine failure warrants a report warning.
+ self._validate_query_security(query)
+
+ try:
</file context>
Connector Tests ResultsConnector tests failed for commit To skip connector tests, add the Autogenerated by the connector-tests CI pipeline. |
…08-profiler-core Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # metadata-ingestion/tests/unit/bigquery/test_bigquery_profiler.py # metadata-ingestion/tests/unit/bigquery/test_bigquery_profiling.py
…+ cache deferred external - P1: _build_partition_profiling_sql no longer emits TABLESAMPLE. BigQuery applies TABLESAMPLE to whole-table blocks before the partition WHERE and sizes the percentage from the whole-table row count, so a small target partition of a large table could come back empty/undersized. Rely on the WHERE + row limit and let the downstream SQLAlchemy profiler sample the materialized partition instead. - P2: the deferred external-table path now threads the dataset partition metadata cache into discovery and labels the profile with the single partition ID (type=PARTITION) when the predicate scanned exactly one partition, mirroring the inline path. - P3: correct a stale comment (skip_stale_tables is opt-in, not default-on). Tests updated to assert the partition path never samples and the unpartitioned path still does; added a regression test for the deferred external label+cache. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
maggiehays
added
the
needs-review
…tions Address reviewer findings on the profiler core: - Date windowing no longer ANDs a today-based range onto a discovered range predicate (half-open month/timestamp partitions). Columns already bounded by a range are left untouched; the today fallback applies only to unpinned columns. - Unpartitioned sampling no longer emits an inline TABLESAMPLE. The SQLAlchemy profiler adapter (mirrored by the GE profiler) already samples the source table once; emitting it here double-sampled and changed the default path vs. the pre-rewrite profiler. Sampling is deferred downstream. - The per-dataset partition-metadata cache is now guarded by a lock and warmed in the main thread before deferred external discovery fans out, so worker threads no longer race to run the dataset-wide INFORMATION_SCHEMA query. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="metadata-ingestion/tests/unit/bigquery/test_bigquery_profiler.py">
<violation number="1">
P3: This assertion is trivially true: for a sampled table _build_custom_sql returns None, so the result has no `custom_sql` key at all and `result.get("custom_sql", "")` is always empty. It cannot catch a regression that, for example, emits a row-limited custom SQL without TABLESAMPLE (losing the intended deferred sampling). Assert that sampling is actually deferred by checking `"custom_sql" not in result`, keeping the row_count assertion.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…08-profiler-core Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # metadata-ingestion/tests/unit/bigquery/test_bigquery_profiler.py # metadata-ingestion/tests/unit/bigquery/test_bigquery_profiling.py
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 85aac26. Configure here.
| # partition. Labeling that with max_partition_id would misdescribe the data | ||
| # scanned, so drop the label. | ||
| if not partition_where: | ||
| return None |
There was a problem hiding this comment.
Sharded tables lose partition labels
Medium Severity
_single_partition_label reads max_shard_id but then returns None whenever partition_where is empty. Date-sharded tables are standalone shard tables, so discovery correctly emits no WHERE; the old profiler still labeled those profiles with the shard id. Profiles for sharded tables now emit as unlabeled full-table scans even though the scanned table is exactly that shard.
Reviewed by Cursor Bugbot for commit 85aac26. Configure here.
| ): | ||
| date_columns.append(col_name) | ||
|
|
||
| return date_columns |
There was a problem hiding this comment.
Windowing skips ingestion-time columns
Medium Severity
_extract_date_columns_from_filters only treats a column as date-like if its full name or an underscore token is in DATE_LIKE_COLUMN_NAMES. BigQuery ingestion-time columns _PARTITIONDATE and _PARTITIONTIME tokenize to partitiondate / partitiontime, which are not in that set, so partition_datetime_window_days never widens the most common partition type.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 85aac26. Configure here.
|
|
||
| query_job = self.config.get_bigquery_client().query( | ||
| query, job_config=job_config | ||
| ) |
There was a problem hiding this comment.
New client created per query
Medium Severity
execute_query_safely calls get_bigquery_client() on every query, and that helper always constructs a new bigquery.Client. Partition cache and discovery issue many queries per dataset, so each run opens a fresh HTTP session instead of reusing one client the way the main BigQuery source does.
Reviewed by Cursor Bugbot for commit 85aac26. Configure here.
2 participants
Summary
Stack PR 8/9 splitting #12825 (BigQuery multi-column / external partition profiling) into reviewable slices. Base is the previous stack branch
bq-profiling/07-discovery-scanprobe; #12825 stays open as the reference until the whole stack lands.This PR rewrites the profiler core and lands its config:
profiling/profiler.py— full rewrite: partition-awareget_batch_kwargs, custom-SQL profiling, row sampling (_sample_percent/_should_sample), staleness skipping, date-window pruning, single-partition predicate detection, and a per-dataset partition-metadata cache.profiling/query_executor.py(new) — timeout-bounded, injection-guarded query execution used by the profiler.bigquery_config.py—BigQueryProfilingConfig(subclass ofGEProfilingConfig) carrying the new profiling knobs;BigQueryV2Config.profilingretyped to it (the field type upgrade landed in PR6 to satisfy discovery's build dependency; this PR completes the config surface).docs/how/updating-datahub.md— documents the new options.Design decisions applied
skip_stale_tablesdefaults tofalseandpartition_datetime_window_daysdefaults toNone. Profiling behaviour is unchanged on upgrade unless explicitly enabled. The changelog entries are worded as "new opt-in option", not "default changed".profiling_row_limitkeeps its1000000default (unchanged from feat(ingest/bigquery): support multi-column partition and external table profiling #12825).TABLESAMPLE(behaviour change vs. feat(ingest/bigquery): support multi-column partition and external table profiling #12825): BigQuery appliesTABLESAMPLE SYSTEMto whole-table storage blocks before the partitionWHERE, and sizes the sampled percentage from the whole-table row count — so a small target partition of a large table could return an empty or badly undersized sample. The partition path now applies only theWHERE+ row limit; whenuse_samplingis on, the shared SQLAlchemy profiler samples the materialized partition result instead. Unpartitioned tables still useTABLESAMPLEas before.profile_external_tables(defaultfalse) and discovery's still-stubbed external method. It is wired up in PR9. One external test that exercises the real discovery method (test_external_table_discovery_fallback_warns) is deferred to PR9 accordingly.Test plan
./gradlew :metadata-ingestion:lintFix+:lint(ruff + mypy) — clean.pytest tests/unit/bigquery/— 560 passed, including the rewrittentest_bigquery_profiler.pyand the profiler scenarios added totest_bigquery_profiling.py.Stack
Summary by cubic
Rewrites the BigQuery profiler core to support partition-aware profiling, row sampling, staleness skipping, and date-window pruning, and lands
BigQueryProfilingConfigwith the new options.INFORMATION_SCHEMAqueries and is warmed before deferred external discovery fans out; single-partition scans are labeled PARTITION while widened scans are not.query_executor.pyruns profiler queries with a timeout and injection guards.skip_stale_tablesandpartition_datetime_window_daysdefault off — so profiling behavior is unchanged on upgrade, with no migration needed.profile_external_tables(default false); the next PR wires it up.Written for commit 85aac26. Summary will update on new commits.