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

[Bug] temporal tools fail with SQLite 3.31 #3466

Closed
wenzeslaus opened this issue Mar 1, 2024 · 10 comments
Closed

[Bug] temporal tools fail with SQLite 3.31 #3466

wenzeslaus opened this issue Mar 1, 2024 · 10 comments
Labels
blocker Blocking a release bug Something isn't working temporal Related to temporal data processing
Milestone

Comments

@wenzeslaus
Copy link
Member

Describe the bug

The temporal tools fail with SQLite 3.31 although minimal supported version according to #3350 is 3.30 (REQUIREMENTS.md does not specify a minimal SQLite version).

5e3858b works and 0c73ced does not, so this points to #3350.

To Reproduce
Steps to reproduce the behavior:

Run test:

pytest temporal/t.rast.list/tests/t_rast_list_test.py::test_defaults

Possibly like this:

LD_LIBRARY_PATH=$(.../bin.x86_64-pc-linux-gnu/grass --config path)/lib:$LD_LIBRARY_PATH PYTHONPATH=$(.../bin.x86_64-pc-linux-gnu/grass --config python-path):$PYTHONPATH pytest temporal/t.rast.list/tests/t_rast_list_test.py::test_defaults

Expected behavior

According to #3350, the code should work with SQLite 3.30, but it does not work for me with 3.31. Maybe the requirements are higher?

Screenshots

----------------------------------------------------------------------------------- Captured stderr setup ------------------------------------------------------------------------------------
Default TGIS driver / database set to:
driver: sqlite
database: $GISDBASE/$LOCATION_NAME/$MAPSET/tgis/sqlite.db
WARNING: Temporal database connection defined as:
         /tmp/.../pytest-252/raster_time_series0/test/PERMANENT/tgis/sqlite.db
         But database file does not exist.
Creating temporal database:
/tmp/.../pytest-252/raster_time_series0/test/PERMANENT/tgis/sqlite.db
 100%
ERROR: Unable to execute transaction:
--#############################################################################
-- This SQL script is to update the spatial and temporal extent as well as
-- the modification time and revision of a space time dataset. This script
-- should be called when maps inserted or deleted in a space time dataset.
...
raster_map_register_6481a51fe8d1409e9c41be6ceb12226d.id =
raster_metadata.id
) AS new_stats
WHERE strds_metadata.id = 'precipitation@PERMANENT';
ERROR: near "FROM": syntax error
================================================================================== short test summary info ===================================================================================
ERROR temporal/t.rast.list/tests/t_rast_list_test.py::test_defaults - grass.exceptions.CalledModuleError: Module run `t.register -i type=raster input=precipitation file=/tmp/...

System description

  • Operating System: Linux, Pop!_OS 20.04 LTS (Ubuntu 20.04)
  • GRASS GIS version: main branch
$ .../bin.x86_64-pc-linux-gnu/grass --tmp-location XY --exec g.version -rge
version=8.4.0dev
date=2024
revision=76bcffb357
build_date=2024-02-29
build_platform=x86_64-pc-linux-gnu
build_off_t_size=8
libgis_revision=7d9bc36105
libgis_date=2024-02-07T12:39:27+00:00
proj=8.2.0
gdal=3.4.3
geos=3.10.2
sqlite=3.31.1

$ python
>>> import sqlite
>>> sqlite3.sqlite_version
'3.31.1'

Additional context

CI works with Ubuntu 22.04.

Clearly, I should upgrade from 20.04, but that's not the point here :-)

@wenzeslaus wenzeslaus added the bug Something isn't working label Mar 1, 2024
@ninsbl
Copy link
Member

ninsbl commented Mar 1, 2024

Ops, sorry! I think the culprit is actually: #3359
I read that that the UPDATE-FROM syntax was added in 3.30, while in fact it was added in 3.33. I should have checked the primary source for the changelog of SQLite (https://www.sqlite.org/changes.html).

3.33 is available in:

  • Alpine >= 3.13
  • CentOS Stream >= 9
  • Cygwin
  • Debian >= 11
  • Fedora >= 32
  • Ubuntu >= 22.04

So, Ubuntu 20.04 ships 3.31, which i not compatible... 20.04 is End of Standardsupport by April 2025, so ~next year...

What should we do? Revert #3359 until then? #3350 is the bigger performance improvement and should not increase the required SQLite verion....

@nilason
Copy link
Contributor

nilason commented Mar 1, 2024

You could perhaps do a version check, like:

if driver == "sqlite":
sqlite3_version = gscript.read_command(
"db.select",
sql="SELECT sqlite_version();",
flags="c",
database=database,
driver=driver,
).split(".")[0:2]
if [int(i) for i in sqlite3_version] >= [int(i) for i in "3.35".split(".")]:

and use the syntax supported for older versions only there (even if it is slower).

@echoix
Copy link
Member

echoix commented Mar 2, 2024

Are the SQLite operation done by Python? In that case, is the version of SQLite dependent on the Python version (if from the standard library, ie not using shared library)?

@wenzeslaus wenzeslaus added the blocker Blocking a release label May 15, 2024
@wenzeslaus wenzeslaus added this to the 8.4.0 milestone May 15, 2024
@wenzeslaus
Copy link
Member Author

This needs to be resolved (one way or the other) before the 8.4 release. What do you think @ninsbl?

@ninsbl
Copy link
Member

ninsbl commented May 15, 2024

Hm, I would suggest to revert #3359, re-open the PR and wait with merging until Ubuntu 20.04 is out of support (which is less than a year from now)...

@echoix
Copy link
Member

echoix commented May 15, 2024

Is there a way that both can be kept until then? What amount of performance regression would we be trading with a revert like that? (We would be worsening the goal of #2185 no?)

@ninsbl
Copy link
Member

ninsbl commented May 15, 2024

We could reactivate the old templates and use them depending on the DB backend and version. Maybe I can look into that tonight...

@echoix
Copy link
Member

echoix commented May 15, 2024

Great! Still one year is quite long, and I'd really like that the 8.4.0 release would be out before the community sprint, so we could be ambitious with our development during that week, instead of trying to catch up with the release that is due, or still taking some time to test out the RCs.

@wenzeslaus or @neteler, is it still realistic to be finished with 8.4.0 before the sprint or I'm only dreaming?

@neteler
Copy link
Member

neteler commented May 15, 2024

In our recent PSC meeting we decided that the release should happen before the community meeting!

@ninsbl ninsbl added the temporal Related to temporal data processing label May 18, 2024
@ninsbl
Copy link
Member

ninsbl commented May 24, 2024

Fixed in #3723, (that PR should be reverted when Ubuntu 20.04 is EOL, 04.2025, see #3736)

@ninsbl ninsbl closed this as completed May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Blocking a release bug Something isn't working temporal Related to temporal data processing
Projects
None yet
Development

No branches or pull requests

5 participants