raster: Restrict GDAL-linked raster reads to the needed column range

[ninsbl]

read_data_gdal() - used when reading a linked-raster maps (r.external) - always reads the full native row width from the GDAL dataset, regardless of the current region.
For a raster map that is much wider than the region - e.g. a country-wide mosaic VRT built from many source tiles, this can have significant impact on the processing time. Because every row read touches every source tile across the full width, even when the region only overlaps one or two of them. The consequence is significantly degraded performance for such cases, esp. if several maps are read simultaneously e.g. in a mapcalculator expression and if data is stored on file systems with higher latency (e.g. NAS with CIFS mount).

This PR restricts the GDAL read to the native column range actually needed. It is computed from the column mapping GRASS already builds once per file open. The behavior for hflip'ed datasets is unchanged and falls back to the original full-width read.

r.in.gdal and r.external are unaffected by the changes. OpenMP parallelized tools are not affected by the change either. I do have a regression-test for that in my local working copy and can push it to the PR if it seems useful. And I am happy to remove the r.in.gdal test if that is not needed (I just wanted to make sure all possible caveats are covered).

The PR includes a pytest covering cases where the computaional region is fully inside, partially outside, and fully outside the linked file's extent. THere is also one test to verify r.in.gdal is unaffected.

Given the significant performance degradation in some cases I take the liberty to classify this as a blocker. May be also a reason to backport...

Generated with the help of AI with lots of manual debugging and verifications...

[echoix]

I don't understand the C code changes enough to approve or reject

[ninsbl]

I don't understand the C code changes enough to approve or reject

Thanks for looking into it anyway!

[ninsbl]

If someone with knowledge of this area of the code could review this PR, I would be really glad. I am aware of time constraints of course and I would not ask if not this would be a major issue (for me). It grinds one of my use-cases for GRASS practically to a halt, because stuff that should take less thqan half a minute can take more than two hours...

So, this PR would increase the performance of GRASS for my case up to more than x250-300 (depending on the input dataset and file system of course)...

I assume it will also significantly improve usage of GRASS as processing backend in QGIS in many cases...

[echoix]

Well, it doesn’t look wrong, and you’ll be the first to change it back if it doesn’t work, so here you go.

I was hoping on someone that would have known if something was missing or could be done in a better way if it existed.

[ninsbl]

Well, it doesn’t look wrong, and you’ll be the first to change it back if it doesn’t work, so here you go.

I was hoping on someone that would have known if something was missing or could be done in a better way if it existed.

Thanks! I addressed your comments and added the mentioned tests...

[nilason]

The code changes looks reasonable to me, but I’m not totally comfortable with this part of the code, let us give @metzm a chance to chime in.

[ninsbl]

@echoix just to make sure: Your AI review referenced only tests outside of this PR. Are the findings related to the changes here or to the (relativly new) ctypes based tests?

[echoix]

@echoix just to make sure: Your AI review referenced only tests outside of this PR. Are the findings related to the changes here or to the (relativly new) ctypes based tests?

Oh, did I mistype the PR number ? I didn't caught on since it was the same topics and touching bounds. Let me retry something. Or you could on your side too.

[echoix]

@echoix just to make sure: Your AI review referenced only tests outside of this PR. Are the findings related to the changes here or to the (relativly new) ctypes based tests?

Oh, did I mistype the PR number ? I didn't caught on since it was the same topics and touching bounds. Let me retry something. Or you could on your side too.

Indeed, it's for #7879

[echoix]

AI-assisted review — Claude Code (Opus 5)

No correctness bugs found. Reviewed 775c8eb against origin/main (4 files, +267/−11): the narrowed GDALRasterIO window is index-consistent with every consumer of fcb->data, and the cached column range cannot go stale because Rast_set_input_window() already fatal-errors while any map is open for read. Two things worth a look: the -1 sentinel for gdal_min_col/gdal_max_col is not what an fd actually defaults to (new_fileinfo() zeroes the struct, so a skipped mapping reads as the valid range "column 0 only"), and nothing in the new tests exercises an hflip/vflip link — the one branch the change deliberately special-cases.

Full review

What was verified

The change is only safe if a handful of invariants hold, so each was checked against the source rather than by eye:

