feat: refactor prql-java and add JDBC driver

[zhicwu]

Closes #1643

Hi @max-sixty / @snth, could you help on the following items?

• error handling in prql-java/prql-api/src/main/rust/src/lib.rs - now a simple query fr0m table will crash the JVM
• build multi-platform native binaries in .github/workflows/build-java.yaml so that we can pack into JDBC driver and publish for others to use - for testing or demonstration, you may use prqlc command line instead
• sonatype account, registered maven group id org.prqllang, and gpg key for publishing signed artifacts to maven central
• misc
  • beautiful PRQL logos, which will be later added into DBeaver - examples
  • platform to support - I only added mac, windows, and linux in the build file, but it's up to you on platforms should be supported
  • official docker image containing prqlc, I assume it's prql/prql in current implementation but it might be different

Key changes:

• prql-api module for JNI(call rust lib) and CLI(wrapper of prqlc) integration
• prql-jdbc module is the JDBC wrapper
• rust lib is moved to prql-java/prql-api/src/main/rust

Build instructions:

cd prql-java
mvn clean package
# if you don't have Java and maven installed, try below using docker(should be similar using podman)
# docker run --rm -it  -v `pwd`:/data -w /data maven:3-openjdk-17-slim mvn clean package
ls -alF prql-jdbc/target/prql-jdbc-0.5.0-SNAPSHOT.jar

To use in DBeaver:

1. Add JDBC driver in Database -> Driver Manager -> New
   
   
   Note: driver class is org.prqllang.jdbc.PrqlDriver.

2. Create new connection for testing
   
   

• URL: jdbc:prql:ch:https://explorer@play.clickhouse.com:443/default (no user name and password)
• Properties:
  • set prql.api to cli (as native lib is unstable)
  • prql.custom.classpath -> { full path to ClickHouse JDBC driver, URL like https://repo1.maven.org/maven2/com/clickhouse/clickhouse-jdbc/0.4.1/clickhouse-jdbc-0.4.1-http.jar works too }, only needed when the JDBC driver is not in class path
  • prqlc.path -> { full path to prqlc }, only needed when it cannot be found in PATH

3. Issue query
   I hope you're not surprised that both PRQL and SQL will work. This is because 1) sometimes the underlying JDBC driver may issue SQL query so that wrapper has to support; 2) PRQL does not support DDL and mutation, so it's more convenient to support both instead of openning two windows one for PRQL and the other for SQL.
   As to the error message shown below, it's not friendly for PRQL user, so I'll add an option later to override that(show PRQL compilation error instead).
   

[max-sixty]

This looks great @zhicwu !

It's difficult for me to review the Java as I don't know it well.

Could we ensure we've included links to files that we've taken from elsewhere? Same for noting the files that are autogenerated.

Are the tests running in CI?

[max-sixty]

Is this the only DB that's currently supported, or it's a draft list?

[zhicwu]

Sorry I was too lazy as I only tried on ClickHouse and SQLite. It should work for all JDBC-compatible databases. It's going to be a long list but I'll document a few more after testing.

[max-sixty]

No prob! Fine to leave it as a TODO! As long as we're transparent about the current state it's fine to make incremental progress...

[max-sixty]

Also just saw the full PR description, I'll respond to that now

[max-sixty]

Re your question on handling errors — my guess is that here we need to instruct it to throw the error in Java with something like (from your link): jni-rs/jni-rs#154 (comment)

[zhicwu]

Yes, I'm asking for your help. I brought a book and just started to read tutorials created for Java developers, but it's going to take a while for me enhance the code. Appreciate if you can take care of changes on rust side, thanks in advance!

[max-sixty]

OK, I can likely have a look soon. But I'll need to get a Java environment set up, and I do most of my coding on the weekend.

In the meantime, I think something like this should at least raise a simple error:

Suggested change

	prql_compiler::compile(&prql_query, &option).expect("Couldn't compile query to SQL!");
	prql_compiler::compile(&prql_query, &option).unwrap_or(env.throw_new("java/lang/RuntimeException", "Query error (TODO a better description)"));

[zhicwu]

Thanks but it generates compile error.

[max-sixty]

@zhicwu this is one approach — I'm not sure it's great, but it works OK:

diff --git a/prql-java/prql-api/src/main/rust/src/lib.rs b/prql-java/prql-api/src/main/rust/src/lib.rs
index c711961..58c67d9 100644
--- a/prql-java/prql-api/src/main/rust/src/lib.rs
+++ b/prql-java/prql-api/src/main/rust/src/lib.rs
@@ -48,11 +48,23 @@ pub extern "system" fn Java_org_prqllang_internal_JniImpl_compile0<'local>(
         signature_comment,
     };
 
