Move Generic JDBC driver support to its own non-NI library #12472

GregoryTravis · 2025-03-11T16:45:05Z

This moves generic JDBC support into its own library. This library will not be included in the native image, since dynamic loading of JDBC drivers requires JVM mode.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.
If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
or the Snowflake database integration, a run of the Extra Tests has been scheduled.
- If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

JaroslavTulach

Few comments inline.
overall this is a good approach
I'd like to see use-cases how this shall behave in IDE and CLI
in JVM and NI modes
can you please enumerate the use-cases, describe how to simulate them and what shall be the outcome?

JaroslavTulach · 2025-03-13T15:39:25Z

build.sbt

@@ -4642,6 +4645,8 @@ val `base-polyglot-root`  = stdLibComponentRoot("Base") / "polyglot" / "java"
 val `table-polyglot-root` = stdLibComponentRoot("Table") / "polyglot" / "java"
 val `image-polyglot-root` = stdLibComponentRoot("Image") / "polyglot" / "java"
 val `image-native-libs`   = stdLibComponentRoot("Image") / "polyglot" / "lib"
+val `generic-jdbc-polyglot-root` = stdLibComponentRoot("Generic_JDBC") / "polyglot" / "java"
+val `generic-jdbc-native-libs`   = stdLibComponentRoot("Generic_JDBC") / "polyglot" / "lib"


Are there going to be any native libs for Generic_JDBC? Isn't JDBC API (all that is needed) already part of the JDK?

No native libs are needed now, other than a driver that the user will provide. Possibly ODBC libs in the far future.

Can we remove the generic-jdbc-native-libs variable and its usages then?

JaroslavTulach · 2025-03-13T15:43:59Z

distribution/lib/Standard/Generic_JDBC/0.0.0-dev/src/Generic_JDBC_Connection.enso

-import project.SQL_Query.SQL_Query
-from project.Internal.Result_Set import read_column, result_set_to_table
+import Standard.Database.Connection.Connection_Options.Connection_Options
+import Standard.Database.Internal.Column_Fetcher as Column_Fetcher_Module


Importing "internal" stuff will bite us in the future. Such an cross-library import turns such "internal" stuff into API.

These could be moved to a separate SQL_Utils or Database_Utils library.

JaroslavTulach · 2025-03-13T15:48:22Z

test/Table_Tests/src/Database/JDBC/Generic_JDBC_Postgres_Spec.enso

@@ -12,6 +12,8 @@ from Standard.Test import all
 import project.Database.Helpers.Name_Generator
 from project.Database.Postgres_Spec import create_connection_builder

+import Standard.Generic_JDBC.Generic_JDBC_Connection.Generic_JDBC_Connection


Can we make Table_Tests independent of Standard.Generic_JDBC library? There are test/Generic_JDBC_Tests - why not to move these "posgress" and "sqlite" tests there?

These tests have to be run in the context of the Postgres / SQLite testing infrastructure.

The dependency of Table_Tests on "everything" including Generic_JDBC is unfortunate. Shall Table_Tests be split to core Table_Tests and various database extension _Tests running such tests in different configurations?

Yes I think we should put in a ticket to make this split. I'd like to see the tests closer to the code which would make this division more natural (but that is probably a bigger discussion than we need here)

The refactoring would be to move the test instance setup out of Table_Tests so it could be used for other tests, which would be a very large refactoring.

The Postgres and SQLite generic JDBC tests are there just to test the generic JDBC support -- they are not important for our direct Postgres and SQLite support, so a major refactoring is not warranted here. The SQLite dependency is much smaller, since it doesn't require spinning up an instance.

JaroslavTulach · 2025-03-13T16:08:00Z

(Not) Packaging in Native `enso` Executable

the Standard.Generic_JDBC library is inherently not able to work in native image mode
the Standard.Generic_JDBC comes with an extensive documentation describing how to add JDBC driver JAR into polyglot/java directory (@GregoryTravis can you provide a link to such documentation?)
inability to run in JVM mode must properly be reported to the user
as such adding/typing/having import Standard.Generic_JDBC statement in code must warn user in native image mode
such a warning is provided by Emit a warning on non-AOT ready libraries #12468 functionality
following example is based on previous example:

enso$ ENSO_LAUNCHER=native,-ls,fast sbt --java-home ~/bin/graalvm/ buildEngineDistribution
enso$ ./built-distribution/enso-engine-*/enso-*bin/enso --run e.enso --log-level info
[INFO] [DefaultPackageRepository] Found library Standard.Generic_JDBC @ 0.0.0-dev is not AOT ready at [***/0.0.0-dev].
42

where e.enso file is:

import Standard.Generic_JDBC

main = 42

@GregoryTravis, can you confirm this behavior with your branch? Is this behavior sufficient or do you demand changes to the reported warning?

GregoryTravis · 2025-03-13T18:07:22Z

can you please enumerate the use-cases, describe how to simulate them and what shall be the outcome?

CLI / Native Image

User imports Generic_JDBC:

Desired behavior
- an error telling the user to run in JVM mode, with a link to instructions
Current behavior
- "not AOT ready" log message, and cli execution fails with "No suitable driver found"

