feat(java): add table names java api #1279
Conversation
|
@QianZhu @eddyxu @walterddr PTAL the table names |
|
DONE testing for this PR. |
| assertEquals("dataset_version", tableNames.get(0)); | ||
| assertEquals("new_empty_dataset", tableNames.get(1)); | ||
| assertEquals("test", tableNames.get(2)); | ||
| assertEquals("write_stream", tableNames.get(3)); |
There was a problem hiding this comment.
is the returned table names sorted in a certain way?
There was a problem hiding this comment.
Yeah, it's always in ascending order (alphabetical order), will add it to the method description
|
|
||
| /// TODO(lu) import from lance-jni without duplicate | ||
| /// Extend JNIEnv with helper functions. | ||
| pub trait JNIEnvExt { |
There was a problem hiding this comment.
Yeah, i added some examples of test cases but haven't got time to add more.
https://github.com/lancedb/lance/blob/main/java/core/src/test/java/com/lancedb/lance/JNITest.java
Because testing JNIEnv requires the launch of an actual JVM, so i plan to add the test on the Java side.
Ideally the tests will be passing some object from Java to Rust, Rust side consumes the Java object, convert to rust object, and convert back to java object to return to the Java side. If anything is wrong, the test will fail
| OtherLanceDB { message: String, location: Location }, | ||
| } | ||
|
|
||
| impl Error { |
There was a problem hiding this comment.
prob it is easier to map errors (with an ErrorExt trait). You can see an example here:
Line 33 in e688484
There was a problem hiding this comment.
Good idea, i will test this tonight and create another PR for the error related changes
| F: FnOnce(&mut JNIEnv, &JObject) -> Result<T>; | ||
| } | ||
|
|
||
| impl JNIEnvExt for JNIEnv<'_> { |
There was a problem hiding this comment.
All functions are returning Result but do we need it if the result is always Ok?
There was a problem hiding this comment.
Using "Result" for convenient handle the returned result of the inner called methods. JNI needs special error handling differently, require calling env.throw_new(JAVA exception) to throw java exception to end user.
| /** | ||
| * Represents LanceDB database. | ||
| */ | ||
| public class Database { |
There was a problem hiding this comment.
Lets make the naming conventions be consistent with the rest of the rust/python/nodejs packages.
YOu can take a look of https://docs.rs/lancedb/latest/lancedb/
There was a problem hiding this comment.
thanks i will take a look
I am trying to save the connection on JNI side so that the connection can be reused for create_table, open_table and other funcs
i am pretty sure it is OK to cache the connection on the Java side as well as long as the API is clear :-P
|
@QianZhu @eddyxu @walterddr PTAL, thanks! |
| import org.junit.jupiter.api.io.TempDir; | ||
|
|
||
| public class ConnectionTest { | ||
| @TempDir |
There was a problem hiding this comment.
do we need to close the connection?
There was a problem hiding this comment.
Yeah the connections are closed in try (Connection conn = Connection.connect(databaseUri)) {
| assertEquals("new_empty_dataset", tableNames.get(1)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
can we add a test where both startAfter and limit are specified?
|
ACTION NEEDED Lance follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
LuQQiu
changed the title
|
@QianZhu Addressed the comments, added test cases, PTAL |
| <artifactId>lancedb-core</artifactId> | ||
| <name>LanceDB Core</name> | ||
| <packaging>jar</packaging> |
There was a problem hiding this comment.
what's the relationship between this and the lancedb/lance/pull/2115?
AFAIK these cannot be identical. (maybe this should be lancedb-core and the other one should be lance-core?)
There was a problem hiding this comment.
@walterddr This is the LanceDB java module with LanceDB functionalities e.g. embeddings, table names
The other one is Lance java module with Lance Dataset/Fragment functionalities
Put it to the sub module to give space if we want to put other connectors e.g. Spark connectors directly in LanceDB java side
There was a problem hiding this comment.
ok i see. so lancedb-core and lance-core are both core modules and doesn't depend on each other (and thus are independent basically?) nice i will pull in both in the trino side.
There was a problem hiding this comment.
Yeah yeah!! From the Java Side you need to import both
The only problem later is the packaged jar may be bigger, but we can consider that issue later
Add lancedb-jni and table names API