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

WhiteRabbit 1.0, with Snowflake support #401

Merged
merged 34 commits into from
Feb 7, 2024
Merged

Conversation

janblom
Copy link
Collaborator

@janblom janblom commented Jan 30, 2024

This is a very large pull request due to many refactorings that were needed to be able to isolate the database facing code, which contributes to modularity, simpler code, better testability and easier support for new (JDBC) databases. I do not recommend to review all code changes, but if you do want to review, start from the newly added interface StorageHandler and class Snowflake handler.

So, new in this release:

  • Snowflake support
  • integration tests for PostgreSQL, Oracle, MySQL, CSV, SAS (run with mvn verify). These tests depend on having Docker available. There is also an integration test for Snowflake, but that only runs when credentials are supplied (see README.md)
  • updated dependencies as far as possible without code changes
  • the distributable packages run with Java 1.8 and higher, but the tests can only be run with Java 17. Maven has been configured to fail if the available Java version is not 17.

One caveat: all tests that involve the GUI (Swing) fail on MacOS with UnspportedOperationException/HeadlessException.

jan-at-the-hyve and others added 30 commits July 10, 2023 17:41
Project now requires Java 17 to build. Should still produce java 8 (1.8) compatible artifacts though.
Bumps org.apache.avro:avro from 1.11.2 to 1.11.3.

---
updated-dependencies:
- dependency-name: org.apache.avro:avro
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
…apache.avro-avro-1.11.3

Bump org.apache.avro:avro from 1.11.2 to 1.11.3 in /rabbit-core
Without this change, the table panel height is always higher than
needed (when using stem table), because the stem table is counted
as one of the items in the components list. It is however shown
separately at the top, which is already accounted for by the
stem table margin.
* Refactor RichConnection into separate classes, and add an abstraction for the JDBC connection. Implement a Snowflake connection with this abstraction

* Add unit tests for SnowflakeConnector

* Added Snowflake support for SourceDataScan; added minimal test for it; some refactorings to move database responsibility to rabbit-core/databases

* Move more database details to rabbit-core/databases

* Clearer name for method

* Ignore snowflake.env

* Create PostgreSQL container in the TestContainers way

* Refactored Snowflake tests + a bit of documentation

* Fix Snowflake test for Java 17, and make it into an automated integration test instead of a unit test

* Remove duplicate postgresql test

* Make TestContainers based database tests into automated integration tests

* Suppress some warnings when generating fat jars

* Let autimatic integration tests fail when docker is not available

* Allow explicit skipping of Snowflake integration tests

* Added tests for Snowflake, delimited text files

* Switch to fully verifying the scan results against a reference version (v0.10.7)

* Working integration test for Snowflake, and some refactorings

* Some proper logging, small code improvements and cleanup

* Remove unused interface

* Added tests, some changes to support testing

* Make automated test work reliably (way too many changes, sorry)

* Rudimentary support for Snowflake authenticator parameter (untested)

* review xmlbeans dependencies, remove conflict

* extend integration test for distribution

* Restructuring database configuration. Work in process, but unit and integration tests all OK

* Restructuring database configuration 2/x. Still work in process, but unit and integration tests all OK

* Restructuring database configuration 3/x. Still work in process, but unit and integration tests all OK

* Restructuring database configuration 4/x. Still work in process, but unit and integration tests all OK

* Restructuring database configuration 5/x. Still work in process, but unit and integration tests all OK

* Restructuring database configuration 6/x. Still work in process, but unit and integration tests all OK

* Restructuring database configuration 7/x. Still work in process, but unit and integration tests all OK

* Intermezzo: get rid of the package naming error (upper case R in whiteRabbit)

* Intermezzo: code cleanup

* Snowflake is now working from the GUI. And many small refactorings, like logging instead of printing to stout/err

* Refactor DbType into an enum, get rid of DBChoice

* Move DbType and DbSettings classes into configuration subpackage

* Avoid using a manually destructured DbSettings object when creating a RochConnection object

* Code cleanup, remove unneeded Snowflake references

* Refactoring, code cleanup

* More refactoring, code cleanup

* More refactoring, code cleanup and documentation

* Make sure that order of databases in pick list in GUI is the same as before, and enforce completeness of that list in a test

* Add/update copyright headers

* Add line to verify that a tooltip is shown for a DBConnectionInterface implementing class

* Test distribution for Snowflake JDBC issue with Java 17

* cleanup of build files

* Add verification that all JDBC drivers are in the distributed package

* Add/improve error reporting for Snowflake

* Disable screenshottaker in GuiTestExtension, hoping that that is what blocks the build on github. Fingers crossed

* Better(?) naming for database interface and implementing class

* Use our own GUITestExtension class

---------

Co-authored-by: Jan Blom <[email protected]>
Fix image crop when using stem table
* Fixed a bug in the comparison for sort; let comparison report report all differences before failing

* Allow the user to specify the port for a MySQL server

* Add tests for a MySQL source database
janblom and others added 3 commits January 25, 2024 10:52
* Add automated regression tests for SAS files

* Fix problems with comparisons of test results to references

* create bypass for value mismatch that only shows up in github actions so far

* create bypass for value mismatch that only shows up in github actions so far, 2nd
* Add warehouse/database handling to StorageHandler class

* Show stdout/stderr from distribution verification when there are errors

* Pom updates to enable building on MacOS

* Update dependencies as far as possible without code changes

* Update README.md

---------

Co-authored-by: Jan Blom <[email protected]>
Copy link
Collaborator

@MaximMoinat MaximMoinat 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. I was able to build the distribution and test WhiteRabbit and RabbitInAHat applications. See inline comments for minor issues found.

It would be good to update the concept id hints before the v1.0 release. This is now based on the vocabulary version from 21-DEC-20. I can open a separate PR for that.

README.md Show resolved Hide resolved
@@ -101,16 +102,23 @@ To generate the files ready for distribution, run `mvn install`.
### Testing
Copy link
Collaborator

Choose a reason for hiding this comment

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

A considerable part of the Readme is now about development. I would propose to create a new file "CONTRIBUTING.md" (as suggested in the Github guidlines) and move the "Example in- and output" and "Development" sections to it. And the last point of 'Getting Involved', in which we can also refer to the new contributing document.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion, I will fix this in a separate commit before releasing.

…n.java


The sample size should start disabled, as the calculateNumericStats checkbox is unchecked by default.

Co-authored-by: Maxim Moinat <[email protected]>
@janblom janblom merged commit 4bfe7e8 into OHDSI:master Feb 7, 2024
1 check passed
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.

4 participants