Skip to content

OLAP Connectors: Add DSN configuration option #6870

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

Merged
merged 12 commits into from
Mar 27, 2025
Merged

Conversation

ericpgreen2
Copy link
Contributor

@ericpgreen2 ericpgreen2 commented Mar 11, 2025

Closes #6579

image image

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated (FYI @royendo for docs updates, please!)
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@ericpgreen2 ericpgreen2 self-assigned this Mar 11, 2025
@ericpgreen2 ericpgreen2 marked this pull request as ready for review March 12, 2025 12:29
@royendo
Copy link
Contributor

royendo commented Mar 12, 2025

definitely will need to look at revisiting
https://docs.rilldata.com/reference/olap-engines/clickhouse but that seems to be about it :)

@ericpgreen2
Copy link
Contributor Author

definitely will need to look at revisiting https://docs.rilldata.com/reference/olap-engines/clickhouse but that seems to be about it :)

Sounds good – just note that Druid & Pinot will have this DSN option too

@k-anshul
Copy link
Member

LGTM from an experience POV.

One minor thing that I noticed(exists on main not introduced in this PR) is that if clickhouse modeling is disabled then I can't add one more clickhouse connection from UI. The data option is hidden. I need to manually create another connection.yaml

@ericpgreen2
Copy link
Contributor Author

One minor thing that I noticed(exists on main not introduced in this PR) is that if clickhouse modeling is disabled then I can't add one more clickhouse connection from UI. The data option is hidden. I need to manually create another connection.yaml

Thanks, Anshul. Addressed this comment here: #6904

@ericpgreen2 ericpgreen2 requested a review from AdityaHegde March 26, 2025 13:28
@ericpgreen2 ericpgreen2 merged commit f3c12c1 into main Mar 27, 2025
11 checks passed
@ericpgreen2 ericpgreen2 deleted the olap-connector-dsn-form branch March 27, 2025 19:01
grahamplata pushed a commit that referenced this pull request Apr 14, 2025
* Fix bug in connector explorer

* Add alternate form for DSN

* Add tests

* Cleanup

* Give the DSN form its own error state

* Remove debug line

* Add `waitForURL` to tests

* Update locator

* Use native Clickhouse protocol for placeholder
grahamplata pushed a commit that referenced this pull request Apr 14, 2025
* Fix bug in connector explorer

* Add alternate form for DSN

* Add tests

* Cleanup

* Give the DSN form its own error state

* Remove debug line

* Add `waitForURL` to tests

* Update locator

* Use native Clickhouse protocol for placeholder
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.

Connectors UI: Provide an option to connect via DSN
4 participants