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

Restructuring Database.Connection to allow for database specific types. #3632

Merged
merged 29 commits into from
Sep 7, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Aug 4, 2022

Pull Request Description

  • Added databases, database, set_database.
  • Added schemas, schema, set_schema.
  • Added table_types,
  • Added tables.
  • Moved the vast majority of the connection work into a lower level JDBC_Connection object.
  • Connection represents the standard API for database connections and provides a base JDBC implementation.
    • SQLite_Connection has the Connection API but with custom databases and schemas methods for SQLite.
    • Postgres_Connection has the Connection API but with custom set_database, databases, set_schema and schemas methods for Postgres.
  • Updated Redshift - no public API change.

Checklist

  • Note: the Redshift connection does not pass the Redshift tests at the moment.
    • This is not introduced by this PR but was already the case.
    • Column names are returned in lower case.
    • Order of results is not stable.

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide dist and ./run ide watch.

@jdunkerley jdunkerley force-pushed the wip/jd/database-connections-182652055 branch from 1719945 to 9217cb6 Compare August 4, 2022 13:53
@radeusgd
Copy link
Member

Having both properties database and databases seems slightly confusing to me - when I would look at the code I wouldn't be sure what is exactly the difference between the two. I may guess correctly, and also documentation should make it clear. But maybe we could rename database to something like current_database? Then it would be clear that this is the currently selected one and databases are all available ones.

The same applies for schema and schemas.

@jdunkerley jdunkerley force-pushed the wip/jd/database-connections-182652055 branch from baef070 to 3c2776b Compare August 23, 2022 07:49
@jdunkerley
Copy link
Member Author

Having both properties database and databases seems slightly confusing to me - when I would look at the code I wouldn't be sure what is exactly the difference between the two. I may guess correctly, and also documentation should make it clear. But maybe we could rename database to something like current_database? Then it would be clear that this is the currently selected one and databases are all available ones.

The same applies for schema and schemas.

The database/databases and schema/schemas was discussed with Ned and Wojciech in Gdansk. It was felt it was clear and fairly simple but we could rename if it doesn't feel that way within the UI experience.

@jdunkerley jdunkerley force-pushed the wip/jd/database-connections-182652055 branch 2 times, most recently from d58e2a4 to 8587b14 Compare September 5, 2022 15:22
@JaroslavTulach
Copy link
Member

SQLite_Connection implements the Connection for SQLite overriding databases and schemas

I am curious to know what that is sentence supposed to mean? I see SQLite_Connection, but it doesn't extend Connection. I don't even know how are they related? Where does the "implements" happen?

Is the list of supported DB types hardcoded currently? How can one connect to SQLite and how can one connect to Postgres?

Is there a way to expand the list of supported DB now? In the future? Where I would add support of such a DB?

@jdunkerley
Copy link
Member Author

SQLite_Connection implements the Connection for SQLite overriding databases and schemas

I will re-word it. The specific connection types are supposed to implement the same API as the Connection type. We have no way to do inheritance (either as an interface or a derived type). The idea is all specific implementations will share the API and can use the basic implementation in Connection if they don't need any custom behaviour.

Is the list of supported DB types hardcoded currently? How can one connect to SQLite, and how can one connect to Postgres?

Is there a way to expand the list of supported DB now? In the future? Where I would add support of such a DB?

Currently, the supported databases are SQLite, Postgres and Redshift. They will be presented as a union type on the Database.connect method. It is envisaged that a modular expansion approach will be added in future. Currently (and for v1), adding a new database such as SQL Server would involve expanding the Standard.Database library with the required types.

@jdunkerley jdunkerley force-pushed the wip/jd/database-connections-182652055 branch from fb768b1 to 04b3945 Compare September 6, 2022 16:21
@jdunkerley jdunkerley marked this pull request as ready for review September 6, 2022 16:25
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a few small suggestions.

@jdunkerley jdunkerley force-pushed the wip/jd/database-connections-182652055 branch from c12f2b9 to 0a3e753 Compare September 7, 2022 11:34
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Sep 7, 2022
@mergify mergify bot merged commit 2b425f8 into develop Sep 7, 2022
@mergify mergify bot deleted the wip/jd/database-connections-182652055 branch September 7, 2022 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants