-
Notifications
You must be signed in to change notification settings - Fork 245
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: add update to the async API #1093
Conversation
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.
Some minor nits
@@ -113,6 +113,7 @@ export class Table { | |||
countRows(filter?: string | undefined | null): Promise<number> | |||
delete(predicate: string): Promise<void> | |||
createIndex(index: Index | undefined | null, column: string, replace?: boolean | undefined | null): Promise<void> | |||
update(onlyIf: string | undefined | null, columns: Array<[string, string]>): Promise<void> |
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 still prefer where
, as it is most SQL-y
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 think it's where in table.ts
. In rust, where
is a reserved word, so you need to do r#where
. This is why I used only_if
in rust. In the nodejs bindings it appears that napi can't handle r#where
(it generated update(r#where: string | undefined | null, ...)
which typescript complained about). In python I was able to use where
throughout. Also, in typescript, where
is kind of a reserved word. You can create a function or class/interface property named where
but you can't create a variable named where
.
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.
Okay I see. That's fine as-is then.
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 we can stick with where
in rust too. The user shouldn't need to use r#where
to call a function named where
and so they won't be affected.
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'll give it a try
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 like that didn't work. You need to escape where
even if you are using it as a method name. E.g. table.update.r#where("i < 5")
. That would be pretty annoying so I'm leaving as only_if
.
python/python/lancedb/table.py
Outdated
@@ -2252,20 +2252,28 @@ async def update( | |||
0 1 [1.0, 2.0] | |||
1 2 [3.0, 4.0] | |||
2 3 [5.0, 6.0] | |||
>>> table.update(where="x = 2", values={"vector": [10, 10]}) | |||
>>> table.update({"vector": [10, 10]}, where="x = 2") |
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 like this is failing in doctest because table here is still the sync version with the old signature.
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.
Ah, thanks. Fixed. Sadly, it doesn't seem there is any great way to use doctest with asyncio. Best I could find is...
>>> import asyncio
>>> async def some_fn():
... await do_async_stuff()
>>> asyncio.run(some_fun())
* wip: start docs * wip: start outlining compaction impl * fix the chunker * fix various tests * adjust tests * pr feedback
No description provided.