IDE / Native Image

User imports Generic_JDBC in the code editor, or user creates Generic_JDBC_Connection node or uses Database.connect with the JDBC option:

Desired behavior
- an error telling the user to run in JVM mode, with a link to instructions
Current behavior
- "not AOT ready" log message, and IDE error "No suitable driver found"

CLI and IDE / JVM Mode

Desired and current behavior
- Generic_JDBC is available and works as documented

To simulate these in CLI, any Enso program importing from Generic_JDBC will work.
In the IDE, an import, or using a Generic_JDBC_Connection node or a Database.connect node will work.

In my opinion Generic_JDBC needs to always be visible to the user in both modes, so that it is discoverable; but when the attempt to use it native mode, it gives an error and instructions for running in JVM mode.

GregoryTravis · 2025-03-13T18:14:50Z

@GregoryTravis, can you confirm this behavior with your branch? Is this behavior sufficient or do you demand changes to the reported warning?

This is sufficient for the current release, but later we will need this error to be shown in the IDE.

hubertp · 2025-03-14T09:38:26Z

@GregoryTravis, can you confirm this behavior with your branch? Is this behavior sufficient or do you demand changes to the reported warning?

This is sufficient for the current release, but later we will need this error to be shown in the IDE.

Since it is not a warning attached to a value but rather an internal warn log message it won't be shown in IDE. We need to find a way to attach it to the value. I would also agree that this can go in as-is for now, and we can do incremental improvements.

GregoryTravis added 6 commits March 10, 2025 16:09

move code and update references

f97ed93

builds

f052f86

wip

e33bde2

Merge branch 'develop' into wip/gmt/12337-separate-library

fd3da79

green

Loading
Loading status checks…

d6c0020

package.yaml

Loading
Loading status checks…

4cb7544

hubertp added the CI: Clean build required label Mar 12, 2025

GregoryTravis added 2 commits March 12, 2025 12:36

Merge branch 'develop' into wip/gmt/12337-separate-library

9e00ef1

main

Loading
Loading status checks…

88910e7

GregoryTravis closed this Mar 12, 2025

GregoryTravis reopened this Mar 12, 2025

JaroslavTulach self-requested a review March 13, 2025 15:49

JaroslavTulach approved these changes Mar 13, 2025

View reviewed changes

Merge branch 'develop' into wip/gmt/12337-separate-library

e0f4ae6

fmt

Loading
Loading status checks…

50492b1

GregoryTravis marked this pull request as ready for review March 14, 2025 18:20

GregoryTravis requested review from jdunkerley, radeusgd, AdRiley, marthasharkey, 4e6, hubertp, Akirathan and Frizi as code owners March 14, 2025 18:20

GregoryTravis added the CI: No changelog needed label Mar 14, 2025

hubertp approved these changes Mar 14, 2025

View reviewed changes

AdRiley approved these changes Mar 15, 2025

View reviewed changes

Merge branch 'develop' into wip/gmt/12337-separate-library

Loading
Loading status checks…

8003f81

GregoryTravis added the CI: Ready to merge label Mar 17, 2025

GregoryTravis added 4 commits March 20, 2025 12:19

Merge branch 'develop' into wip/gmt/12337-separate-library

Loading
Loading status checks…

84ae7f6

Merge branch 'develop' into wip/gmt/12337-separate-library

Loading
Loading status checks…

e5960c2

Merge branch 'develop' into wip/gmt/12337-separate-library

Loading
Loading status checks…

90dd894

Merge branch 'develop' into wip/gmt/12337-separate-library

Loading
Loading status checks…

ab76078

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Generic JDBC driver support to its own non-NI library #12472

Move Generic JDBC driver support to its own non-NI library #12472

GregoryTravis commented Mar 11, 2025 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Mar 13, 2025

GregoryTravis Mar 13, 2025

JaroslavTulach Mar 15, 2025

JaroslavTulach Mar 13, 2025

GregoryTravis Mar 13, 2025

JaroslavTulach Mar 13, 2025

GregoryTravis Mar 13, 2025

JaroslavTulach Mar 15, 2025

AdRiley Mar 15, 2025

GregoryTravis Mar 18, 2025

JaroslavTulach commented Mar 13, 2025 •

edited

Loading

GregoryTravis commented Mar 13, 2025

GregoryTravis commented Mar 13, 2025

hubertp commented Mar 14, 2025

Move Generic JDBC driver support to its own non-NI library #12472

Are you sure you want to change the base?

Move Generic JDBC driver support to its own non-NI library #12472

Conversation

GregoryTravis commented Mar 11, 2025 • edited Loading

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Mar 13, 2025 • edited Loading

(Not) Packaging in Native enso Executable

GregoryTravis commented Mar 13, 2025

CLI / Native Image

IDE / Native Image

CLI and IDE / JVM Mode

GregoryTravis commented Mar 13, 2025

hubertp commented Mar 14, 2025

GregoryTravis commented Mar 11, 2025 •

edited

Loading

JaroslavTulach commented Mar 13, 2025 •

edited

Loading

(Not) Packaging in Native `enso` Executable