feat(persistence): use sqlean v3.45.1 as sqlite engine #2947
Conversation
dosubot
bot
added
the
size:L
| @@ -147,6 +146,9 @@ def _insert_project_abc(session: Session) -> None: | |||
| start_time=datetime.fromisoformat("2021-01-01T00:00:00.000+00:00"), | |||
| end_time=datetime.fromisoformat("2021-01-01T00:00:05.000+00:00"), | |||
| attributes={ | |||
| "input": { | |||
| "value": "XY%*Z", | |||
There was a problem hiding this comment.
What is the %* symbol for?
There was a problem hiding this comment.
Because % and * are special symbols in alternative implementations such as LIKE and GLOB, the test will fail (I tried it) if it's not implemented correctly.
| url = get_db_url(driver="sqlite+aiosqlite", database=database) | ||
| engine = create_async_engine(url=url, echo=echo, json_serializer=_dumps) | ||
| async_url = get_async_db_url(url.render_as_string()) | ||
| assert async_url.database |
There was a problem hiding this comment.
Is this an assertion for not None?
There was a problem hiding this comment.
It's gives the same result, because It's asserting on the truthiness of the expression.
There was a problem hiding this comment.
This is mainly needed for the type-checker to rule out the None type for the code below.
There was a problem hiding this comment.
This check is already carried out by lines 39 and 63.
| if url.database.startswith(":memory:"): | ||
| url = url.set(query={"cache": "shared"}) |
There was a problem hiding this comment.
Does this only affect :memory: mode, or do we want this setting anytime we are running SQLite?
There was a problem hiding this comment.
Yes, this is setting ourselves up for using two separate engines. Otherwise each engine will gets its own database, which is not shared.
There was a problem hiding this comment.
This is actually a legacy feature that's no longer applicable to file-base databases.
src/phoenix/db/models.py
Outdated
| @compiles(JSONB, "sqlite") # type: ignore | ||
| def _(*args: Any, **kwargs: Any) -> str: | ||
| return "JSONB" |
There was a problem hiding this comment.
Docstring would help here.
src/phoenix/db/models.py
Outdated
| @compiles(TextContains) # type: ignore | ||
| def _(element: Any, compiler: Any, **kw: Any) -> Any: | ||
| string, substring = list(element.clauses) | ||
| return compiler.process(string.contains(substring), **kw) | ||
|
|
||
|
|
||
| @compiles(TextContains, "postgresql") # type: ignore | ||
| def _(element: Any, compiler: Any, **kw: Any) -> Any: | ||
| string, substring = list(element.clauses) | ||
| return compiler.process(func.strpos(string, substring) > 0, **kw) | ||
|
|
||
|
|
||
| @compiles(TextContains, "sqlite") # type: ignore | ||
| def _(element: Any, compiler: Any, **kw: Any) -> Any: | ||
| string, substring = list(element.clauses) | ||
| return compiler.process(func.text_contains(string, substring) > 0, **kw) |
There was a problem hiding this comment.
I'll just be linking it to the docs. I'm not sure how to explain this without confusing ppl.
| class JSONB(JSON): | ||
| __visit_name__ = "JSONB" | ||
|
|
||
|
|
||
| @compiles(JSONB, "sqlite") # type: ignore | ||
| def _(*args: Any, **kwargs: Any) -> str: | ||
| return "JSONB" | ||
|
|
||
|
|
||
| JSON_ = ( | ||
| JSON() | ||
| .with_variant( | ||
| postgresql.JSONB(), # type: ignore | ||
| "postgresql", | ||
| ) | ||
| .with_variant( | ||
| JSONB(), | ||
| "sqlite", | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Not sure if it matters, but we've got some duplication here.
There was a problem hiding this comment.
yea, I wasn't sure if we want to create a dependency here, given that the migration may be run in the command line
This gives:
JSONBtext_contains(case sensitive substring search)