Skip to content

OBSL-1027: SQL AST window functions + UNION-in-CTE#2776

Open
mivds wants to merge 64 commits into
mainfrom
obsl-1027-sql-ast-window-functions-and-cte-union
Open

OBSL-1027: SQL AST window functions + UNION-in-CTE#2776
mivds wants to merge 64 commits into
mainfrom
obsl-1027-sql-ast-window-functions-and-cte-union

Conversation

@mivds

@mivds mivds commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR chain (soda-core)

  1. OBSL-1013: ship table discovery in soda-core (soda data-source discover) #2773 — ship table/column discovery
  2. OBSL-1025: emit metrics[] in scan-results payload when measurements present #2774 — emit metrics[] in scan-results payload
  3. OBSL-1027: SQL AST window functions + UNION-in-CTE #2776 — SQL AST window functions + UNION-in-CTE ← this PR
  4. OBSL-1019/1007 (soda-core part): raw feature-config passthroughs on the dataset-config DTO + MM/dialect seams #2777 — raw feature-config passthrough (config DTO)
  5. All-datasource dialect groundwork: time/interval, percentile, typed timestamp literal, NaN filter #2787 — all-datasource dialect groundwork

Summary

Adds the SQL-AST primitives the v3 numeric profiler needs, unblocking the profiling SQL port (OBSL-1017). Scope was determined by the OBSL-1014 spike.

  • WINDOW_FUNCTION AST node (sql_ast.py) — name, args, optional partition_by: list[COLUMN], optional order_by (reuses ORDER_BY_ASC/ORDER_BY_DESC). Renders NAME(args) OVER ([PARTITION BY ...] [ORDER BY ...]).
  • UNION-in-CTE — widened CTE.cte_query to accept UNION (covers UNION_ALL); _build_cte renders it via build_union_sql.
  • Rendering reuses existing function-arg + ORDER BY logic; additive dispatch branch in build_expression_sql.

Rendered output examples (asserted in tests): ROW_NUMBER() OVER (ORDER BY "value_" DESC), SUM("amount") OVER (PARTITION BY "category" ORDER BY "id" ASC), CTE bodies containing UNION / UNION ALL.

Stacked on #2774 (OBSL-1025) → #2773 (OBSL-1013). Closes the AST gap from the OBSL-1014 spike.

Test Plan

  • 7 new unit tests asserting rendered SQL (window fns w/ partition+order, no-clause, multi-col partition, CTE UNION/UNION ALL)
  • Regression: soda-tests/tests/unit → 993 passed, 2 skipped, 0 failures
  • pre-commit (isort/black/autoflake) clean

🤖 Generated with Claude Code

mivds and others added 12 commits June 30, 2026 15:44
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ect DQN via dialect hooks

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… DQNs

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nmatch

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… present (OBSL-1025)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…E (OBSL-1027)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mivds and others added 8 commits July 1, 2026 23:30
…handler-level test (OBSL-1013)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…to obsl-1027-sql-ast-window-functions-and-cte-union
…core' into obsl-1025-extend-payload-builder-with-metrics-field
…t (OBSL-1027)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The handler discovers with unscoped prefixes, which doesn't surface the
test table on databricks/sparkdf shared metastores. The test verifies
the handler's connection open/close lifecycle, which is
dialect-independent, so one dialect suffices.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…core' into obsl-1025-extend-payload-builder-with-metrics-field
…to obsl-1027-sql-ast-window-functions-and-cte-union
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mivds mivds self-assigned this Jul 4, 2026
mivds and others added 8 commits July 5, 2026 01:35
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…core' into obsl-1025-extend-payload-builder-with-metrics-field
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…to obsl-1027-sql-ast-window-functions-and-cte-union
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add `"scanId": os.environ.get("SODA_SCAN_ID", None)` to build_discovery_payload
- Extract resolve_scan_definition_name helper with precedence: CLI arg > SODA_SCAN_DEFINITION env > default
- Use resolve_scan_definition_name in handle_discover_data_source
- TDD: 5 new unit tests covering scanId env set/unset and all 3 resolution precedence levels

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…L-1013)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mivds and others added 17 commits July 13, 2026 13:33
All CLI dependency resolvers now share one contract: expected/unusable
configuration shapes (missing flag, unparseable/invalid config,
validation rejection) raise ScanExecutionFailedException carrying the
user-facing message — nothing logged at the raise site — while genuinely
unexpected failures (e.g. ImportError from a missing plugin) propagate
raw so the caller logs them with the traceback, preserving the
deliberate traceback-for-data-source-creation asymmetry.

run_with_failure_reporting sheds all construction knowledge: it takes
the already-constructed reporting channel (soda_cloud) and a zero-arg
command, and owns only the Logs lifecycle, the two log-form arms (SEFE
clean, everything else with traceback) and failure reporting. The
discover wiring resolves the reporting channel first in a small guard
(unusable → exit 4, nothing can be reported); the data source and the
mandatory scan definition name resolve inside the wrapped command so
their failures take the standard mark-with-logs mapping. The local flow
adopts the same exception contract with ad-hoc semantics (log →
LOG_ERRORS). One nuance: cloud-resolution failure records are no longer
captured by the wrapper's Logs — they were never deliverable anyway.

ScanExecutionFailedException now extends SodaCoreException (audited: the
only 'except SodaCoreException' site wraps contract-verification query
execution, which the discover SEFE path never crosses), and
SodaCoreException's docstring states its real role as the package-wide
base class.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Both resolvers add YamlParserException to their expected-shape catch
  tuples: a YAML syntax error and a nonexistent file path — the two most
  common user mistakes — raise it from the parsing layer and now map to
  a clean ScanExecutionFailedException message instead of a raw
  traceback. The tuples are symmetric across the two resolvers.
