feat: add support for add to async python API #1037
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
westonpace
changed the title
westonpace
force-pushed
the
feat/table-add
branch
from
044c77c
to
bf1107a
Compare
…t_rows. Start adding support for query
westonpace
force-pushed
the
feat/table-add
branch
from
bf1107a
to
451b931
Compare
|
For simplicity I've disabled |
wjones127
approved these changes
Mar 4, 2024
nodejs/__test__/connection.test.ts
Outdated
There was a problem hiding this comment.
These tests look very nice!
nodejs/src/connection.rs
Outdated
| mode: String, | ||
| ) -> napi::Result<Table> { | ||
| let schema = ipc_file_to_schema(schema_buf.to_vec()) | ||
| .map_err(|e| napi::Error::from_reason(format!("Failed to read IPC file: {}", e)))?; |
There was a problem hiding this comment.
I think the user would be confused by the idea of an "IPC file" involved here. This is really about the schema, right?
Suggested change
| .map_err(|e| napi::Error::from_reason(format!("Failed to read IPC file: {}", e)))?; | |
| .map_err(|e| napi::Error::from_reason(format!("Failed to parse schema: {}", e)))?; |
There was a problem hiding this comment.
Yes, we use IPC to marshal schemas to and from Rust (we should ideally come up with something based on the C API, I'll make a ticket) Good idea. I went with something slightly different Failed to marshal schema from JS to Rust just because the most likely cause is an internal bug of some kind something and not an invalid schema.
Co-authored-by: Will Jones <willjones127@gmail.com>
raghavdixit99
pushed a commit
to raghavdixit99/lancedb
that referenced
this pull request
Apr 5, 2024
In order to add support for `add` we needed to migrate the rust `Table` trait to a `Table` struct and `TableInternal` trait (similar to the way the connection is designed). While doing this we also cleaned up some inconsistencies between the SDKs: * Python and Node are garbage collected languages and it can be difficult to trigger something to be freed. The convention for these languages is to have some kind of close method. I added a close method to both the table and connection which will drop the underlying rust object. * We made significant improvements to table creation in lancedb@d33026d for the `node` SDK. I copied these changes to the `nodejs` SDK. * The nodejs tables were using fs to create tmp directories and these were not getting cleaned up. This is mostly harmless but annoying and so I changed it up a bit to ensure we cleanup tmp directories. * ~~countRows in the node SDK was returning `bigint`. I changed it to return `number`~~ (this actually happened in a previous PR) * Tables and connections now implement `std::fmt::Display` which is hooked into python's `__repr__`. Node has no concept of a regular "to string" function and so I added a `display` method. * Python method signatures are changing so that optional parameters are always `Optional[foo] = None` instead of something like `foo = False`. This is because we want those defaults to be in rust whenever possible (though we still need to mention the default in documentation). * I changed the python `AsyncConnection/AsyncTable` classes from abstract classes with a single implementation to just classes because we no longer have the remote implementation in python. Note: this does NOT add the `add` function to the remote table. This PR was already large enough, and the remote implementation is unique enough, that I am going to do all the remote stuff at a later date (we should have the structure in place and correct so there shouldn't be any refactor concerns) --------- Co-authored-by: Will Jones <willjones127@gmail.com>
raghavdixit99
pushed a commit
to raghavdixit99/lancedb
that referenced
this pull request
Apr 5, 2024
In order to add support for `add` we needed to migrate the rust `Table` trait to a `Table` struct and `TableInternal` trait (similar to the way the connection is designed). While doing this we also cleaned up some inconsistencies between the SDKs: * Python and Node are garbage collected languages and it can be difficult to trigger something to be freed. The convention for these languages is to have some kind of close method. I added a close method to both the table and connection which will drop the underlying rust object. * We made significant improvements to table creation in lancedb@d33026d for the `node` SDK. I copied these changes to the `nodejs` SDK. * The nodejs tables were using fs to create tmp directories and these were not getting cleaned up. This is mostly harmless but annoying and so I changed it up a bit to ensure we cleanup tmp directories. * ~~countRows in the node SDK was returning `bigint`. I changed it to return `number`~~ (this actually happened in a previous PR) * Tables and connections now implement `std::fmt::Display` which is hooked into python's `__repr__`. Node has no concept of a regular "to string" function and so I added a `display` method. * Python method signatures are changing so that optional parameters are always `Optional[foo] = None` instead of something like `foo = False`. This is because we want those defaults to be in rust whenever possible (though we still need to mention the default in documentation). * I changed the python `AsyncConnection/AsyncTable` classes from abstract classes with a single implementation to just classes because we no longer have the remote implementation in python. Note: this does NOT add the `add` function to the remote table. This PR was already large enough, and the remote implementation is unique enough, that I am going to do all the remote stuff at a later date (we should have the structure in place and correct so there shouldn't be any refactor concerns) --------- Co-authored-by: Will Jones <willjones127@gmail.com>
westonpace
added a commit
that referenced
this pull request
Apr 5, 2024
In order to add support for `add` we needed to migrate the rust `Table` trait to a `Table` struct and `TableInternal` trait (similar to the way the connection is designed). While doing this we also cleaned up some inconsistencies between the SDKs: * Python and Node are garbage collected languages and it can be difficult to trigger something to be freed. The convention for these languages is to have some kind of close method. I added a close method to both the table and connection which will drop the underlying rust object. * We made significant improvements to table creation in cc5f213 for the `node` SDK. I copied these changes to the `nodejs` SDK. * The nodejs tables were using fs to create tmp directories and these were not getting cleaned up. This is mostly harmless but annoying and so I changed it up a bit to ensure we cleanup tmp directories. * ~~countRows in the node SDK was returning `bigint`. I changed it to return `number`~~ (this actually happened in a previous PR) * Tables and connections now implement `std::fmt::Display` which is hooked into python's `__repr__`. Node has no concept of a regular "to string" function and so I added a `display` method. * Python method signatures are changing so that optional parameters are always `Optional[foo] = None` instead of something like `foo = False`. This is because we want those defaults to be in rust whenever possible (though we still need to mention the default in documentation). * I changed the python `AsyncConnection/AsyncTable` classes from abstract classes with a single implementation to just classes because we no longer have the remote implementation in python. Note: this does NOT add the `add` function to the remote table. This PR was already large enough, and the remote implementation is unique enough, that I am going to do all the remote stuff at a later date (we should have the structure in place and correct so there shouldn't be any refactor concerns) --------- Co-authored-by: Will Jones <willjones127@gmail.com>
alexkohler
pushed a commit
to alexkohler/lancedb
that referenced
this pull request
Apr 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to add support for
addwe needed to migrate the rustTabletrait to aTablestruct andTableInternaltrait (similar to the way the connection is designed).While doing this we also cleaned up some inconsistencies between the SDKs:
nodeSDK. I copied these changes to thenodejsSDK.countRows in the node SDK was returning(this actually happened in a previous PR)bigint. I changed it to returnnumberstd::fmt::Displaywhich is hooked into python's__repr__. Node has no concept of a regular "to string" function and so I added adisplaymethod.Optional[foo] = Noneinstead of something likefoo = False. This is because we want those defaults to be in rust whenever possible (though we still need to mention the default in documentation).AsyncConnection/AsyncTableclasses from abstract classes with a single implementation to just classes because we no longer have the remote implementation in python.Note: this does NOT add the
addfunction to the remote table. This PR was already large enough, and the remote implementation is unique enough, that I am going to do all the remote stuff at a later date (we should have the structure in place and correct so there shouldn't be any refactor concerns)