• Buffer layout stays consistent. read_data_gdal() writes native column c at data_buf + c * cur_nbytes (absolute, not relative to col_off), which is exactly where gdal_values_int/float/double() read it via col_map[i] - 1 (get_row.c:365,412,438). Every consumer of fcb->data, get_null_value_row_gdal() included, goes through col_map, so untouched columns are never read.
• Bounds. col_map entries are clamped to [0, cellhd.cols] in Rast__create_window_mapping(), so gdal_min_col/gdal_max_col land in [0, cols-1]; the write range fits the G_calloc(cellhd.cols, MAP_NBYTES) buffer at open.c:337. COLUMN_MAPPING is typedef int (R.h:16), so the -1 sentinel and the >= 0 tests are signed-safe.
• The cached range cannot go stale. Rast__create_window_mapping() is called from open.c:327, after fcb->gdal and fcb->cellhd are set and while open_mode == -1, so the write-mode early return does not fire. Rast_set_window() / Rast_set_input_window() fatal-error in update_window_mappings() if any map is open for read, so the cached min/max has exactly the same lifetime as col_map itself.
• Fallbacks. No overlap gives gdal_min_col == -1 and a full-width read. hflip is excluded in both places, and with ncols == cellhd.cols the rewritten flip loop is byte-for-byte the old one. VRT maps skip the mapping, but get_map_row_nomask() returns early on fcb->vrt before read_data(), and get_row.c:595 is the only read_data() call site.
• cur_nbytes is current at the new pointer arithmetic: *nbytes = fcb->nbytes runs first and nbytes aliases &fcb->cur_nbytes at the sole call site. Fragile aliasing, but the pre-existing hflip G_malloc already depended on it.
• The parallel tests are meaningful, not racy. r.mapcalc opens a separate fd per thread (raster/r.mapcalc/map.c:581, keyed on thread_num) and Rast_get_gdal_link() does its own GDALOpen per fd (gdal.c:137), so no GDALRasterBandH is shared across threads.
• Test math. All six region cases were hand-checked against the C mapping (C2 accumulation, the negative-rounding x--, and the lat/lon wraparound second pass). The antimeridian case resolves to native columns 350..359, 0..9, matching the helper.

Findings

• lib/raster/get_row.c:213 / lib/raster/open.c:34 — the -1 sentinel is not the actual default. new_fileinfo() memsets the struct to zero, so any fd where Rast__create_window_mapping() does not run leaves gdal_min_col == gdal_max_col == 0, which reads as the valid range "native column 0 only" rather than "unknown". Today the only such fds are VRT and write-mode maps, neither of which reaches read_data_gdal(), so there is no failure now. But a future read path that skips the mapping would silently read one column per row instead of the full width, producing plausible-looking wrong data rather than crashing. Initializing both to -1 in new_fileinfo(), or at the top of Rast__create_window_mapping() before its early return, would make the sentinel self-defending.
• lib/raster/window_map.c:110 — no coverage for the branch the change special-cases. hflip is excluded in two places and vflip remaps the row, yet no test creates a flipped link. A vflip link read under a restricted region is cheap insurance and covers the one interaction (col_off offsetting plus row inversion) that is currently untested.
• lib/raster/tests/lib_raster_gdal_link_window_test.py:180 — test_r_in_gdal_ignores_region does not touch the changed code. r.in.gdal imports into native GRASS format and never goes through read_data_gdal(), so this asserts library behavior unrelated to the PR in a file whose docstring scopes it to windowed GDAL-linked reads.
• lib/raster/tests/lib_raster_gdal_link_window_test.py:220 — double space in the docstring ("r.univar with  linked data"), which AGENTS.md and doc/development/style_guide.md disallow.

Notes, not findings

• R.h reflows the comment alignment on three untouched lines (reclass_flag, row_ptr, col_map) — clang-format fallout from the longer new field names, expected.
• The new module-level session fixture shadows the one in lib/raster/tests/conftest.py. Legal pytest and module-local, but worth knowing it is intentional.
• Unrelated pre-existing issue noticed while checking the stride assumption: raster/r.external/link.c:34 maps GDT_Int8 to cellhd->format = 1 (2 bytes/cell) while GDAL packs Int8 at 1 byte, so Int8 links are already misread. Not this PR's to fix, and the PR neither introduces nor worsens it.

Method and limits

Static review only — GRASS was not built and the new test file was not executed. The review was run by a human-directed AI agent; all findings above were traced to the cited source lines by hand.

---

Generated by Claude Code