You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rad-pat (via GitHub)" <gi...@apache.org> on 2024/05/21 14:19:13 UTC
[PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
rad-pat opened a new pull request, #28627:
URL: https://github.com/apache/superset/pull/28627
### SUMMARY
The Databend driver changed for later dialect versions and this requires different params on the connection string. This PR includes the latest revision of the databend dialect which itself contained a number of fixes and now includes validation via SQLAlchemy testing suite.
### TESTING INSTRUCTIONS
Validate by testing against a Databend instance
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rad-pat (via GitHub)" <gi...@apache.org>.
rad-pat commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-2333846560
@betodealmeida , any chance of this progressing? Do you want me to rebase against master?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-4965850287
Turns out I _can't_ actually push to this fork/branch, so I can't help here. Would love to see this through though, one way or another.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-2124765460
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/28627?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `22.22222%` with `7 lines` in your changes are missing coverage. Please review.
> Project coverage is 64.55%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`9dda563`)](https://app.codecov.io/gh/apache/superset/pull/28627?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 155 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/superset/pull/28627?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/db\_engine\_specs/databend.py](https://app.codecov.io/gh/apache/superset/pull/28627?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fdatabend.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RhdGFiZW5kLnB5) | 22.22% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/28627?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #28627 +/- ##
==========================================
+ Coverage 60.48% 64.55% +4.07%
==========================================
Files 1931 522 -1409
Lines 76236 37490 -38746
Branches 8568 0 -8568
==========================================
- Hits 46114 24203 -21911
+ Misses 28017 13287 -14730
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/28627/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/28627/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `49.10% <22.22%> (-0.06%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/28627/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/28627/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.65% <22.22%> (-0.15%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/28627/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.55% <22.22%> (+1.07%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/28627/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.96% <22.22%> (+1.33%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/28627?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-2672466745
/korbit-review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-4739993542
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 27a297b4ccf833d57723fb3c122e3ce25b4b67b8 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a33b3429f04f30008e36ed2 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-28627--superset-docs-preview.netlify.app](https://deploy-preview-28627--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AslMSSxKTUvNSdFPz0jPzUnWLC1KTgfIpqQU5-ZWeKUAVZonGxknGJkaWaQYmacYGBgYWqcZmqSlGSrUA) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-28627--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rad-pat (via GitHub)" <gi...@apache.org>.
rad-pat commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-3883361259
> @rad-pat any intent to follow through on this one? We'll likely close it soon otherwise.
@rusackas, this is a blast from the past - we apply this change to every release we build so that we connect to Databend ok. There were some comments, and I replied but never got any further response. This works for us as it stands, should not require any database migration to upgrade existing connections. If you're happy, I can rebase on master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-4732495484
Thanks again @rad-pat - trying to get caught up on PRs again :P
The rebase is still a good idea here, and we'd love to see it through, but the Databend dialect has moved on well past 0.4.6 since. If the connection-param change is still needed against the current driver, the touchups here would be welcomed, or a fresh PR against master might be the easiest path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rad-pat (via GitHub)" <gi...@apache.org>.
rad-pat commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-2122751844
@hantmac - as discussed, here is PR for updating superset with latest dialect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rad-pat (via GitHub)" <gi...@apache.org>.
rad-pat commented on code in PR #28627:
URL: https://github.com/apache/superset/pull/28627#discussion_r1965097536
##########
superset/db_engine_specs/databend.py:
##########
@@ -273,7 +276,10 @@ def get_parameters_from_uri(
) -> BasicParametersType:
url = make_url_safe(uri)
query = url.query
- if "secure" in query:
Review Comment:
I was hoping the if...elseif... would cope with URLs of both types, so existing connections using `secure=true/false` would still work anyway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #28627:
URL: https://github.com/apache/superset/pull/28627#discussion_r1964583631
##########
superset/db_engine_specs/databend.py:
##########
@@ -217,10 +217,11 @@ class DatabendConnectEngineSpec(BasicParametersMixin, DatabendEngineSpec):
_function_names: list[str] = []
sqlalchemy_uri_placeholder = (
- "databend://user:password@host[:port][/dbname][?secure=value&=value...]"
+ "databend://user:password@host[:port][/dbname][?sslmode=value&=value...]"
)
parameters_schema = DatabendParametersSchema()
- encryption_parameters = {"secure": "true"}
+ encryption_parameters = {"sslmode": "require"}
+ encryption_disable_parameters = {"sslmode": "disable"}
Review Comment:
Oh, this is nice! If it's not asking too much, can you add this to the `BasicParametersMixin` as well, and update the logic of `build_sqlalchemy_uri` and `get_parameters_from_uri` to use it?
##########
superset/db_engine_specs/databend.py:
##########
@@ -273,7 +276,10 @@ def get_parameters_from_uri(
) -> BasicParametersType:
url = make_url_safe(uri)
query = url.query
- if "secure" in query:
Review Comment:
Can we add a migration updating existing URLs to the new format? This way upgrading Superset won't break any existing installations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "hantmac (via GitHub)" <gi...@apache.org>.
hantmac commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-2144782935
> @hantmac it seems like you maybe added the the Databend engine spec and thus I was wondering whether you would mind reviewing the PR?
Sorry for late response. I will review as soon as possible. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #28627:
URL: https://github.com/apache/superset/pull/28627
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-2123104431
@hantmac it seems like you maybe added the the Databend engine spec and thus I was wondering whether you would mind reviewing the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rad-pat (via GitHub)" <gi...@apache.org>.
rad-pat commented on code in PR #28627:
URL: https://github.com/apache/superset/pull/28627#discussion_r1965095746
##########
superset/db_engine_specs/databend.py:
##########
@@ -217,10 +217,11 @@ class DatabendConnectEngineSpec(BasicParametersMixin, DatabendEngineSpec):
_function_names: list[str] = []
sqlalchemy_uri_placeholder = (
- "databend://user:password@host[:port][/dbname][?secure=value&=value...]"
+ "databend://user:password@host[:port][/dbname][?sslmode=value&=value...]"
)
parameters_schema = DatabendParametersSchema()
- encryption_parameters = {"secure": "true"}
+ encryption_parameters = {"sslmode": "require"}
+ encryption_disable_parameters = {"sslmode": "disable"}
Review Comment:
OK, I'll try to take a look early next week
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rad-pat (via GitHub)" <gi...@apache.org>.
rad-pat commented on code in PR #28627:
URL: https://github.com/apache/superset/pull/28627#discussion_r3728310369
##########
superset/db_engine_specs/databend.py:
##########
@@ -217,10 +217,11 @@ class DatabendConnectEngineSpec(BasicParametersMixin, DatabendEngineSpec):
_function_names: list[str] = []
sqlalchemy_uri_placeholder = (
- "databend://user:password@host[:port][/dbname][?secure=value&=value...]"
+ "databend://user:password@host[:port][/dbname][?sslmode=value&=value...]"
)
parameters_schema = DatabendParametersSchema()
- encryption_parameters = {"secure": "true"}
+ encryption_parameters = {"sslmode": "require"}
+ encryption_disable_parameters = {"sslmode": "disable"}
Review Comment:
Done — `encryption_disable_parameters` is now on `BasicParametersMixin`, with `build_sqlalchemy_uri` applying it on the `else` branch and `get_parameters_from_uri` stripping it. It defaults to `{}`, so `query.update({})` is a no-op and no other spec changes behaviour.
To answer my own question from last time: there's no need for an inverse boolean. `encryption` still derives from `encryption_parameters` alone — the disable set's only job in the read path is to be *removed*, so it doesn't survive into `query` and reappear as a user-supplied extra parameter. Once I framed it that way the asymmetry I was stuck on went away.
One subtlety worth flagging for whoever reviews it: the stripping can't merge the two dicts. They typically share a key with *different* values (`sslmode=require` / `sslmode=disable`), so `{**enable, **disable}` would keep only one and leak the other. It builds a list of `.items()` pairs from both instead.
`DatabendEngineSpec` still overrides both methods, because it needs the `__default__` database substitution, the legacy `secure` fallback, and a bare `databend://` scheme (the mixin builds `engine+default_driver`, which here would be `databend+databend://` and would change every stored URI). So the mixin change is generic groundwork rather than something Databend routes through — happy to take it back out if you'd rather it landed with its first real consumer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-3882989781
@rad-pat any intent to follow through on this one? We'll likely close it soon otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rad-pat (via GitHub)" <gi...@apache.org>.
rad-pat commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-5203948558
Rebased onto master and pushed — both outstanding review points are addressed, and the branch is conflict-free again.
@rusackas — thanks for the nudge, and sorry it sat so long. Two things resolved themselves on the rebase: master is **already** on `databend-sqlalchemy>=0.5.5`, so the `pyproject.toml` bump this PR carried is simply gone, and I confirmed `sslmode` is still the parameter the current driver reads. Master had also merged `DatabendConnectEngineSpec` into `DatabendEngineSpec` since; the changes moved across cleanly.
The migration you asked for is in. Digging into it turned up something I hadn't expected — connections with *no* TLS parameter need migrating too, not just `secure=*` ones. `databend-py` defaulted to an `http` scheme and Superset only ever wrote `secure=true`, so every connection saved with the encryption box unticked has no parameter at all and was plaintext; the Rust core defaults to `https` and breaks them. Details in the thread above.
@betodealmeida — `encryption_disable_parameters` is on `BasicParametersMixin` as you asked, defaulting to `{}` so nothing else changes behaviour, and I've answered my own question from that thread (short version: no inverse boolean is needed — the disable set only ever needs *stripping* on the read path).
I also fixed some pre-existing faults in the two methods this PR touches, which is the bulk of the extra diff. `Database.parameters` was returning `{}` for every Databend connection — `get_parameters_from_uri` took `*_args` and so raised `TypeError` on the keyword call, and it mutated an `immutabledict`; both were swallowed by that property's bare `except`. Net effect: you could create a Databend connection through the dynamic form but reopening it showed an empty form, and the legacy-`secure` fallback was unreachable. Also `sslmode=enable` read as unencrypted (the driver accepts it alongside `require`), which meant a save could silently rewrite a TLS connection to `sslmode=disable`. Happy to split those into a separate PR if you'd prefer them reviewed apart from the `secure`→`sslmode` change — say the word.
@hantmac — you approved this a while back; the scope has grown since, so it's worth another look from a Databend angle if you have time, particularly the migration's treatment of parameter-less URIs.
Tests: unit coverage added for the build/parse round trip, the legacy fallback, the `encrypted_extra` keyword regression, the mixin's opt-out parameters, and the migration's upgrade/downgrade. There were none for these paths before, which is how all of the above survived.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rad-pat (via GitHub)" <gi...@apache.org>.
rad-pat commented on code in PR #28627:
URL: https://github.com/apache/superset/pull/28627#discussion_r3728312100
##########
superset/db_engine_specs/databend.py:
##########
@@ -273,7 +276,10 @@ def get_parameters_from_uri(
) -> BasicParametersType:
url = make_url_safe(uri)
query = url.query
- if "secure" in query:
Review Comment:
You were right and my earlier reply was wrong, on two counts. Migration added.
The `if/elif` I was leaning on only repopulates the **connection form** — it never touches the connection. `DatabendDialect.create_connect_args` does `parameters = dict(url.query)` and concatenates every parameter straight into the DSN handed to the Rust client, so a stored `?secure=false` reaches the driver verbatim.
Worse, that branch could never actually run. `get_parameters_from_uri` was declared `(cls, uri, *_args)` while `Database.parameters` calls it as `get_parameters_from_uri(masked_uri, encrypted_extra=...)` — `TypeError`, swallowed by that property's bare `except` into `parameters = {}`. Even called positionally it then did `url.query.pop(...)` on an `immutabledict`. So there was no runtime fallback at all; both are fixed here with regression tests.
What the driver actually does with the stale parameter (`core/src/client.rs` in `databendlabs/bendsql`):
```rust
let mut scheme = "https";
...
"sslmode" => match v.as_ref() {
"disable" => scheme = "http",
"require" | "enable" => scheme = "https",
_ => return Err(Error::BadArgument(format!("Invalid value for sslmode: {v}"))),
},
...
_ => { session_state.set(k, v); } // unknown keys are not an error
```
So `secure=false` is absorbed as a session variable and the scheme defaults to `https` — plaintext installs stop connecting, while `secure=true` survives by luck.
One thing that surprised me while writing it: connections with **no** TLS parameter need migrating too. `databend-py` took `secure=False` and set `self.schema = "http"`, and Superset only ever wrote `secure=true` (never `secure=false`), so every connection saved with the encryption box unticked has no parameter at all and was plaintext. Under the new `https` default those break in exactly the way this migration exists to prevent, so `upgrade()` pins them to `sslmode=disable`. That isn't a TLS downgrade — there was never any TLS on those rows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-3180914924
Pending changes + @betodealmeida approval ... Switching to draft to get off of our pile temporarily
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "rad-pat (via GitHub)" <gi...@apache.org>.
rad-pat commented on code in PR #28627:
URL: https://github.com/apache/superset/pull/28627#discussion_r1967582437
##########
superset/db_engine_specs/databend.py:
##########
@@ -217,10 +217,11 @@ class DatabendConnectEngineSpec(BasicParametersMixin, DatabendEngineSpec):
_function_names: list[str] = []
sqlalchemy_uri_placeholder = (
- "databend://user:password@host[:port][/dbname][?secure=value&=value...]"
+ "databend://user:password@host[:port][/dbname][?sslmode=value&=value...]"
)
parameters_schema = DatabendParametersSchema()
- encryption_parameters = {"secure": "true"}
+ encryption_parameters = {"sslmode": "require"}
+ encryption_disable_parameters = {"sslmode": "disable"}
Review Comment:
@betodealmeida, it seems straightforward to add the `encryption_disable_parameters` to the mixin, and to use it in `build_sqlalchemy_uri` based on `encryption` True/False parameter, but I'm not sure how to use it sensibly in `get_parameters_from_uri` - either encryption param is True or False determined by if the `encryption_parameters` are set.
Here is where I'm struggling to make sense of it. I'm not sure a separate parameter makes sense
```
def get_parameters_from_uri( # pylint: disable=unused-argument
cls, uri: str, encrypted_extra: dict[str, Any] | None = None
) -> BasicParametersType:
url = make_url_safe(uri)
query = {
key: value
for (key, value) in url.query.items()
if (key, value) not in cls.encryption_parameters.items()
}
encryption = bool(cls.encryption_parameters) and all(
item in url.query.items() for item in cls.encryption_parameters.items()
)
# The opposite, does this make sense?
disable_encryption = bool(cls.encryption_disable_parameters) and all(
item in url.query.items()
for item in cls.encryption_disable_parameters.items()
)
return {
"username": url.username,
"password": url.password,
"host": url.host,
"port": url.port,
"database": url.database,
"query": query,
"encryption": encryption # and not disable_encryption,
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-4741769536
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(engine): update databend engine spec for dialect version >=0.4.6 [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-5205924515
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org