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.

Sign up for GitHub

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

Jump to bottom

Update Database.connect to match new API #3542

Merged
merged 18 commits into from
Jul 4, 2022
Merged

Update Database.connect to match new API #3542

merged 18 commits into from
Jul 4, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jun 22, 2022 •
edited
Loading

Pull Request Description

Initial work restructuring the Database.connect API

  • New SQLite API with support for InMemory.
  • Updated PostgreSQL API with SSL and Client Certificate Support.
  • Updated Redshift API.

Important Notes

Follow up tasks:

  • PostgreSQL SSL additional testing.
  • Driver version updating.
  • .pgpass support.

Checklist

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-connect-design branch 2 times, most recently from f9ff328 to 290f44e Compare June 28, 2022 09:32
@jdunkerley jdunkerley changed the title Design prototype for Database.connect Update Database.connect to match new API Jun 28, 2022
@jdunkerley jdunkerley marked this pull request as ready for review June 28, 2022 10:00
radeusgd
radeusgd reviewed Jul 4, 2022
View reviewed changes
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 overall, a few missing docs should be added and besides that a few small suggestions.

distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Client_Certificate.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Connection.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Connection.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Connection/PostgreSQL.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Connection/PostgreSQL.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Redshift.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Redshift.enso Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Connection/SSL_Mode.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Dialect.enso Outdated Show resolved Hide resolved
std-bits/database/src/main/java/org/enso/database/JDBCProxy.java
private static void initialize() throws SQLException {
if (!org.postgresql.Driver.isRegistered()) {
org.postgresql.Driver.register();
public static String[] getStringColumn(ResultSet resultSet, String column) throws SQLException {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why this is implemented in Java instead of pure Enso? Seems like Vector.new_builder + regular polyglot calls would suffice.

Copy link
Member Author

Choose a reason for hiding this comment

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

one to consider during the read implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, can we have a TODO then? Otherwise I'd be worried it can likely get lost. But maybe it's not that important so it's not a problem too.

@jdunkerley jdunkerley force-pushed the wip/jd/database-connect-design branch from 89396eb to cb621f8 Compare July 4, 2022 13:53
@jdunkerley jdunkerley force-pushed the wip/jd/database-connect-design branch from 88d3936 to d8e076d Compare July 4, 2022 15:25
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jul 4, 2022
jdunkerley and others added 4 commits July 4, 2022 16:57
@jdunkerley @radeusgd
Update distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Po…
d12507b 
…stgreSQL.enso

Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
@jdunkerley @radeusgd
Update distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Re…
95911b5 
…dshift.enso

Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
@jdunkerley
PR comments.
276c951
@jdunkerley
Comments.
9c62b4a
@jdunkerley jdunkerley force-pushed the wip/jd/database-connect-design branch from d8e076d to 9c62b4a Compare July 4, 2022 15:58
@mergify mergify bot merged commit 5174cc6 into develop Jul 4, 2022
@mergify mergify bot deleted the wip/jd/database-connect-design branch July 4, 2022 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hubertp hubertp hubertp approved these changes

@radeusgd radeusgd radeusgd approved these changes

@4e6 4e6 Awaiting requested review from 4e6

@JaroslavTulach JaroslavTulach Awaiting requested review from JaroslavTulach

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jdunkerley @hubertp @radeusgd @mwu-tow