refactor: sqlite async session for graphql api #2784
Conversation
dosubot
bot
added
the
size:XXL
| project_rowid := await session.scalar( | ||
| text("SELECT rowid FROM projects WHERE name = :name;"), | ||
| {"name": project_name}, | ||
| ) | ||
| ): | ||
| project_rowid = await session.scalar( | ||
| text("INSERT INTO projects(name) VALUES(:name) RETURNING rowid;"), | ||
| {"name": project_name}, | ||
| ) |
There was a problem hiding this comment.
this makes it so that you have to hit the database on every insert. I think this is where we can build a bit of optimization via dynamic programming where you can keep a list of seen projects. If you've seen it before you don't need to insert.
There was a problem hiding this comment.
yea, that makes sense. we may have to cache it outside the handler though
| async def _insert_span(session: AsyncSession, span: Span, project_name: str) -> None: | ||
| if not ( | ||
| project_rowid := await session.scalar( | ||
| text("SELECT rowid FROM projects WHERE name = :name;"), |
There was a problem hiding this comment.
Is this safe to sql injection?
There was a problem hiding this comment.
yes. colon is one of the three ways for parameter binding like ?, but sqlalchemy only allows the colon syntax.
| await session.execute( | ||
| text( | ||
| """ | ||
| SELECT | ||
| sum(cumulative_error_count), | ||
| sum(cumulative_llm_token_count_prompt), | ||
| sum(cumulative_llm_token_count_completion) | ||
| FROM spans | ||
| WHERE parent_span_id = :parent_span_id | ||
| """ | ||
| ), # noqa E501 | ||
| {"parent_span_id": span.context.span_id}, | ||
| ) | ||
| ).first(): |
There was a problem hiding this comment.
this also feels a tad in-efficient - where a read needs to be performed before you insert. If anything you should be able to execute all precursor work on a single async gather call so that the db calls are parallelized?
There was a problem hiding this comment.
This is probably fine assuming that the child spans are still in the buffer pool. On the other hand, parallelization doesn't really help sqlite because it locks the whole database
resolves #2753
Future TODOs