Repository: geofileops/geofileops
Pull request #587 (2024-12-10 16:41:44)

ENH: refactor: fill in databasename placeholders earlier on in sql_template

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/geofileops/geofileops/pull/587?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops) Report
Attention: Patch coverage is `97.61905%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 90.15%. Comparing base [(`5f08842`)](https://app.codecov.io/gh/geofileops/geofileops/commit/5f08842e475a5aaff2f4ace75e7bc77c9c2fe023?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops) to head [(`9b9be81`)](https://app.codecov.io/gh/geofileops/geofileops/commit/9b9be81ea2972e00e29f889348987fbb2b2a425f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops).
> Report is 73 commits behind head on main.

| [Files with missing lines](https://app.codecov.io/gh/geofileops/geofileops/pull/587?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops) | Patch % | Lines |
|---|---|---|
| [geofileops/util/\_geoops\_sql.py](https://app.codecov.io/gh/geofileops/geofileops/pull/587?src=pr&el=tree&filepath=geofileops%2Futil%2F_geoops_sql.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops#diff-Z2VvZmlsZW9wcy91dGlsL19nZW9vcHNfc3FsLnB5) | 95.83% | [1 Missing :warning: ](https://app.codecov.io/gh/geofileops/geofileops/pull/587?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #587      +/-   ##
==========================================
- Coverage   93.11%   90.15%   -2.97%     
==========================================
  Files          21       21              
  Lines        4113     4268     +155     
==========================================
+ Hits         3830     3848      +18     
- Misses        283      420     +137     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/geofileops/geofileops/pull/587?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops).


Patch:
diff --git a/geofileops/util/_geoops_sql.py b/geofileops/util/_geoops_sql.py
index f50d6fc..638d50b 100644
--- a/geofileops/util/_geoops_sql.py
+++ b/geofileops/util/_geoops_sql.py
@@ -12,7 +12,7 @@ from collections.abc import Iterable
 from concurrent import futures
 from datetime import datetime
 from pathlib import Path
-from typing import Literal, Optional, Union
+from typing import Any, Literal, Optional, Union
 
 import pandas as pd
 import pygeoops
@@ -628,7 +628,7 @@ def _single_layer_vector_operation(
                 sql_tmp = sql_template.format(batch_filter="")
                 cols = _sqlite_util.get_columns(
                     sql_stmt=sql_tmp,
-                    input_path=processing_params.input1_path,
+                    input_databases={"input_db": processing_params.input1_path},
                 )
                 geom_selected = input_layerinfo.geometrycolumn in cols
 
@@ -647,7 +647,8 @@ def _single_layer_vector_operation(
             # Get all columns of the sql_template
             sql_tmp = sql_template.format(batch_filter="")
             cols = _sqlite_util.get_columns(
-                sql_stmt=sql_tmp, input_path=processing_params.input1_path
+                sql_stmt=sql_tmp,
+                input_databases={"input_db": processing_params.input1_path},
             )
             attributes = [
                 col for col in cols if col.lower() != input_layerinfo.geometrycolumn
@@ -2634,7 +2635,7 @@ def _two_layer_vector_operation(
         raise ValueError(
             f"{operation_name}: output_path must not equal one of input paths"
         )
-    if use_ogr is True and input1_path != input2_path:
+    if use_ogr and input1_path != input2_path:
         raise ValueError(
             f"{operation_name}: if use_ogr True, input1_path should equal input2_path!"
         )
@@ -2747,12 +2748,18 @@ def _two_layer_vector_operation(
                 f"{input1_tmp_layerinfo.crs} \n\tinput2: {input2_tmp_layerinfo.crs}"
             )
 
+        # Prepare the database names to fill out in the sql_template
+        input_db_placeholders, input_db_names = _prepare_input_db_names(
+            [processing_params.input1_path, processing_params.input2_path],
+            use_ogr=use_ogr,
+        )
+
         # Fill out sql_template as much as possible already
         # -------------------------------------------------
         # Keep input1_tmp_layer and input2_tmp_layer for backwards compatibility
         sql_template = sql_template.format(
-            input1_databasename="{input1_databasename}",
-            input2_databasename="{input2_databasename}",
+            input1_databasename=input_db_placeholders["input1_databasename"]["db_name"],
+            input2_databasename=input_db_placeholders["input2_databasename"]["db_name"],
             layer1_columns_from_subselect_str=input1_col_strs.from_subselect(),
             layer1_columns_prefix_alias_str=input1_col_strs.prefixed_aliased(),
             layer1_columns_prefix_str=input1_col_strs.prefixed(),
@@ -2773,13 +2780,10 @@ def _two_layer_vector_operation(
         column_datatypes = None
         # Use first batch_filter to improve performance
         sql_stmt = sql_template.format(
-            input1_databasename="{input1_databasename}",
-            input2_databasename="{input2_databasename}",
-            batch_filter=processing_params.batches[0]["batch_filter"],
+            batch_filter=processing_params.batches[0]["batch_filter"]
         )
         column_datatypes = _sqlite_util.get_columns(
-            sql_stmt=sql_stmt,
-            input_path=[processing_params.input1_path, processing_params.input2_path],
+            sql_stmt=sql_stmt, input_databases=input_db_names
         )
 
         # Apply gridsize if it is specified
@@ -2897,8 +2901,7 @@ def _two_layer_vector_operation(
                 # Remark: this temp file doesn't need spatial index
                 future = calculate_pool.submit(
                     _calculate_two_layers,
-                    input1_path=processing_params.batches[batch_id]["input1_path"],
-                    input2_path=processing_params.batches[batch_id]["input2_path"],
+                    input_databases=input_db_names,
                     output_path=tmp_partial_output_path,
                     sql_stmt=sql_stmt,
                     output_layer=output_layer,
@@ -3009,6 +3012,35 @@ def _two_layer_vector_operation(
             shutil.rmtree(tmp_dir, ignore_errors=True)
 
 
+def _prepare_input_db_names(
+    input_paths: list[Optional[Path]], use_ogr
+) -> tuple[dict, dict]:
+    input_db_placeholders: dict[str, dict[str, Any]] = {}
+    input_db_names: dict[str, Path] = {}
+    for index, path in enumerate(input_paths):
+        db_placeholder = f"input{index + 1}_databasename"
+
+        # If path is already in input_databases, reuse the db_name
+        db_name = None
+        for _, db_info in input_db_placeholders.items():
+            if db_info["path"] == path:
+                db_name = db_info["db_name"]
+                break
+
+        if db_name is None and path is not None:
+            if use_ogr:
+                # use_ogr needs main as db_name
+                db_name = "main"
+            else:
+                db_name = f"input{index + 1}"
+
+        input_db_placeholders[db_placeholder] = {"db_name": db_name, "path": path}
+        if db_name is not None and path is not None:
+            input_db_names[db_name] = path
+
+    return input_db_placeholders, input_db_names
+
+
 def _check_crs(
     input1_path: Path, input1_layer: str, input2_path: Path, input2_layer: str
 ) -> int:
@@ -3043,8 +3075,7 @@ def _check_crs(
 
 
 def _calculate_two_layers(
-    input1_path: Path,
-    input2_path: Path,
+    input_databases: dict[str, Path],
     output_path: Path,
     sql_stmt: str,
     output_layer: str,
@@ -3055,15 +3086,16 @@ def _calculate_two_layers(
     column_datatypes: dict,
     use_ogr: bool,
 ):
-    if use_ogr is False:
+    if not use_ogr:
         # If explodecollections, write first to tmp file, then apply explodecollections
         # to the final output file.
         output_tmp_path = output_path
         if explodecollections:
             output_name = f"{output_path.stem}_tmp{output_path.suffix}"
             output_tmp_path = output_path.parent / output_name
+
         _sqlite_util.create_table_as_sql(
-            input_path=[input1_path, input2_path],
+            input_databases=input_databases,
             output_path=output_tmp_path,
             sql_stmt=sql_stmt,
             output_layer=output_layer,
@@ -3073,6 +3105,7 @@ def _calculate_two_layers(
             profile=_sqlite_util.SqliteProfile.SPEED,
             column_datatypes=column_datatypes,
         )
+
         if explodecollections:
             _ogr_util.vector_translate(
                 input_path=output_tmp_path,
@@ -3087,15 +3120,12 @@ def _calculate_two_layers(
             gfo.remove(output_tmp_path)
     else:
         # Use ogr to run the query
-        #   * input2 path (= using attach) doesn't seem to work
-        #   * ogr doesn't fill out database names, so do it now
-        sql_stmt = sql_stmt.format(
-            input1_databasename="main",
-            input2_databasename="main",
-        )
+        #   * input2 path (= using attach) doesn't seem to work at time of writing
+        if len(input_databases) != 1:
+            raise ValueError("use_ogr=True only supports one input file")
 
         _ogr_util.vector_translate(
-            input_path=input1_path,
+            input_path=next(iter(input_databases.values())),
             output_path=output_path,
             sql_stmt=sql_stmt,
             output_layer=output_layer,
diff --git a/geofileops/util/_sqlite_util.py b/geofileops/util/_sqlite_util.py
index 6763998..d1b1850 100644
--- a/geofileops/util/_sqlite_util.py
+++ b/geofileops/util/_sqlite_util.py
@@ -7,7 +7,6 @@ import pprint
 import shutil
 import sqlite3
 import tempfile
-from collections.abc import Iterable
 from pathlib import Path
 from typing import Optional, Union
 
@@ -126,54 +125,45 @@ def create_new_spatialdb(path: Path, crs_epsg: Optional[int] = None):
 
 def get_columns(
     sql_stmt: str,
-    input_path: Union[Path, list[Path]],
+    input_databases: dict[str, Path],
     empty_output_ok: bool = True,
     use_spatialite: bool = True,
     output_geometrytype: Optional[GeometryType] = None,
 ) -> dict[str, str]:
-    # Create temp output db to be sure the output DB is writable, even though we only
-    # create a temporary table.
-    if isinstance(input_path, (str, Path)):
-        input_path = [input_path]
-    assert isinstance(input_path, Iterable)
-
-    tmp_dir = Path(tempfile.mkdtemp(prefix="geofileops/get_columns_"))
-    tmp_path = tmp_dir / f"temp{input_path[0].suffix}"
-    create_new_spatialdb(path=tmp_path)
+    # Connect to/create sqlite main database
+    tmp_dir = None
+    if "main" in input_databases:
+        # If an input database is main, use it as the main database
+        main_db_path = input_databases["main"]
+    else:
+        # Create temp output db to be sure the output DB is writable, even though we
+        # only create a temporary table.
+        tmp_dir = Path(tempfile.mkdtemp(prefix="geofileops/get_columns_"))
+        main_db_path = tmp_dir / f"temp{next(iter(input_databases.values())).suffix}"
+        create_new_spatialdb(path=main_db_path)
 
     sql = None
-    conn = sqlite3.connect(tmp_path, detect_types=sqlite3.PARSE_DECLTYPES)
+    conn = sqlite3.connect(main_db_path, detect_types=sqlite3.PARSE_DECLTYPES)
     try:
         # Load spatialite if asked for
         if use_spatialite:
             load_spatialite(conn)
-            if tmp_path.suffix.lower() == ".gpkg":
+            if main_db_path.suffix.lower() == ".gpkg":
                 sql = "SELECT EnableGpkgMode();"
                 conn.execute(sql)
 
             # Attach to all input databases
-            db_path_to_placeholder: dict[Path, str] = {}
-            db_placeholder_to_value: dict[str, str] = {}
-            for index, path in enumerate(input_path):
-                db_placeholder = f"input{index + 1}_databasename"
-                if path in db_path_to_placeholder:
-                    db_placeholder_to_value[db_placeholder] = db_placeholder_to_value[
-                        db_path_to_placeholder[path]
-                    ]
+            for dbname, path in input_databases.items():
+                # main is already opened, so skip it
+                if dbname == "main":
                     continue
 
-                input_databasename = f"input{index + 1}"
-                sql = f"ATTACH DATABASE ? AS {input_databasename}"
+                sql = f"ATTACH DATABASE ? AS {dbname}"
                 dbSpec = (str(path),)
                 conn.execute(sql, dbSpec)
-                db_path_to_placeholder[path] = db_placeholder
-                db_placeholder_to_value[db_placeholder] = input_databasename
 
         # Prepare sql statement for execute
-        sql_stmt_prepared = sql_stmt.format(
-            **db_placeholder_to_value,
-            batch_filter="",
-        )
+        sql_stmt_prepared = sql_stmt.format(batch_filter="")
 
         # Log explain plan if debug logging enabled.
         if logger.isEnabledFor(logging.DEBUG):
@@ -269,13 +259,14 @@ def get_columns(
     finally:
         conn.close()
         if ConfigOptions.remove_temp_files:
-            shutil.rmtree(tmp_dir, ignore_errors=True)
+            if tmp_dir is not None:
+                shutil.rmtree(tmp_dir, ignore_errors=True)
 
     return columns
 
 
 def create_table_as_sql(
-    input_path: Union[Path, list[Path]],
+    input_databases: dict[str, Path],
     output_path: Path,
     sql_stmt: str,
     output_layer: str,
@@ -291,7 +282,8 @@ def create_table_as_sql(
     """Execute sql statement and save the result in the output file.
 
     Args:
-        input_path (Path or list): the path(s) to the input file(s).
+        input_databases (dict): dict with the database name(s) and path(s) to the input
+            database(s).
         output_path (Path): the path where the output file needs to be created/appended.
         sql_stmt (str): SELECT statement to run on the input files.
         output_layer (str): layer/table name to use.
@@ -323,12 +315,8 @@ def create_table_as_sql(
         raise ValueError("append=True nor update=True are implemented.")
 
     # All input files and the output file must have the same suffix.
-    if isinstance(input_path, (str, Path)):
-        input_path = [input_path]
-    assert isinstance(input_path, Iterable)
-
     output_suffix_lower = output_path.suffix.lower()
-    for path in input_path:
+    for path in input_databases.values():
         if output_suffix_lower != path.suffix.lower():
             raise ValueError(
                 "output_path and all input paths must have the same suffix!"
@@ -371,22 +359,10 @@ def create_table_as_sql(
             conn.execute(sql)
 
             # Attach to all input databases
-            db_path_to_placeholder: dict[Path, str] = {}
-            db_placeholder_to_value: dict[str, str] = {}
-            for index, path in enumerate(input_path):
-                db_placeholder = f"input{index+1}_databasename"
-                if path in db_path_to_placeholder:
-                    db_placeholder_to_value[db_placeholder] = db_placeholder_to_value[
-                        db_path_to_placeholder[path]
-                    ]
-                    continue
-
-                input_databasename = f"input{index + 1}"
-                sql = f"ATTACH DATABASE ? AS {input_databasename}"
+            for dbname, path in input_databases.items():
+                sql = f"ATTACH DATABASE ? AS {dbname}"
                 dbSpec = (str(path),)
                 conn.execute(sql, dbSpec)
-                db_path_to_placeholder[path] = db_placeholder
-                db_placeholder_to_value[db_placeholder] = input_databasename
 
             # Use the sqlite profile specified
             if profile is SqliteProfile.SPEED:
@@ -396,10 +372,7 @@ def create_table_as_sql(
 
                 # These options don't really make a difference on windows, but
                 # it doesn't hurt and maybe on other platforms...
-                for databasename in [
-                    output_databasename,
-                    *db_placeholder_to_value.values(),
-                ]:
+                for databasename in [output_databasename, *input_databases.keys()]:
                     conn.execute(f"PRAGMA {databasename}.journal_mode=OFF;")
 
                     # These pragma's increase speed
@@ -411,7 +384,7 @@ def create_table_as_sql(
             if column_types is None:
                 column_types = get_columns(
                     sql_stmt=sql_stmt,
-                    input_path=input_path,
+                    input_databases=input_databases,
                     empty_output_ok=empty_output_ok,
                     use_spatialite=True,
                     output_geometrytype=output_geometrytype,
@@ -421,9 +394,6 @@ def create_table_as_sql(
             if output_geometrytype is None and "geom" in column_types:
                 output_geometrytype = GeometryType(column_types["geom"])
 
-            # Prepare sql statement
-            sql_stmt = sql_stmt.format(**db_placeholder_to_value)
-
             # Now we can create the table
             # Create output table using the gpkgAddGeometryColumn() function
             # Problem: the spatialite function gpkgAddGeometryColumn() doesn't support


Test patch:
diff --git a/tests/test_sqlite_util.py b/tests/test_sqlite_util.py
index 7e31434..6c95fd9 100644
--- a/tests/test_sqlite_util.py
+++ b/tests/test_sqlite_util.py
@@ -42,9 +42,12 @@ def test_create_table_as_sql(tmp_path, create_spatial_index):
                AND ST_Intersects(layer1.geom, layer2.geometry) = 1
                AND ST_Touches(layer1.geom, layer2.geometry) = 0
             """
+    sql_stmt = sql_stmt.format(
+        input1_databasename="input1_db", input2_databasename="input2_db"
+    )
 
     sqlite_util.create_table_as_sql(
-        input_path=[input1_path, input2_path],
+        input_databases={"input1_db": input1_path, "input2_db": input2_path},
         output_path=output_path,
         output_layer=output_path.stem,
         output_geometrytype=gfo.GeometryType.MULTIPOLYGON,
@@ -72,26 +75,36 @@ def test_create_table_as_sql(tmp_path, create_spatial_index):
         ({"append": True}, "append=True nor update=True are implemented."),
         ({"update": True}, "append=True nor update=True are implemented."),
         (
-            {"input_path": [Path("input1.sqlite"), Path("input2.gpkg")]},
+            {
+                "input_databases": {
+                    "input1": Path("input1.sqlite"),
+                    "input2": Path("input2.gpkg"),
+                }
+            },
             "output_path and all input paths must have the same suffix!",
         ),
         (
-            {"input_path": [Path("input1.gpkg"), Path("input2.sqlite")]},
+            {
+                "input_databases": {
+                    "input1": Path("input1.gpkg"),
+                    "input2": Path("input2.sqlite"),
+                }
+            },
             "output_path and all input paths must have the same suffix!",
         ),
         (
-            {
-                "input_path": [Path("input1.gpkg"), Path("input2.gpkg")],
-                "output_path": Path("output.sqlite"),
-            },
+            {"output_path": Path("output.sqlite")},
             "output_path and all input paths must have the same suffix!",
         ),
     ],
 )
 def test_create_table_as_sql_invalidparams(kwargs, expected_error):
     # Set default values for kwargs that are not specified:
-    if "input_path" not in kwargs:
-        kwargs["input_path"] = [Path("input1.gpkg"), Path("input2.gpkg")]
+    if "input_databases" not in kwargs:
+        kwargs["input_databases"] = {
+            "input1_db": Path("input1.gpkg"),
+            "input2_db": Path("input2.gpkg"),
+        }
     if "output_path" not in kwargs:
         kwargs["output_path"] = Path("output.gpkg")
     if "output_layer" not in kwargs:
@@ -139,8 +152,8 @@ def test_get_columns():
     # was a bug (https://github.com/geofileops/geofileops/pull/477).
     sql_stmt = f"""
         SELECT layer1.OIDN, layer1.UIDN, layer1.datum, layer2.naam, 'test' AS naam
-          FROM {{input1_databasename}}."{input1_info.name}" layer1
-          CROSS JOIN {{input2_databasename}}."{input2_info.name}" layer2
+          FROM input1_db."{input1_info.name}" layer1
+          CROSS JOIN input2_db."{input2_info.name}" layer2
          WHERE 1=1
     """
 
@@ -150,7 +163,8 @@ def test_get_columns():
 
     # Run test
     columns = sqlite_util.get_columns(
-        sql_stmt=sql_stmt, input_path=[input1_path, input2_path]
+        sql_stmt=sql_stmt,
+        input_databases={"input1_db": input1_path, "input2_db": input2_path},
     )
 
     assert len(columns) == 5
@@ -174,7 +188,7 @@ def test_create_table_as_sql_single_input(tmp_path):
           FROM "parcels" layer
     """
     sqlite_util.create_table_as_sql(
-        input_path=input_path,
+        input_databases={"input": input_path},
         output_path=output_path,
         output_layer=None,
         output_geometrytype=None,