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

Expose db memory address and path to libduckdb_java.so #87

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Jens-H
Copy link
Contributor

@Jens-H Jens-H commented Sep 19, 2024

This PR adds two methods to the DuckDBConnection object to:

  1. Get the memory address of the DuckDB pointer from JNI
  2. Get the path and name of the loaded libduckdb_java.so

With these two pieces of information, it should be possible to create db connections with another driver like e.g. https://github.com/techascent/tmducken for Clojure in parallel. (By the way: this driver is not yet mentioned on the homepage as third-party driver.)

@Jens-H
Copy link
Contributor Author

Jens-H commented Sep 20, 2024

No idea why the format check failed - locally it runs without errors. Maybe the non-ASCII chars ...

@Mause Any idea? Or may we ignore it?

@Mause
Copy link
Member

Mause commented Sep 20, 2024

Looks like it might be an encoding thing yeah.

As far as the change goes, why return the memory address as a string?

And I'd like to see some kind of javadoc on the memory method at least mentioning the unsafety

@Jens-H
Copy link
Contributor Author

Jens-H commented Sep 20, 2024

Sadly a string seems to be the simplest and secure way without having pointers and unsigned 64 bit integers in Java/JNI return types.

You're right, some kind of warning seems appropriate. I'll add that.

@Mause
Copy link
Member

Mause commented Oct 1, 2024

Can you expand on why returning a pointer cast to an integer via jni is unsafe?

@Jens-H
Copy link
Contributor Author

Jens-H commented Oct 1, 2024

Well, basically I'm not sure if casting the memory address like this will work everywhere (Linux-AMD64 seems ok):
return (jlong)conn_ref->db.get();

Maybe you can confirm that this should be fine. Then I'm happy to change the PR accordingly.

@Jens-H
Copy link
Contributor Author

Jens-H commented Dec 29, 2024

I created a new PR #115 where the cast to string is replaced by a cast to long.

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.

2 participants