Fixes #28941: Migrate pinotdb, presto, clickhouse, teradata, exasol and iomete to BaseConnection#28942
Open
IceS2 wants to merge 2 commits into
Open
Fixes #28941: Migrate pinotdb, presto, clickhouse, teradata, exasol and iomete to BaseConnection#28942IceS2 wants to merge 2 commits into
IceS2 wants to merge 2 commits into
Conversation
…Connection Migrate six single-scheme database connectors onto BaseConnection subclasses whose get_connection_url is a @staticmethod (oracle/trino/snowflake precedent) and whose _get_client inlines the connector-specific engine build, preserving each connector's existing connectionArguments/connectionOptions handling and test-connection logic. connection_class is wired into each service spec. Colocated URL-parity unit tests are added under tests/unit/source/database/<x>/, and the shared pinotdb/presto/exasol URL asserts are graduated out of test_source_connection.py. iomete's source override and topology test are repointed to the migrated connector via the generic get_connection resolver. Fixes two latent bugs found while reading the URL builders: teradata doubled the whole URL when extra connection options were set (url += f"{url}..." instead of url = f"{url}..."), and exasol mutated connection.password inside its URL builder (now read-only). exasol also drops dead hasattr branches for databaseSchema/database, which are not fields on the model.
github-actions
Bot
added
Ingestion
safe to test
Contributor
🟡 Playwright Results — all passed (15 flaky)✅ 4272 passed · ❌ 0 failed · 🟡 15 flaky · ⏭️ 88 skipped
🟡 15 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 |
Code Review ✅ ApprovedRefactors six database connectors to use BaseConnection, consolidating connection logic while resolving URL construction bugs in Teradata and Exasol. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
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
1 participant
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.
Fixes #28941
What
Migrates six single-scheme database connectors onto
BaseConnection(foundations in #28854): pinotdb, presto, clickhouse, teradata, exasol, iomete.How
Each connector's module-level
get_connection/test_connectionare replaced by aBaseConnectionsubclass whoseget_connection_urlis a@staticmethod(oracle/trino/snowflake precedent) and whose_get_clientinlines the engine build — preserving each connector's existingconnectionArguments/connectionOptionshandling and test-connection logic (verified line-by-line against main).connection_classis wired into each service spec.Colocated URL-parity unit tests are added under
tests/unit/source/database/<x>/; the shared pinotdb/presto/exasol URL asserts are graduated out oftest_source_connection.py. iomete's source override and topology test are repointed to the migrated connector via the genericget_connectionresolver.Bug fixes (behavior change, intentional)
connectionOptionswere set (url += f"{url}…"→url = f"{url}…").get_connection_urlmutatedconnection.password— now read-only (rendered URL identical). Also drops deadhasattr(databaseSchema/database)branches (not fields on the model).Tests
ruff + basedpyright (no new errors) + 87 unit tests. db2 (scheme dispatch + SSL + runtime clidriver) is handled separately.