-    let rs_sql_str: String =
-        prql_compiler::compile(&prql_query, &option).expect("Couldn't compile query to SQL!");
-    env.new_string(rs_sql_str)
-        .expect("Couldn't create java string!")
-        .into_raw()
+    match prql_compiler::compile(&prql_query, &option) {
+        Ok(sql) => env
+            .new_string(sql)
+            .expect("Couldn't create java string!")
+            .into_raw(),
+        Err(e) => {
+            env.throw_new(
+                "java/lang/RuntimeException",
+                format!("Query error: {}", e).as_str(),
+            )
+            .unwrap();
+
+            // TODO: can we return something more meaningful than an empty
+            // string? Are there better docs than https://github.com/jni-rs/jni-rs/issues/154?
+            env.new_string("").unwrap().into_raw()
+        }
+    }
 }
 
 #[no_mangle]

[max-sixty]

Hi @max-sixty / @snth, could you help on the following items?

* [ ]  error handling in `prql-java/prql-api/src/main/rust/src/lib.rs` - now a simple query `fr0m table` will crash the JVM

I added a comment above, I think we can call something on the rust side to throw an error in Java rather than crashing the JVM. I can look more later if needed

* [ ]  build multi-platform native binaries in `.github/workflows/build-java.yaml` so that we can pack into JDBC driver and publish for others to use - for testing or demonstration, you may use prqlc command line instead

Yes, are there other projects that do this? Often this is the sort of thing we take from other libraries.

It looks like it's working on Ubuntu, is that correct? But not yet on Windows.

* [ ]  sonatype account, registered maven group id `org.prqllang`, and gpg key for publishing signed artifacts to maven central

Yes, I can do this later, and add them to the repo

[zhicwu]

I think we can call something on the rust side to throw an error in Java rather than crashing the JVM. I can look more later if needed.

Exactly. The was link is probably outdated. You may check more in https://github.com/exonum/exonum-java-binding. The idea is to throw a Java exception(java.lang.IllegalArgumentException if the PRQL is invalid, or java.lang.IllegalStateException if somehow failed to compile a valid query) with meaningful error message.

Yes, are there other projects that do this? Often this is the sort of thing we take from other libraries.

zstd-jni was the one I mentioned earlier but I didn't find a rust project, until I realized Deno is written in rust :)

It looks like it's working on Ubuntu, is that correct? But not yet on Windows.

Just tried to rebuild failed jobs at my end. Windows is the only one failing according to https://github.com/zhicwu/prql/actions/runs/4243658539, which is probably due to a naming issue - it's libprql_java*.* on mac and linux, but maybe prql_java.dll on windows.

Yes, I can do this later, and add them to the repo

Thank you. Please use the same repository secrets as used in build_java.yaml, if there's no concern.

[zhicwu]

Hmm... the build will never success as pre-commit kept replacing testng to testing - it's not a type but a popular testing framework :)

[snth]

Thank you @zhicwu . This looks great!

I was going to say that I don't have a java environment setup but I see you already thought of that with your docker command:
docker run --rm -it -v $(pwd):/data -w /data maven:3-openjdk-17-slim mvn clean package

I will give that a try.

[max-sixty]

Hmm... the build will never success as pre-commit kept replacing testng to testing - it's not a type but a popular testing framework :)

FYI I fixed this!

[zhicwu]

I will give that a try.

In addition to build by yourself, you may download JDBC driver(bundled with JNI libs for macos, windows and linux) at https://github.com/zhicwu/prql/actions/runs/4257825932 - scroll down to bottom and download prql-jdbc in Artifacts list, unzip and prql-jdbc-0.5.0-SNAPSHOT.jar is the driver.

[max-sixty]

Unneeded

Suggested change

	- uses: actions-rs/toolchain@v1
	with:
	profile: minimal
	toolchain: stable
	target: ${{ matrix.target }}
	override: true

[zhicwu]

Why? Is matrix.target unneeded too?

[max-sixty]

We need to put the target in the cargo command, just like you do above: https://github.com/PRQL/prql/pull/1929/files#diff-787cc4dbb3b51e6d39655bfc50bee79b4b6c34a7d629402d34342ebd0e878549R60

Unfortunately actions-rs/toolchain@v1 isn't maintained, and doesn't bring much benefit above the standard runner now. So we just use the standard runner and pass the target.

[max-sixty]

@zhicwu this is looking really good. Thanks for adding tests.

Is there somewhere that describes where the Java code comes from? Is lots of it autogenerated? Could we explain that in the readme?

[max-sixty]

I will give that a try.

If you can @snth, this would be great. Otherwise I can this weekend...

[zhicwu]