- Root fix in SodaCloud.from_yaml_source: a config missing the top-level
  'soda_cloud' key now raises InvalidSodaCloudConfigurationException
  (same contract as the sibling api_key checks) instead of logging a
  debug line and crashing on a None dereference.
- exit_with_code is annotated NoReturn so bindings after guard except
  arms (e.g. soda_cloud in the discover wiring) are provably assigned.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
send_discovery_results reached into SodaCloud._execute_command from the
discovery module; the transport now lives where it belongs:

- SodaCoreInsertScanResultsDTO (soda_cloud_dto): TypedDict mirroring the
  backend SodaCoreInsertScanResultsCommand field set — required keys are
  the backend's @NotNull/@notempty fields plus the type discriminator,
  everything else optional via a total=False split (PEP 563-safe
  __required_keys__). Intended shared shape for all result-inserting
  flows; profiling adopts it in soda-extensions.
- SodaCloud.insert_scan_results(payload) -> bool: owns the
  _execute_command call, same accepted-bool contract as
  mark_scan_as_failed. The contract flow keeps its richer
  send_check_collection_results (response post-processing) — deliberate,
  converged separately.
- build_discovery_payload returns the DTO; send_discovery_results is
  deleted; the discovery handler calls soda_cloud.insert_scan_results.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n tests, wiring complexity

- dependencies.py: drop pydantic ValidationError from the resolver catch
  tuples — it subclasses ValueError in pydantic v2, so the tuple entry was
  dead; the coverage is now noted in the resolver docstrings.
- test_cli_dependencies.py / test_cli_data_source_handlers.py: hoist path
  construction and mock setup out of pytest.raises blocks so only the call
  under test can throw; every assertion is kept.
- cli.py: extract _resolve_soda_cloud_for_discovery_or_exit and
  _discover_data_source_locally from the discover wiring to bring its
  cognitive complexity under the threshold; behavior unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cated resolvers

- FabricSqlDialect.is_system_schema extends (not replaces) the base
  information_schema exclusion: discovery now includes views, so Fabric
  was the one dialect where INFORMATION_SCHEMA objects would surface as
  discoverable datasets.
- The local discovery handler no longer catch-logs: failures propagate
  raw and the local wiring — the single logging site — maps them to
  LOG_ERRORS, so connection-open failures are no longer mislabeled as
  query failures and the wiring's generic arm is reachable again.
- resolve_scan_definition_name moves to handlers/dependencies.py next to
  the other resolvers (its tests move to test_cli_dependencies.py).
- discovery_payload reads the scan id through EnvConfigHelper, the
  canonical accessor.
- The DTO's type discriminator is Literal["sodaCoreInsertScanResults"].

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
run_with_failure_reporting owns a Logs() collector and reports its records
on failure, but until now the wrapped command received nothing. A command
that constructs its own inner Logs (e.g. via execute_check_collections with
logs=None building per-impl Logs) displaced the wrapper's collector, so
records emitted downstream never reached the wrapper's failure report.

The command now receives the wrapper's own logs, so a command running a
check-collection session can thread it through (each impl built with
logs=logs) and keep the failure report complete — matching the shared-logs
pattern #2779 relies on. Discovery constructs no inner Logs (DiscoveryRun
emits via soda_logger, which already lands in the active wrapper collector),
so its wiring accepts and ignores the logs argument.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@m1n0

m1n0 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Reviewed. Clean — no findings. The new node follows the existing patterns closely: __post_init__ parent-node wiring matches FUNCTION's, the ASC/DESC rendering is the same isinstance idiom the SELECT-level order-by builder already uses (sql_dialect.py:1152), UNION_ALL subclasses UNION so the CTE branch covers both, and .AS() aliasing works generically via the ALIAS wrapper. Rendering is pinned by tests incl. the OVER () edge case. CI green.

Two passing notes, neither actionable here:

  • partition_by is list[COLUMN] rendered via _build_column_sql, so partition-by-expression isn't supported — fine for the profiler port, just flagging the boundary.
  • CTE-UNION now leans on build_union_sql, which carries a pre-existing "only tested/verified for Postgres" TODO (sql_dialect.py:683) — the profiling SQL port will inherit that caveat on other dialects.

@mivds

mivds commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

No code changes — agreed it's clean. The two boundary notes are real but out of scope here: partition-by-expression (partition_by is list[COLUMN]) isn't needed for the profiler port, and build_union_sql's "Postgres-only" TODO is pre-existing and inherited by the profiling SQL port on other dialects. Flagging both for the follow-up rather than expanding this PR.

@mivds
mivds deleted the branch main July 20, 2026 16:03
@mivds mivds closed this Jul 20, 2026
@mivds mivds reopened this Jul 20, 2026
@mivds
mivds changed the base branch from obsl-1025-extend-payload-builder-with-metrics-field to main July 20, 2026 16:04
…dow-functions-and-cte-union

# Conflicts:
#	soda-core/src/soda_core/cli/cli.py
#	soda-core/src/soda_core/cli/handlers/data_source.py
#	soda-core/src/soda_core/common/soda_cloud.py
#	soda-postgres/src/soda_postgres/common/data_sources/postgres_data_source.py
#	soda-postgres/tests/unit/test_postgres_dialect.py
#	soda-tests/tests/integration/test_discovery.py
#	soda-tests/tests/unit/test_cli_data_source_handlers.py
#	soda-tests/tests/unit/test_cli_discover.py
#	soda-tests/tests/unit/test_discovery_payload.py
#	soda-tests/tests/unit/test_payload_metrics_field.py
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants