Repository: geofileops/geofileops (python)

Issue:
ENH: improve handling of string passed for `columns` parameter in e.g. `copy_layer`
Now this is treated as any other iterable, requesting columns for each character in the string.

Easiest for the user would be that a single string is also supported and is treated that there is only one column to be retained, so everywhere before a `columns` - like columns is used put something like this:
``` py
if isinstance(columns, str):
    columns = [columns]
```

In practice this will have to end up on several places:
- _ogr_util.vector_translate
- fileops._append_to_nolock
- _geoops_sql._single_layer_vector_operation
- _geoops_sql._two_layer_vector_operation
- _geoops_gpd._apply_geooperation_to_layer
- _geoops_gpd.dissolve
- ...?

Pull request description:
ENH: improve handling of string passed for columns parameter
resolves #523

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a4a9e2d..3ccb375 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,8 @@
 ### Improvements
 
 - Add support for renaming column with only difference in casing (#549)
+- Improve handling of string passed for columns parameter in e.g. copy_layer (#523)
+  Single string is also supported and is treated that there is only one column to be retained
 
 ## 0.9.1 (2024-07-18)
 
diff --git a/geofileops/fileops.py b/geofileops/fileops.py
index da25882..bcc003d 100644
--- a/geofileops/fileops.py
+++ b/geofileops/fileops.py
@@ -1123,6 +1123,9 @@ def _read_file_base(
     """Reads a file to a pandas Dataframe."""
     # Check if the fid column needs to be read as column via the columns parameter
     fid_as_column = False
+    if isinstance(columns, str):
+        # If a string is passed, convert to list
+        columns = [columns]
     if columns is not None:
         if "fid" in [column.lower() for column in columns]:
             fid_as_column = True
@@ -2274,6 +2277,10 @@ def _append_to_nolock(
     options: dict = {},
 ):
     # Check/clean input params
+    if isinstance(columns, str):
+        # If a string is passed, convert to list
+        columns = [columns]
+
     options = _ogr_util._prepare_gdal_options(options)
     if (
         create_spatial_index is not None
diff --git a/geofileops/util/_geoops_gpd.py b/geofileops/util/_geoops_gpd.py
index 4e20e8a..8235ad9 100644
--- a/geofileops/util/_geoops_gpd.py
+++ b/geofileops/util/_geoops_gpd.py
@@ -717,6 +717,9 @@ def _apply_geooperation_to_layer(
         output_layer = gfo.get_default_layer(output_path)
     if isinstance(force_output_geometrytype, GeometryType):
         force_output_geometrytype = force_output_geometrytype.name
+    if isinstance(columns, str):
+        # If a string is passed, convert to list
+        columns = [columns]
 
     # Check if we want to preserve the fid in the output
     preserve_fid = False
diff --git a/geofileops/util/_geoops_sql.py b/geofileops/util/_geoops_sql.py
index ac1adf3..2e68830 100644
--- a/geofileops/util/_geoops_sql.py
+++ b/geofileops/util/_geoops_sql.py
@@ -534,6 +534,9 @@ def _single_layer_vector_operation(
         raise ValueError(f"{operation_name}: output_path must not equal input_path")
     if where_post is not None and where_post == "":
         where_post = None
+    if isinstance(columns, str):
+        # If a string is passed, convert to list
+        columns = [columns]
 
     # Check/get layer names
     if input_layer is None:
@@ -2609,6 +2612,13 @@ def _two_layer_vector_operation(
     # Init
     logger = logging.getLogger(f"geofileops.{operation_name}")
 
+    if isinstance(input1_columns, str):
+        # If a string is passed, convert to list
+        input1_columns = [input1_columns]
+    if isinstance(input2_columns, str):
+        # If a string is passed, convert to list
+        input2_columns = [input2_columns]
+
     if not input1_path.exists():
         raise ValueError(f"{operation_name}: input1_path doesn't exist: {input1_path}")
     if not input2_path.exists():
diff --git a/geofileops/util/_ogr_util.py b/geofileops/util/_ogr_util.py
index 9ed620f..1366223 100644
--- a/geofileops/util/_ogr_util.py
+++ b/geofileops/util/_ogr_util.py
@@ -243,6 +243,10 @@ def vector_translate(
     args = []
     if isinstance(input_path, str):
         input_path = Path(input_path)
+    if isinstance(columns, str):
+        # If a string is passed, convert to list
+        columns = [columns]
+
     gdal_options = _prepare_gdal_options(options, split_by_option_type=True)
 
     # Input file parameters


Test patch:
diff --git a/tests/test_geofile.py b/tests/test_geofile.py
index f458bfd..af7c427 100644
--- a/tests/test_geofile.py
+++ b/tests/test_geofile.py
@@ -244,15 +244,16 @@ def test_copy_layer(tmp_path, dimensions, suffix_input, suffix_output):
 
 
 @pytest.mark.parametrize("suffix", [s for s in SUFFIXES_FILEOPS if s != ".csv"])
-def test_copy_layer_columns(tmp_path, suffix):
+@pytest.mark.parametrize("columns", [["OIDN", "UIDN"], "OIDN"])
+def test_copy_layer_columns(tmp_path, suffix, columns):
     # Prepare test data
     src = test_helper.get_testfile("polygon-parcel", suffix=suffix)
 
     # Test
     dst = tmp_path / f"output{suffix}"
-    gfo.copy_layer(src, dst, columns=["OIDN", "UIDN"])
+    gfo.copy_layer(src, dst, columns=columns)
     copy_gdf = gfo.read_file(dst)
-    input_gdf = gfo.read_file(src, columns=["OIDN", "UIDN"])
+    input_gdf = gfo.read_file(src, columns=columns)
     assert_geodataframe_equal(input_gdf, copy_gdf)
 
 
@@ -839,9 +840,10 @@ def test_read_file(suffix, dimensions, engine_setter):
     "columns, geometry",
     [
         ([], "YES"),
-        (["OIDN", "uidn", "HFDTLT", "lblhfdtlt", "GEWASGROEP", "lengte"], "YES"),
-        (["OIDN", "uidn", "HFDTLT", "lblhfdtlt", "GEWASGROEP", "lengte"], "NO"),
-        (["OIDN", "uidn", "HFDTLT", "lblhfdtlt", "GEWASGROEP", "lengte"], "IGNORE"),
+        ("OIDN", "YES"),
+        (["OIDN", "GEWASGROEP", "lengte"], "YES"),
+        (["OIDN", "GEWASGROEP", "lengte"], "NO"),
+        (["OIDN", "GEWASGROEP", "lengte"], "IGNORE"),
     ],
 )
 def test_read_file_columns_geometry(tmp_path, suffix, columns, geometry, engine_setter):
@@ -889,9 +891,9 @@ def test_read_file_columns_geometry(tmp_path, suffix, columns, geometry, engine_
     else:
         expect_geometry = True
 
-    exp_columns = columns
+    exp_columns = list(columns) if isinstance(columns, list) else [columns]
     if expect_geometry:
-        exp_columns = columns + ["geometry"]
+        exp_columns += ["geometry"]
 
     if columns == [] and not expect_geometry:
         exp_featurecount = 0
diff --git a/tests/test_geofileops_singlelayer.py b/tests/test_geofileops_singlelayer.py
index b85f4bd..2377375 100644
--- a/tests/test_geofileops_singlelayer.py
+++ b/tests/test_geofileops_singlelayer.py
@@ -237,7 +237,13 @@ def test_buffer(
 @pytest.mark.parametrize("suffix", SUFFIXES_GEOOPS)
 @pytest.mark.parametrize("testfile", ["polygon-parcel"])
 @pytest.mark.parametrize("geoops_module", GEOOPS_MODULES)
-def test_buffer_columns_fid(tmp_path, suffix, geoops_module, testfile):
+@pytest.mark.parametrize(
+    "columns, exp_columns",
+    [(["LblHfdTlt", "fid"], ["LblHfdTlt", "fid_1"]), ("LblHfdTlt", ["LblHfdTlt"])],
+)
+def test_buffer_columns_fid(
+    tmp_path, suffix, geoops_module, testfile, columns, exp_columns
+):
     """Buffer basics are available both in the gpd and sql implementations."""
     # Prepare test data
     input_path = test_helper.get_testfile(testfile, suffix=suffix)
@@ -253,7 +259,7 @@ def test_buffer_columns_fid(tmp_path, suffix, geoops_module, testfile):
         input_path=input_path,
         output_path=output_path,
         distance=1,
-        columns=["LblHfdTlt", "fid"],
+        columns=columns,
         explodecollections=True,
         keep_empty_geoms=False,
         nb_parallel=2,
@@ -279,8 +285,9 @@ def test_buffer_columns_fid(tmp_path, suffix, geoops_module, testfile):
     output_layerinfo = fileops.get_layerinfo(output_path)
     output_gdf = fileops.read_file(output_path)
     assert output_gdf["geometry"][0] is not None
-    assert list(output_layerinfo.columns) == ["LblHfdTlt", "fid_1"]
-    assert len(output_gdf[output_gdf.fid_1 == multi_fid]) == 2
+    assert list(output_layerinfo.columns) == exp_columns
+    if "fid" in columns:
+        assert len(output_gdf[output_gdf.fid_1 == multi_fid]) == 2
 
 
 @pytest.mark.parametrize("geoops_module", GEOOPS_MODULES)
diff --git a/tests/test_geofileops_twolayers.py b/tests/test_geofileops_twolayers.py
index 62610f6..977be76 100644
--- a/tests/test_geofileops_twolayers.py
+++ b/tests/test_geofileops_twolayers.py
@@ -984,7 +984,13 @@ def test_intersection_self(tmp_path):
 
 @pytest.mark.parametrize("testfile", ["polygon-parcel"])
 @pytest.mark.parametrize("suffix", SUFFIXES_GEOOPS)
-def test_intersection_columns_fid(tmp_path, testfile, suffix):
+@pytest.mark.parametrize(
+    "input1_columns, input2_columns",
+    [(["lblhfdtlt", "fid"], ["naam", "FiD"]), ("lblhfdtlt", "naam")],
+)
+def test_intersection_columns_fid(
+    tmp_path, testfile, suffix, input1_columns, input2_columns
+):
     input1_path = test_helper.get_testfile(testfile, suffix=suffix)
     input2_path = test_helper.get_testfile("polygon-zone", suffix=suffix)
     input1_layerinfo = gfo.get_layerinfo(input1_path)
@@ -995,8 +1001,6 @@ def test_intersection_columns_fid(tmp_path, testfile, suffix):
         tmp_path / f"{input1_path.stem}_intersection_{input2_path.stem}{suffix}"
     )
     # Also check if fid casing is preserved in output
-    input1_columns = ["lblhfdtlt", "fid"]
-    input2_columns = ["naam", "FiD"]
     gfo.intersection(
         input1_path=input1_path,
         input2_path=input2_path,
@@ -1007,24 +1011,30 @@ def test_intersection_columns_fid(tmp_path, testfile, suffix):
         batchsize=batchsize,
     )
 
-    # Check if the tmp file is correctly created
+    # Check if the result file is correctly created
     assert output_path.exists()
     exp_spatial_index = GeofileInfo(output_path).default_spatial_index
     assert gfo.has_spatial_index(output_path) is exp_spatial_index
     output_layerinfo = gfo.get_layerinfo(output_path)
-    assert len(output_layerinfo.columns) == len(input1_columns) + len(input2_columns)
     assert output_layerinfo.geometrytype == GeometryType.MULTIPOLYGON
     assert output_layerinfo.featurecount == 30
 
+    exp_nb_columns = len(input1_columns) if isinstance(input1_columns, list) else 1
+    exp_nb_columns += len(input2_columns) if isinstance(input2_columns, list) else 1
+    assert len(output_layerinfo.columns) == exp_nb_columns
+
     # Check the contents of the result file
     output_gdf = gfo.read_file(output_path)
     assert output_gdf["geometry"][0] is not None
-    assert "l1_fid" in output_gdf.columns
-    assert "l2_FiD" in output_gdf.columns
-    if _geofileinfo.get_geofileinfo(input2_path).is_fid_zerobased:
-        assert sorted(output_gdf.l2_FiD.unique().tolist()) == [0, 1, 2, 3, 4]
-    else:
-        assert sorted(output_gdf.l2_FiD.unique().tolist()) == [1, 2, 3, 4, 5]
+
+    if "fid" in input1_columns:
+        assert "l1_fid" in output_gdf.columns
+    if "fid" in input2_columns:
+        assert "l2_FiD" in output_gdf.columns
+        if _geofileinfo.get_geofileinfo(input2_path).is_fid_zerobased:
+            assert sorted(output_gdf.l2_FiD.unique().tolist()) == [0, 1, 2, 3, 4]
+        else:
+            assert sorted(output_gdf.l2_FiD.unique().tolist()) == [1, 2, 3, 4, 5]
 
 
 @pytest.mark.parametrize(
@@ -1764,8 +1774,7 @@ def test_select_two_layers_select_star_fids_unique(tmp_path, suffix):
     assert output_path.exists()
     output_layerinfo = gfo.get_layerinfo(output_path)
     # 1 attribute column expected: layer2.fid is aliased
-    exp_nb_columns = 1
-    assert len(output_layerinfo.columns) == exp_nb_columns
+    assert len(output_layerinfo.columns) == 1
 
 
 def test_split(tmp_path):
@@ -1986,7 +1995,7 @@ def test_union(
         batchsize=batchsize,
     )
 
-    # Check if the tmp file is correctly created
+    # Check if the output file is correctly created
     assert output_path.exists()
     exp_spatial_index = GeofileInfo(output_path).default_spatial_index
     assert gfo.has_spatial_index(output_path) is exp_spatial_index
diff --git a/tests/test_geoops_dissolve.py b/tests/test_geoops_dissolve.py
index 17043e7..1a9d676 100644
--- a/tests/test_geoops_dissolve.py
+++ b/tests/test_geoops_dissolve.py
@@ -101,7 +101,8 @@ def test_dissolve_linestrings(
 
 @pytest.mark.parametrize("suffix", SUFFIXES_GEOOPS)
 @pytest.mark.parametrize("epsg", EPSGS)
-def test_dissolve_linestrings_groupby(tmp_path, suffix, epsg):
+@pytest.mark.parametrize("groupby_columns", [["NiScoDe"], "NiScoDe"])
+def test_dissolve_linestrings_groupby(tmp_path, suffix, epsg, groupby_columns):
     # Prepare test data
     input_path = test_helper.get_testfile(
         "linestring-watercourse", suffix=suffix, epsg=epsg
@@ -116,7 +117,6 @@ def test_dissolve_linestrings_groupby(tmp_path, suffix, epsg):
         output_basepath.parent
         / f"{output_basepath.stem}_groupby_noexpl{output_basepath.suffix}"
     )
-    groupby_columns = ["NiScoDe"]
     gfo.dissolve(
         input_path=str(input_path),
         output_path=str(output_path),
@@ -281,6 +281,7 @@ def test_dissolve_linestrings_aggcolumns_json(tmp_path, agg_columns):
     "expected_featurecount",
     [
         (".gpkg", 31370, False, ["GEWASgroep"], True, 0.0, "", 26),
+        (".gpkg", 31370, False, "GEWASgroep", True, 0.0, "", 26),
         (".gpkg", 31370, False, ["GEWASgroep"], True, 0.01, "", 25),
         (".gpkg", 31370, False, ["GEWASGROEP"], False, 0.0, "", 6),
         (".gpkg", 31370, True, ["GEWASGROEP"], False, 0.0, "", 6),
@@ -346,7 +347,10 @@ def test_dissolve_polygons(
     assert gfo.isvalid(output_path)
     output_layerinfo = gfo.get_layerinfo(output_path)
     assert output_layerinfo.featurecount == expected_featurecount
-    if groupby is True:
+
+    if groupby_columns is not None and isinstance(groupby_columns, str):
+        groupby_columns = [groupby_columns]
+    if groupby:
         # No groupby -> normally no columns.
         # Shapefile needs at least one column, if no columns: fid
         if suffix == ".shp":
diff --git a/tests/test_ogr_util.py b/tests/test_ogr_util.py
index e084f02..ae4da87 100644
--- a/tests/test_ogr_util.py
+++ b/tests/test_ogr_util.py
@@ -280,3 +280,22 @@ def test_vector_translate_sql_input_empty(tmp_path, input_suffix, output_suffix)
     input_layerinfo = gfo.get_layerinfo(input_path)
     output_layerinfo = gfo.get_layerinfo(output_path)
     assert len(input_layerinfo.columns) == len(output_layerinfo.columns)
+
+
+@pytest.mark.parametrize("input_suffix", test_helper.SUFFIXES_GEOOPS)
+@pytest.mark.parametrize("output_suffix", test_helper.SUFFIXES_GEOOPS)
+@pytest.mark.parametrize("columns", [["OIDN"], "OIDN"])
+def test_vector_translate_columns(tmp_path, input_suffix, output_suffix, columns):
+    # Prepare test data
+    input_path = test_helper.get_testfile(
+        "polygon-parcel", suffix=input_suffix, empty=True
+    )
+    output_path = tmp_path / f"output{output_suffix}"
+    exp_columns = columns if isinstance(columns, list) else [columns]
+
+    _ogr_util.vector_translate(input_path, output_path, columns=columns)
+
+    # Check result
+    output_layerinfo = gfo.get_layerinfo(output_path)
+    assert len(output_layerinfo.columns) == 1
+    assert list(output_layerinfo.columns) == exp_columns