Is there somewhere that describes where the Java code comes from? Is lots of it autogenerated? Could we explain that in the readme?

Did you mean prql-jdbc? VSCode helped to generate empty methods but I had to fill in all the blanks :) Do you want me to add author and copyright to each of the files?

Understood it's time-consuming to review a PR with a few thousands line of code. Do you think it's better to meet on Slack so that I can walk you through? And you may want to review the features implemented so far as well.

[max-sixty]

Did you mean prql-jdbc? VSCode helped to generate empty methods but I had to fill in all the blanks :) Do you want me to add author and copyright to each of the files?

For example, this file: prql-java/prql-api/src/main/java/org/prqllang/internal/Utils.java. Thanks for adding one link there for a function that was taken from Netty. What about the remainder of the file? We don't have to hit 100% on every piece of code, but if there are large amounts of code that we've copied then it's helpful for future contributors to know where it came from.

VSCode helped to generate empty methods but I had to fill in all the blanks

How about prql-java/prql-jdbc/src/main/java/org/prqllang/jdbc/PrqlCallableStatement.java? Or the 1000 line file prql-java/prql-jdbc/src/main/java/org/prqllang/jdbc/PrqlResultSet.java. It looks auto-generated — but maybe it was lots of typing? 😄

If it was auto-generated, let's make a note, so someone knows not to edit it, and how to generate it again.

We can put the details at the top of a file, or in the Readme.

---

I recognize this is 5K lines of Java, and we have 20K lines of Rust, so it'll be a big part of the repo, and so it's helpful to know where stuff comes from.

Is that reasonable? I'm really not trying to create more work, I very much appreciate the contribution.

---

Do you want me to add author and copyright to each of the files?

I'm not sure of the exact rules here. If the original file has a notice, we shouldn't remove it. But I don't think we need to write new copyright notices on everything we copy.

I'm asking because it helps the maintenance of this repo.

---

Re Slack — what timezone are you on? I'm on PT. We can set something up if we need to discuss live. I'm not really on IM platforms most of the day though.

---

Thank you @zhicwu !

[zhicwu]

What about the remainder of the file?

That'd be me :)

It looks auto-generated — but maybe it was lots of typing?

Yes, methods were initially generated by VSCode, but I have to implement each of the methods - that's why I said it's not complex but do have some workload ;)

Is that reasonable? I'm really not trying to create more work, I very much appreciate the contribution.

Of course and I totally understand. Does it make sense to create a separate repo for prql-java or just prql-jdbc?

I'm not sure of the exact rules here. If the original file has a notice, we shouldn't remove it. But I don't think we need to write new copyright notices on everything we copy.

As far as I know, many opensource projects under Apache 2.0 license have copyright header in each source file, but it's really up to you. I was just trying to align with what we have here.

Re Slack — what timezone are you on? I'm on PT. We can set something up if we need to discuss live. I'm not really on IM platforms most of the day though.

I'm on China Standard Time(UTC +8), so probably we can meet at early morning my time before I head to work or weekend.

[max-sixty]

Yes, methods were initially generated by VSCode, but I have to implement each of the methods - that's why I said it's not complex but do have some workload ;)

Oh right — so lots of typing from you!

Could we just add at the top of the file that that was how it was created? This is the sort of explanation I was hoping for.

Same for files like https://github.com/PRQL/prql/pull/1929/files#diff-3ceb4a7cb5c600632a0681b0ef28bea171f39e43c8e8a4db7c16b840d2a37f76R1. So if someone else came along and wanted to make a small change, they would have some context on the origin and purpose of the code.

---

No need to make a separate repo (unless you have a strong preference to, in which case that would be OK)

And I think we're all fine on the licenses. If there's any code that's copied from elsewhere, we should link to it, but otherwise great.

---

I'm on China Standard Time(UTC +8), so probably we can meet at early morning my time before I head to work or weekend.

How about this coming weekend? I'm 16 hours behind, so anytime your morning?

[linux-china]

A question here, how to handle parameters in PreparedStatement? Now ? is not legal parameter placeholder in PRQL.

PreparedStatement stmt = conn.prepareStatement("from employee | filter name == ?");
//  SELECT * FROM employee WHERE name = ?
stmt.setString(1, "John");

I wrote an issue here: #1987

[zhicwu]

Since C ABI was implemented in prql-lib, it's probably better to use JNA instead(10 times slower than direct JNI in general), with no additional rust code.

As to JDBC wrapper, I'll create a separate repo for that as it can be extended to support more query languages in addition to SQL and PRQL, for instance (schemaless) GraphQL.

[max-sixty]

As to JDBC wrapper, I'll create a separate repo for that as it can be extended to support more query languages in addition to SQL and PRQL, for instance (schemaless) GraphQL.

