Configurable DBQL lookback and row cap for the Teradata profiler - #2644
Configurable DBQL lookback and row cap for the Teradata profiler#2644dey-abhishek wants to merge 18 commits into
Conversation
Add ${name} substitution variables to profiler pipelines, resolved with precedence: packaged pipeline defaults < user credentials file < explicit overrides. Values are validated against a safe character set before being interpolated into SQL sent to the source. The Teradata core DBQL extract now uses ${lookback_days} and ${max_rows}, prompted for during configure-database-profiler and stored under the source's profiler section in the credentials file so they survive upgrades. Defaults (7 days, 100000 rows) preserve prior behavior.
|
|
dey-abhishek
added
feat/profiler
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2644 +/- ##
==========================================
+ Coverage 71.25% 71.57% +0.32%
==========================================
Files 112 112
Lines 10119 10144 +25
Branches 1111 1113 +2
==========================================
+ Hits 7210 7261 +51
+ Misses 2695 2657 -38
- Partials 214 226 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
✅ 181/181 passed, 8 flaky, 2 skipped, 2h47m38s total Flaky tests:
Running from acceptance #5433 |
There was a problem hiding this comment.
A few immediate topics that we need to address before getting into the finer details.
-
Before we implement or support this, the Teradata profiler needs to be updated to support the streaming interface. Without this we end up with unbounded memory use and allowing users to configure the look-back will only make that more problematic than it already is. (This could be a loose thread that unravels somewhat… we will need to ensure that the streamed batches all have the same schema, and verify that every query has paired DDL in place.)
-
Sorry, but we can't do variable substitution in an adhoc manner using a custom implementation: it's unsafe and will invariably be duplicated down the track into more places. Implementations like this are always brittle: only database-specific drivers (from the vendor) know how to do this safely and properly. (Custom interpolation also prevents us from improving our quality checks on SQL down the track.) The only safe way to allow for interpolation like this is to use prepared statements with execution-time binding. (In some cases this will lead to changes to the queries because variables can't be bound into all locations of the query, but that's a compromise we must accept.)
Addresses review feedback: replace the custom ${name} string substitution with driver
execution-time bind parameters, so values are never spliced into SQL text. fetch()/stream()
now accept a parameters mapping; the SQLAlchemy-based connectors bind via :name placeholders
(translated to the driver paramstyle), and the raw-DBAPI connectors reject parameters until
binding is implemented for them.
The Teradata DBQL core extract is rewritten so both values sit in bindable positions:
lookback via CURRENT_DATE - :lookback_days, and the row cap via
QUALIFY ROW_NUMBER() OVER (ORDER BY TotalFirstRespTime DESC) <= :max_rows (TOP cannot be bound).
Renames the pipeline 'variables' concept to 'parameters'. Validated end-to-end against a live
Teradata box: the core extract step completes and returns rows.
Implement the streaming interface for TeradataConnector so profiler SQL steps pull results in bounded fetchmany() batches rather than materializing the whole result set in memory. Each batch is built as an Arrow table with a schema derived once from the cursor description, so every batch of a query shares one schema regardless of which columns are all-NULL in a given batch (batches are appended positionally into the DDL-pre-created DuckDB table). Validated against a live Teradata box: the streamed DuckDB output is schema-identical to the previous buffered path, with no rows lost or altered.
…ack' into feat/profiler/configurable-lookback
|
Thanks for the review — both points addressed. @asnare 1. Streaming prerequisite. Implemented the streaming interface for Validated end-to-end against a live Teradata box: all steps complete, and the streamed DuckDB extract is schema-identical to the previous buffered path with no rows lost or altered (the fetch-path row set is a strict subset of the streamed one, differing only by live query-log growth between runs). 2. Custom substitution → execution-time bind parameters. Removed the
|
- Scope profiler bind parameters to the profiled source (PipelineClass now takes source_system) instead of returning the first 'profiler' block in a shared credentials file. - Validate configured lookback_days/max_rows are >= 1 so 0/negative can't yield a silent empty or future-dated extract. - Pass bind parameters to source_ddl steps too, for parity with sql steps. - Carry Teradata DECIMAL columns as exact text rather than float64 to avoid rounding counters beyond 2**53; DuckDB casts to the declared column type. - Normalize tz-aware datetimes to UTC-naive (and strip tzinfo from times) before building Arrow batches, so a TIMESTAMP WITH TIME ZONE column can't crash the streamed step.
…ack' into feat/profiler/configurable-lookback
The PDCR history extracts and the ResUsageSpma system usage/node queries had hardcoded lookback windows (180/60/30 days) while only the core DBQL extract honored a configurable window. Add pdcr_lookback_days (180), sys_usage_lookback_days (60), and sys_nodes_lookback_days (30) as bind parameters alongside the existing lookback_days/max_rows, wired through pipeline_config.yml and the Teradata configurator prompts. Also refresh the docs: correct the parameter reference and drop the obsolete --variant flag (Teradata core + PDCR now extract automatically in one run, with PDCR steps skipped when PDCRINFO is absent).
Raise the packaged lookback_days default from 7 to 30 (pipeline_config.yml and the configure-database-profiler prompt), and refresh the docs accordingly. Still overridable via the credentials profiler section.
The Teradata Arrow-batch streaming (TeradataConnector.stream/_arrow_batches, the Arrow helper functions, and their tests) is split out to feat/profiler/teradata-streaming so this branch carries only the profiler parameterization. The execution-time bind-parameter plumbing stays: fetch/stream still accept a parameters mapping, and Snowflake streaming (already in main) is unaffected. Teradata now buffers via fetch() as it does on main.
…ack' into feat/profiler/configurable-lookback
The teradata.mdx updates (parameter reference and the --variant correction) move out of the parameterization branch; docs are handled on the streaming branch (#2649). This branch is now code-only.
Add the profiler settings prompts to the configure-database-profiler example and document the five bind parameters (lookback_days, pdcr_lookback_days, sys_usage_lookback_days, sys_nodes_lookback_days, max_rows) with their SQL predicates and defaults. The --variant/Workload-Extraction doc corrections are handled separately on the streaming branch (#2649).
…rable-lookback # Conflicts: # src/databricks/labs/lakebridge/resources/assessments/teradata/pdcr/td_pdcr_info_agg_extract.sql # src/databricks/labs/lakebridge/resources/assessments/teradata/pipeline_config.yml
3 participants
Changes
What does this PR do?
Makes the Teradata profiler's look-back windows and the DBQL row cap configurable via execution-time bind parameters, with defaults that preserve (or lightly update) the previous behavior. Previously each of these was hardcoded in SQL:
lookback_days(default30) — core DBQL extract:CollectTimeStamp >= CURRENT_DATE - :lookback_days(raised from the previous hardcoded 7-day window)pdcr_lookback_days(default180) — PDCR history extracts (PDCRINFO.DBQLogTbl_Hst):LogDate >= date - :pdcr_lookback_dayssys_usage_lookback_days(default60) — system usage aggregation (DBC.ResUsageSpma):TheDate >= date - :sys_usage_lookback_dayssys_nodes_lookback_days(default30) — node hardware inventory (DBC.ResUsageSpma):TheDate >= date - :sys_nodes_lookback_daysmax_rows(default100000) — core DBQL row cap:QUALIFY ROW_NUMBER() OVER (ORDER BY TotalFirstRespTime DESC) <= :max_rowsThe windows are intentionally independent:
DBC.DBQLogTblis the short-lived live table, whilePDCRINFOretains far more history, so collapsing them onto one value would either shrink the PDCR extract or overrun the live table's retention. All are prompted for byconfigure-database-profilerand stored under the source'sprofilersection in the user-owned credentials file; the packaged defaults keep existing setups working.Relevant implementation details
DatabaseConnector.fetch()/stream()take aparametersmapping; the SQLAlchemy-based connectors bind:nameplaceholders (translated to the driver's paramstyle), so values are never spliced into SQL text.TOPcan't take a bind parameter, so the row cap is expressed withQUALIFY ROW_NUMBER(). Parameters are bound globally to every step, so the core DBQL extract, both PDCR steps, and the twoResUsageSpmaqueries pick up their windows automatically.>= 1so0/negative can't silently produce an empty (or future-dated) extract.Caveats/things to watch out for when reviewing:
NotImplementedErroruntil binding is implemented for them; no current pipeline passes them any.optional: true, so whenPDCRINFOis absent they are markedABSENTand tolerated and the run still succeeds.Functionality
databricks labs lakebridge configure-database-profilerTests
make fmtclean (black/ruff/mypy, pylint 10.00/10) andmake testpassingPDCRINFOis absent