-
Notifications
You must be signed in to change notification settings - Fork 224
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
feat: Initial remote table implementation for rust #1024
Conversation
rust/lancedb/src/remote/client.rs
Outdated
headers.insert( | ||
"Host", | ||
HeaderValue::from_str(&host).map_err(|_| Error::Http { | ||
message: format!("non-ascii table name '{}' provided", db_name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message: format!("non-ascii table name '{}' provided", db_name), | |
message: format!("non-ascii database name '{}' provided", db_name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@westonpace you marked this as resolved, but looks like it's still "table name". Is it supposed to be "table name"?
api_key: &str, | ||
region: &str, | ||
db_name: &str, | ||
has_host_override: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this supposed to be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user has overridden the host then we set the x-lancedb-database
header. I copied this from the existing python client. I believe the rationale is that we normally get the db header from the URI. However, when testing locally, we don't have DNS setup and we need to fallback to some alternate method.
It would probably be simpler, and I think it would be safe, if we just always set this header too. CC @albertlockett @chebbyChefNEQ for second opinion.
.client | ||
.get("/v1/table/") | ||
.query(&[("limit", 10)]) | ||
.query(&[("page_token", "")]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess pagination is TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. It wasn't part of the Table
abstract class, so it wasn't documented at all and there was no implementation for the local table. I figure I can add it back in, with support for both tables, at a later date. I'll make a issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just referring to the fact we only request the first page here (first 10).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
723d39f
to
9a5a1a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but have a question about the earlier comment I made.
b8a3b02
to
0691be2
Compare
This will eventually replace the remote table implementations in python and node.
This will eventually replace the remote table implementations in python and node.
This will eventually replace the remote table implementations in python and node.
This will eventually replace the remote table implementations in python and node.