grass.temporal: support fully qualified names on read (#7641)#7670
grass.temporal: support fully qualified names on read (#7641)#7670gkneighb wants to merge 2 commits into
Conversation
|
Heads-up on the red macOS build check here: it's a pre-existing, unrelated test-isolation flake, not something in this PR. |
ninsbl
left a comment
There was a problem hiding this comment.
Hi @gkneighb and thanks for your PR. Extending the SQLDatabaseConnection class with an add_mapset method is one option to solve the issue that I have not thought about before. Could be a good approach.
However, injecting a call to that function to every attempt to fetch data from the TGIS DB is probably not what I would do, as the sql functions may get executed quite frequently, and there we would not want to have avoidable overhead.
I can imagine two slightly alternative approaches.
- One approach may be to modify the open_stds functions to identify is the mapset in the ident is in the SQLDatabaseConnection and use your add_mapset function if not.
I have not checked if all tools use that function set or rather e.g. dataset factory, or even the space_time_datasets We would have to research a bit. The advantade with such a solution on the library level is that we do not have to adjust all the tools. - Another approach would be that all tools that take STDS as input, do the following:
a) get (all) required mapsets from user input (fully qualified dataset names)
b) initialize SQLDatabaseConnection with those mapsets (in addition to the current mapset if write access is required).
| return | ||
| new_mapsets = get_available_temporal_mapsets(mapset) | ||
| if mapset not in new_mapsets: | ||
| return |
There was a problem hiding this comment.
If one actively tries to add a mapset to the SQLDatabaseConnection object, that either does not exist or does not have a temporal database, at least a warning should be given.
However, that of course depends on how the function is supposed to be used. With the pattern below (with possibly frequent calls) a warning would be annoying...
There was a problem hiding this comment.
On the warning: the existing guard already surfaces this — if the mapset has no accessible temporal DB, add_mapset no-ops and the if mapset not in self.tgis_mapsets: fatal(...) in the query methods raises the usual "no permission ... or no temporal database" error; a genuinely missing dataset falls through to the normal not-found path. A warning inside add_mapset would also fire in the benign "already present" case. If we move the call up to the per-dataset entry points (see the main comment), a targeted warning there is cleaner and I can add it.
|
On the overhead: agreed the connection's SQL methods ( To scope the alternatives I checked which read paths actually reach an off-search-path mapset, and it's broader than
So Alt 1 (open_stds only) would leave Proposed middle ground that avoids the per-statement overhead: call Does that direction work for you? If so I'll rework it that way. For reference, I verified coverage on a two-mapset setup (STRDS in an off-search-path mapset, queried from PERMANENT): |
|
OK, I think it would be reasonable to limit the scope of this PR to:
Tools like t.info create the database interface separately, so they would have to make sure that the interface contains connections for the required mapsets. t.info then call database related function with that interface, e.g. For such cases like it may make sense to create a helper function to check if an ID references a dataset on the search path mapset. But I am not sure where or how such a function should be implemented in detail. Would have to think about it a bit more... |
1122073 to
4079381
Compare
|
Reworked to that scope: |
Add an idempotent method to include a mapset's temporal database in an existing connection, enabling access to space time datasets outside the current search path. Part of OSGeo#7641. Authored with the help of an AI assistant.
open_old_stds() now ensures the dataset's own mapset is part of the database connection (using SQLDatabaseInterfaceConnection.add_mapset), so a fully qualified name (name@mapset) can be opened even when the mapset is not on the current search path. Scope is limited to tools that go through open_old_stds. Tools that build their own database interface (e.g. t.info) need to add the required mapset to that interface themselves and are left as follow-up. Part of OSGeo#7641. Authored with the help of an AI assistant.
4079381 to
8c8761e
Compare
Summary
Temporal read tools could only access a space time dataset from mapsets on the current search path, even when given a fully qualified name (
name@mapset). This makesopen_old_stds()resolve a fully qualified name from any accessible mapset, matching how other GRASS data types behave.Part of #7641.
Approach
SQLDatabaseInterfaceConnection.add_mapset()that pulls an accessible mapset's temporal database into an existing connection (building on themapsetssupport from t.list: Add mapset options for t.list #7631 / temporal: add parameter to skip creation of TGIS DB in current mapset on tgis.init #7645; a no-op when the mapset is already present).open_old_stds(), after the connection is initialized, so the dataset's own mapset is part of the connection before the dataset is queried.Scope
Limited to tools that open datasets through
open_old_stds. Tools that build their own database interface and bypassopen_old_stds(e.g.t.info, which callsis_in_db(dbif)on an interface it created itself) must ensure the required mapset is added to that interface; that is left as follow-up. The write/create path is unchanged (datasets are still created only in the current mapset).Test plan
New pytest suite
python/grass/temporal/tests/grass_temporal_fully_qualified_names_test.py:add_mapsetunit: adds an off-search-path mapset to a connection; idempotent.open_old_stdsresolvesname@mapsetfor a mapset off the search path.AI assistance
Implemented with the assistance of an AI coding assistant (Claude), reviewed and verified by the author.