Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move cc_superset under databend_sqlalchemy #30

Closed
wants to merge 1 commit into from

Conversation

rad-pat
Copy link
Contributor

@rad-pat rad-pat commented Apr 19, 2024

Superset is looking for cc_superset at databend_sqlalchemy.cc_superset For issue #29

@hantmac, does secure query param still apply or should it be switched to use sslmode now?

@rad-pat
Copy link
Contributor Author

rad-pat commented Apr 19, 2024

@hantmac I'm a little confused. The cc_superset.engine code looks older than the version in superset repo. Is the superset repo the correct place to amend this now? If so, why does it still try to load engine spec from dialect?

@rad-pat
Copy link
Contributor Author

rad-pat commented Apr 19, 2024

Aha, I think I understand. The failure to load the engine spec is ok because in latest superset it already had the spec. If the superset version is the more recent engine spec then this version should be updated to be the same for people using older version of superset where the engine spec is not defined? Or, the superset.db_engine_specs entrypoint and this code needs removing from the dialect. I think possible the sslmode param needs to be handled in engine spec anyway. Please advise.

@hantmac
Copy link
Member

hantmac commented Apr 22, 2024

Aha, I think I understand. The failure to load the engine spec is ok because in latest superset it already had the spec. If the superset version is the more recent engine spec then this version should be updated to be the same for people using older version of superset where the engine spec is not defined? Or, the superset.db_engine_specs entrypoint and this code needs removing from the dialect. I think possible the sslmode param needs to be handled in engine spec anyway. Please advise.

The cc_superset no need to move to databend_sqlalchemy, the params were handled in superset code.

@rad-pat
Copy link
Contributor Author

rad-pat commented Apr 22, 2024

Aha, I think I understand. The failure to load the engine spec is ok because in latest superset it already had the spec. If the superset version is the more recent engine spec then this version should be updated to be the same for people using older version of superset where the engine spec is not defined? Or, the superset.db_engine_specs entrypoint and this code needs removing from the dialect. I think possible the sslmode param needs to be handled in engine spec anyway. Please advise.

The cc_superset no need to move to databend_sqlalchemy, the params were handled in superset code.

Yes, I now think the cc_superset should be removed from this library along with the entrypoint as the code in superset is newer, superset still tries to load (but fails) from package databend_sqlalchemy.cc_superset.engine.DatabendEngineSpec because an entrypoint exists in setup.cfg, but package actually exists at cc_superset.engine.DatabendEngineSpec. This change happened in commit a31337e. If the code did load successfully from this library, it would override the version in the superset repo. See what you think, I will close this.

@rad-pat rad-pat closed this Apr 22, 2024
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