Skip to content
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

Support for generated columns #1117

Merged
merged 5 commits into from Nov 30, 2020
Merged

Support for generated columns #1117

merged 5 commits into from Nov 30, 2020

Conversation

simonw
Copy link
Owner

@simonw simonw commented Nov 30, 2020

Refs #1116. My first attempt at this worked on my laptop but broke in CI, so I'm going to iterate on it in a pull request instead.

@simonw simonw mentioned this pull request Nov 30, 2020
@simonw
Copy link
Owner Author

simonw commented Nov 30, 2020

I need to replicate these failures on my laptop. My hunch is that this is down to the version of SQLite available to Python.

@simonw
Copy link
Owner Author

simonw commented Nov 30, 2020

On my laptop:

platform darwin -- Python 3.8.6, pytest-6.0.1, py-1.9.0, pluggy-0.13.1
SQLite: 3.33.0

In CI they are all SQLite: 3.22.0

@simonw
Copy link
Owner Author

simonw commented Nov 30, 2020

This kind of problem is why I have a tmate workflow:

Actions_·_simonw_datasette

@simonw
Copy link
Owner Author

simonw commented Nov 30, 2020

Here's the problem: https://www.sqlite.org/changes.html#version_3_26_0

2018-12-01 (3.26.0)

CI is running 3.22.0.

sqlite3.enable_callback_tracebacks(True)


def sqlite_version():
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm nervous about the performance impact of this, since every time it is called it creates a new in-memory connection.

I'm going to cache this so that it only ever gets called a maximum of once for the lifetime of the server.

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #1117 (ccdf2c6) into main (dea3c50) will decrease coverage by 0.00%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1117      +/-   ##
==========================================
- Coverage   91.48%   91.48%   -0.01%     
==========================================
  Files          30       31       +1     
  Lines        3841     3852      +11     
==========================================
+ Hits         3514     3524      +10     
- Misses        327      328       +1     
Impacted Files Coverage Δ
datasette/utils/__init__.py 94.10% <87.50%> (-0.20%) ⬇️
datasette/utils/sqlite.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dea3c50...ccdf2c6. Read the comment docs.

@simonw simonw merged commit 461670a into main Nov 30, 2020
@simonw simonw deleted the generated-columns branch November 30, 2020 21:29
@nattaylor
Copy link

nattaylor commented Nov 30, 2020

I just deployed this and its working great.

In a very unscientific benchmark my response times went from around 22-25ms to 33-36ms, but I didn't even dig enough to confirm the latency is related to the change. It's on a VPS, so maybe the load changed. I don't see any difference in performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants