feat(ingestion): Databricks E2E CLI tests + tableDiff PAT extraction fix#27963
Open
feat(ingestion): Databricks E2E CLI tests + tableDiff PAT extraction fix#27963
Conversation
…AT fix Add a Databricks E2E test suite mirroring the Snowflake one (vanilla ingestion, profiler, auto-classification, lineage, filters, mark-deleted, data quality, complex types, external table lineage, plus a combined features test for tags, FK, clustering, view, and materialized view). Fix a pre-existing bug in BaseTableParameter._get_service_connection_config where the token lookup walked `connection.token` (flat) but the schema has it nested under `connection.authType.token`. The empty-token URL silently triggered OAuth U2M browser fallback in databricks-sql-connector, hanging non-interactive runs. The fix reads from authType.token first, falls back to the legacy flat path for backwards compatibility, and raises a clear ValueError when no PAT is found rather than producing a URL that hangs on a browser prompt. Relax the racy sink-record-count assertion in common/test_cli_db.py: the metadata-rest sink batches PUTs and final-flushes on workflow close — the Status object is logged before that flush so sink_status.records understates the true count. Replaced with source records + zero sink failures + (in the Databricks override) an end-to-end retrieve_table API check that proves data actually landed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
The test-connection step previously walked `catalogs.list()` and picked the first catalog (excluding `__databricks_internal`). When that first catalog was a foreign / federated catalog (Lakehouse Federation against an external Postgres / MySQL / etc.), the `information_schema.*_tags` queries get pushed down to the source DB and fail on stale or absent credentials — causing a healthy Unity Catalog workspace to report a broken test connection. Extract the catalog selection into `select_test_catalog`: - Honor `configured_catalog` from the service config when set (the user has explicitly chosen the catalog and may have foreign creds wired up). - Otherwise skip both `__databricks_internal` and any catalog whose `catalog_type` contains `FOREIGN`, picking the first remaining catalog. Add unit tests covering: configured-catalog override, foreign-catalog honoring when explicitly pinned, foreign-catalog skipping when auto- selecting, and the no-internal-catalog fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… extraction
Code review surfaced that the previous guard only checked `if token is
None`. An empty-string token (set via `token: ""` in YAML or an empty
`$E2E_DATABRICKS_TOKEN` env var) would slip past the None check, produce
a `databricks://:@host/...` URL, and silently trigger OAuth U2M browser
fallback in `databricks-sql-connector` — exactly the failure mode the
docstring warned against.
Resolve the value first (handling SecretStr, None, and any object that
stringifies to ""), then validate. This catches:
- `token = None` (missing attribute)
- `token = ""` (empty plain string)
- `token = SecretStr("")` (empty SecretStr / Pydantic field)
with a single `if not token_value` check.
Adds two unit tests covering the empty-string and empty-SecretStr cases
to lock in the behavior.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens Databricks ingestion and validation by adding a Databricks-focused CLI E2E suite, fixing PAT extraction for Databricks data-diff URL construction (to avoid OAuth browser fallback hangs), and reducing flakiness in connector E2E assertions by removing a racy metadata-rest sink record-count check.
Changes:
- Add a comprehensive Databricks CLI E2E test suite plus Databricks connector YAML configuration.
- Fix Databricks data-diff connection URL building to read the PAT from
authType.token(with legacy fallback) and raise when missing. - Relax CLI E2E assertions by removing sink record-count checks that can under-report due to batched flush timing, and add stronger Databricks-specific end-to-end API verification.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ingestion/src/metadata/ingestion/source/database/common/data_diff/databricks_base.py |
Fix PAT extraction path for Databricks data-diff URL generation and introduce explicit error on missing PAT. |
ingestion/tests/cli_e2e/common/test_cli_db.py |
Remove racy sink record-count assertions; document metadata-rest batching/flush timing. |
ingestion/tests/unit/topology/database/test_databricks_migration.py |
Add unit coverage for nested authType.token extraction and missing-token error behavior. |
ingestion/tests/cli_e2e/test_cli_databricks.py |
Introduce a Databricks CLI E2E suite covering ingestion, profiler, classification, lineage, data quality, and feature ingestion scenarios. |
ingestion/tests/cli_e2e/database/databricks/databricks.yaml |
Add Databricks E2E workflow config driven by environment variables (host/httpPath/token/catalog). |
Comment on lines
+56
to
+62
| # Resolve to the bare string before validating: an empty-string token | ||
| # (e.g. `$E2E_DATABRICKS_TOKEN` set but empty, or `token: ""` in YAML) | ||
| # would otherwise build a URL like `databricks://:@host/...` and the | ||
| # SQL driver would fall back to OAuth U2M, opening a browser. Validate | ||
| # the resolved value so we fail fast in non-interactive environments. | ||
| token_value = ( | ||
| token.get_secret_value() |
Comment on lines
+128
to
+129
|
|
||
| def test_empty_string_token_raises(self): |
Contributor
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
CI surfaced two checks failing on the previous commit:
1. Static checks: basedpyright flagged
`databricks_base.py:62: "get_secret_value" is not a known attribute of "None"`.
The conditional expression `token.get_secret_value() if hasattr(...)
else str(token)` could not be narrowed by basedpyright — it could not
infer that `hasattr` implies `token is not None`.
2. py-checkstyle: `ruff format` wanted to inline the same multi-line
conditional, but inlining it would exceed the line-length limit and
bounce against ruff again.
Restructure the resolution into explicit branches:
if token is None:
token_value = ""
elif hasattr(token, "get_secret_value"):
token_value = token.get_secret_value()
else:
token_value = str(token)
This narrows `token` cleanly for basedpyright in the elif branch, and
ruff is satisfied with the natural block layout.
Also tighten `test_token_nested_under_authtype`: replace the
`assert "..." in result` with `assert isinstance(result, str)` followed
by an exact-equality check. The `in` operator on a `str | dict | None`
union was a basedpyright operator-issue, and the equality check below
already implies the substring assertion.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review ✅ Approved 1 resolved / 1 findingsImplements comprehensive Databricks E2E CLI tests and fixes a critical bug in PAT extraction that caused silent OAuth hangs. Resolves the empty-string token authentication issue; no other issues found. ✅ 1 resolved✅ Bug: Empty-string token bypasses ValueError, causes silent OAuth hang
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Contributor
🟡 Playwright Results — all passed (14 flaky)✅ 4014 passed · ❌ 0 failed · 🟡 14 flaky · ⏭️ 86 skipped
🟡 14 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ingestion/tests/cli_e2e/test_cli_databricks.py) mirroring the Snowflake one — vanilla ingestion, profiler, auto-classification, lineage, schema/table filters, mark-deleted, data quality, complex column types (STRUCT/ARRAY/MAP), external table lineage, plus a combined Databricks-features test (table tag, column tag, FK informational constraint, liquid clustering, view, materialized view).BaseTableParameter._get_service_connection_configlookup bug indatabricks_base.pythat was readingconnection.token(flat) when the schema actually nests it underconnection.authType.token. The empty-token URL silently fell back to OAuth U2M browser auth indatabricks-sql-connector, hanging non-interactivetableDiffruns. The fix reads fromauthType.tokenfirst, falls back to the flat path for backwards compat, and raises a clearValueErrorrather than producing a hang-on-browser URL when no PAT is found.common/test_cli_db.py(used by every connector E2E test). Themetadata-restsink batches PUTs and final-flushes on workflow close, but Status is logged before that flush — sosink_status.recordsunderstates the true count. Replaced with source records + zero sink failures + (in the Databricks override) an end-to-endretrieve_tableAPI check that proves data actually landed. Should also un-flake other connectors that hit the same assertion.Files changed
ingestion/src/metadata/ingestion/source/database/common/data_diff/databricks_base.pyauthType.tokenlookup; raise on missing PAT instead of silent OAuth fallbackingestion/tests/cli_e2e/common/test_cli_db.pyingestion/tests/unit/topology/database/test_databricks_migration.pyingestion/tests/cli_e2e/test_cli_databricks.pyingestion/tests/cli_e2e/database/databricks/databricks.yamlTest plan
pytest ingestion/tests/unit/topology/database/test_databricks_migration.py— all 8 unit tests pass (3 new + 5 existing)pytest ingestion/tests/cli_e2e/test_cli_databricks.py::DatabricksCliTest::test_vanilla_ingestion— passes against a real Databricks Unity Catalog workspacepytest ingestion/tests/cli_e2e/test_cli_databricks.py::DatabricksCliTest::test_data_quality— XPASS after thedatabricks_base.pyfix (previously hung opening a browser)pytest ingestion/tests/cli_e2e/test_cli_databricks.py::DatabricksCliTest::test_complex_column_types— passespytest ingestion/tests/cli_e2e/test_cli_databricks.py::DatabricksCliTest::test_databricks_features_ingestion— passes (FK assertion soft-warns due to a separate Databricks-dialect FK-extraction gap, documented inline)pytest --collect-onlylists all 16 testsnox --no-venv -s static-checks— no new errors above baseline (verified by stashing changes; baseline output is byte-identical)Pre-reqs to run the E2E suite locally
Catalog must already exist in UC (
CREATE CATALOGrequires metastore-admin). Schemas and tables are created/torn down bysetUp/tearDownClass.Known gaps (out of scope for this PR)
information_schema.referential_constraints. The features test's FK assertion soft-warns. Worth a follow-up to addget_foreign_keysonDatabricksDialect.E2E_DATABRICKS_EXTERNAL_LOCATIONis set to a UC external location the principal can write to.🤖 Generated with Claude Code
Summary by Gitar
select_test_catalogto automatically skip__databricks_internaland federated (FOREIGN) catalogs.information_schemaqueries to external sources with stale credentials.test_unity_catalog_connection.pycovering manual catalog overrides and filtering logic for various catalog types._extract_pat_tokento explicitly handleNoneand improve string conversion logic for Databricks PATs.ValueErrorrather than triggering silent OAuth fallback.This will update automatically on new commits.