Use MetadataFilter in LanceDB

[blakerosenthal]

Add metadata filter to lancedb storage

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[pmeier]

@blakerosenthal Tests and lint / format CI is failing. I'll review the test when everything is green otherwise.

[blakerosenthal]

@pmeier Thanks for all the comments. Ran into a couple quirks with LanceDB's add_columns method, but otherwise all your suggestions worked out. Do you know what's happening with mypy in this commit? It's complaining about missing types in LanceDB but I thought that the overrides in pyproject.toml would take care of that.

[pmeier]

Do you know what's happening with mypy

Yes. If you have code like this

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from ragna import Rag

def foo() -> Rag:
    from ragna import Rag

    return Rag()

it looks like this should work. Running mypy on this succeeds. But running this script fails with

Traceback (most recent call last):
  File "/home/philip/git/ragna/main.py", line 7, in <module>
    def foo() -> Rag:
NameError: name 'Rag' is not defined

The reason is that Python evaluates the annotations at definition. For example

from ragna import Rag

def foo() -> Rag:
    return Rag()

print(foo.__annotations__)

{'return': <class 'ragna.core.Rag'>}

And since at definition in the top script, the Rag object is not available, you face the error.

That being said, there is an easy solution for it: turn the annotation into a string.

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from ragna import Rag

def foo() -> "Rag":
    from ragna import Rag

    return Rag()

print(foo.__annotations__)

{'return': 'Rag'}

Now mypy and Python are happy. Internally mypy resolves the string back to its actual type with typing.get_type_hints() and so does Ragna as well:

ragna/ragna/core/_components.py

Lines 73 to 75 in 2066dcd

	method = getattr(cls, method_name)
	params = iter(inspect.signature(method).parameters.values())
	annotations = get_type_hints(method)

Finally, because it is a little tedious to always stringify the annotations, there is a shortcut. If you from __future__ import annotations at the top of your module, the stringification is applied automatically in the background:

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from ragna import Rag

def foo() -> Rag:
    from ragna import Rag

    return Rag()

print(foo.__annotations__)

And this is what I did in 4a3b32e.

[pmeier]

@blakerosenthal I've added the following changes in be943a7:

• Remove the need to keep a table schema as instance variable. This was only needed because we unconditionally used db.create_table() and this only succeeded if the schema of the existing table matches the one passed exactly. Instead we now check whether the table already exists and if yes just open the table. Thus, table.schema is now the only source of truth.
• Although it is not possible to add a column of a given type, the string we pass to add_column can contain SQL syntax. Thus, instead of using "''" as value, we use "CAST(NULL {type})" where corresponds to metadata. Thus, we now have the right column type, which is important for later filtering, e.g. > 5 makes no sense on a string.
• Driveby: instead of calling table.add in a nested for loop, we know collect all new rows first and insert them in one call.
• Re Use MetadataFilter in LanceDB #461 (comment): my guess into why Chroma and LanceDB had different results is because you defaulted missing metadata in LanceDB to an empty string, while Chroma used None. And None or better NULL is handled differently in databases. For example, checking for a column not equal to some value, will not include the NULL values. To translate in Python, item != value in SQL means roughly item != value and item is not None. With the fix I detailed above, we can now properly NULL missing metadata and thus Chroma and LanceDB behave the same. I also fixed the tests to account for that.

I'm merging this for velocity, but please do take a look if anything is unclear or a follow-up PR is needed.