Ok great, this could be a nice project.

I thought it was a great idea to have this for PRQL; the PRQL project could still help spread the word about it if you create a different repo.

---

What are good next steps for the prql-api module? Would it be helpful to chat at the weekend?

@snth let us know if you got a chance to try this out; otherwise I'll spend some time testing it.

[zhicwu]

I thought it was a great idea to have this for PRQL; the PRQL project could still help spread the word about it if you create a different repo.

That's very kind of you! And sorry for constantly changing my opinion. It's all started when I was trying to add PRQL into ClickHouse JDBC driver as preferred language(instead of SQL) for manipulating data from multiple external datasources, and then I realized it's probably better to make it a JDBC wrapper to bring PRQL to all JDBC drivers, not just ClickHouse. Now I think it can be further abstracted to support more query languages in the same way. I'm going to add GraphQL in the next following days for PoC and release the first version in case others may interest.

What are good next steps for the prql-api module?

I tried #1971 implemented by @linux-china and it works well, but maybe we don't need additional rust code for consistency, that's why I was thinking JNA even it's a bit slow. Or we probably need both and let user to choose. Let me add JNA implementation later today for comparison.

Would it be helpful to chat at the weekend?

Yes, let's meet in the coming weekend. Ping me on Slack when you're available.

[max-sixty]

Yes, let's meet in the coming weekend. Ping me on Slack when you're available.

OK, is there a good Slack workspace or should I create a new one?

(For anyone reading this — I don't think we have enough folks to be generally split across Discord & Slack, this is just for a single conversation)

[zhicwu]

OK, is there a good Slack workspace or should I create a new one?

This is the invite of my personal workspace, but you may create yours or an official workspace for PRQL :)

[max-sixty]

OK great, I joined and will be around this weekend. If I'm not on, feel free to ping me on GH, or Discord if you get access.

Something that would be good to think about — what could we merge from this work to make incremental progress. There's a lot of code here, lots looks very good. There are lots of tests, and it publishes to Maven, and the API looks quite complete. The code is high quality — I just made a change in #1971 and then I see the code is already here at https://github.com/PRQL/prql/pull/1929/files#diff-d661b1ff3ea2758cbe9719c1ce84e1f2a4280b48ee645533466fb8e3f5dd3a2bR34.

So it would be great to merge some of this, even if the JDBC driver becomes separate. Could we start merging the parts that are easier / pass tests?

[linux-china]

@zhicwu I think we should use more parameters instead of Option class for JNI method. Option object requires more invocations between Java and JNI, and not good for performance. If you want to make API clean, and you can wrap up a new PRQL class to call JNI.

#[no_mangle]
#[allow(non_snake_case)]
pub extern "system" fn Java_org_prqllang_internal_JniImpl_compile0<'local>(
    mut env: JNIEnv<'local>,
    _class: JClass<'local>,
    query: JString<'local>,
    option: JObject<'local>,
) -> jstring {
}

Parameters instead of Option object.

#[no_mangle]
#[allow(non_snake_case)]
pub extern "system" fn Java_org_prql_prql4j_PrqlCompiler_toSql(
    env: JNIEnv,
    _class: JClass,
    query: JString,
    target: JString,
    format: jboolean,
    signature: jboolean,
) -> jstring {

[max-sixty]

I looked into getting a username & password. But:

• I see that the action has been archived. Are there any alternatives?
• Are you familiar with how to get a Sonatype login? I can look more if not, but I couldn't immediately find how to get one.

[zhicwu]

Option object requires more invocations between Java and JNI, and not good for performance. If you want to make API clean, and you can wrap up a new PRQL class to call JNI.

Thanks @linux-china for pointing that out! It was a copy-n-paste without thinking :p Yes, there's a PrqlCompileOptions for the same purpose.

[linux-china]

@zhicwu could you share your prql-jdbc? It's good implementation for PRQL over JDBC. I can not find the code on your fork. thanks.

[zhicwu]

@zhicwu could you share your prql-jdbc? It's good implementation for PRQL over JDBC. I can not find the code on your fork. thanks.

Sorry I discarded all the changes trying to come up with a new PR for pushing prql-java to maven central. I'll create a personal repo jdbc-wrapper for the JDBC implementation. Will update here once it's released.

[linux-china]

@zhicwu I got your code from commit, and it works well.

[zhicwu]

@zhicwu I got your code from commit, and it works well.

Thanks @linux-china. It has glitches handling jni and cli, and the driver does not support dialect mapping. I fixed some of the issues and tried schemaless graphql for testing. Will add a few more changes(e.g. templating and scripting etc.) before publishing to maven central.