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

Connection and Materialization in the Database Library #1546

Merged
merged 38 commits into from
Mar 9, 2021
Merged

Connection and Materialization in the Database Library #1546

merged 38 commits into from
Mar 9, 2021

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Mar 4, 2021 •
edited
Loading

Pull Request Description

Related to #1351

Includes the functionality for connecting to the databases and materializing results of queries.

Important Notes

  • The tests are currently for SQLite, most of the tests will also be ran with PostgreSQL backend, but the configuration for running Postgres tests will be added in a separate PR.

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 documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@radeusgd radeusgd added Type: Enhancement p-high Should be completed in the next sprint labels Mar 4, 2021
@radeusgd radeusgd self-assigned this Mar 4, 2021
@radeusgd radeusgd force-pushed the wip/rw/database-connection branch from 23bddcf to e1f8725 Compare March 4, 2021 18:36
kustosz
kustosz suggested changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@kustosz kustosz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some changes are needed to land this. And some are pure trolling.

distribution/std-lib/Base/src/Error/Common.enso Show resolved Hide resolved
distribution/std-lib/Database/THIRD-PARTY/org.xerial.sqlite-jdbc-3.34.0/LICENSE Show resolved Hide resolved
distribution/std-lib/Database/THIRD-PARTY/org.xerial.sqlite-jdbc-3.34.0/LICENSE Show resolved Hide resolved
distribution/std-lib/Database/THIRD-PARTY/org.xerial.sqlite-jdbc-3.34.0/LICENSE.zentus Show resolved Hide resolved
distribution/std-lib/Database/src/Connection/Connection.enso Outdated Show resolved Hide resolved
distribution/std-lib/Database/src/Connection/Database.enso Outdated Show resolved Hide resolved
distribution/std-lib/Database/src/Connection/Database.enso Outdated Show resolved Hide resolved
distribution/std-lib/Database/src/Data/Table.enso Outdated
@@ -43,14 +47,16 @@ type Table

Converts this table to a JSON structure.
to_json : Json
to_json = this.to_sql.to_json
to_json = case this.internal_columns.is_empty of
True -> "Table: no columns".to_json
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very bad json representation. Use an object with fields or something like that, not "here's a random string"

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
True -> "Table: no columns".to_json
True -> Json.from_pairs [["query", Nothing], ["message", "The table has no columns so a query cannot be generated."]]

Does this make sense?

test/Database_Tests/src/Database_Spec.enso Outdated Show resolved Hide resolved
test/Database_Tests/src/Database_Spec.enso Outdated Show resolved Hide resolved
@radeusgd radeusgd force-pushed the wip/rw/database-connection branch from ba42fcf to 57685b4 Compare March 5, 2021 18:41
@radeusgd radeusgd requested a review from kustosz March 9, 2021 14:08
kustosz
kustosz approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Please add the missing comments.

distribution/std-lib/Database/src/Connection/Connection.enso Outdated Show resolved Hide resolved
radeusgd added 23 commits March 9, 2021 18:22
@radeusgd
Fix index handling and add some tests
f403c15
@radeusgd
Implement display
3d7d58d
@radeusgd
Wrap SQLException
e076bf3
@radeusgd
Implement INSERT for testing
b563174
@radeusgd
Expression tests, fix some stuff
4ed641d
@radeusgd
repro
11303e6
@radeusgd
repro dataflow cast error
f2d0d9b
@radeusgd
Fix handling of no-column tables, add Illegal_State_Error
216b712
@radeusgd
Add Aggregation tests and index accessor
b60b3f2
@radeusgd
Tests for column wide agg
a080b6a
@radeusgd
Sorting tests
b4400a3
@radeusgd
Fix edge cases with name disambiguation on join
d604725
@radeusgd
Add some comments
dc467af
@radeusgd
Update legal review
e22efaf
@radeusgd
Close connection to avoid blocking file delete on Windows
db7154a
@radeusgd
Make builders growable
f978ab5
@radeusgd
Implement new builder-based materialization
6d4e820 
(but it doesn't work yet)
@radeusgd
Fix table access and stacktraces
ab362e1
@radeusgd
Fix row count handling when there are no columns
ebde3dc
@radeusgd
Code review
5df6c57
@radeusgd
Remove jdbc prefix from URL
f95786e
@radeusgd
Add comments regarding decimals, implement better info
75e8269
@radeusgd
Sort out stuff after rebase
796e922
@radeusgd radeusgd force-pushed the wip/rw/database-connection branch from 463d493 to 796e922 Compare March 9, 2021 17:42
@radeusgd radeusgd changed the title Database Connection and Data Materialization in the Database Library Connection and Materialization in the Database Library Mar 9, 2021
@radeusgd radeusgd merged commit 5f8af88 into main Mar 9, 2021
@radeusgd radeusgd deleted the wip/rw/database-connection branch March 9, 2021 18:52
iamrecursion pushed a commit that referenced this pull request Mar 16, 2021
@radeusgd @iamrecursion
Connection and Materialization in the Database Library (#1546)
440a7a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kustosz kustosz kustosz approved these changes

@iamrecursion iamrecursion Awaiting requested review from iamrecursion

@wdanilo wdanilo Awaiting requested review from wdanilo

Assignees

@radeusgd radeusgd

Labels
p-high Should be completed in the next sprint
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@radeusgd @kustosz