Skip to content

PR: GDALG (#9) - #11

Closed
jimbrig wants to merge 9 commits into
developfrom
feature/gdalg
Closed

jimbrig wants to merge 9 commits into
developfrom
feature/gdalg

Conversation

@jimbrig

@jimbrig jimbrig commented Jul 2, 2026

Copy link
Copy Markdown
Owner

GDALG Support


Initial Commit:

For #9

This commit introduces core functionality for handling GDAL Algorithm (GDALG) definition files. It provides gdalg_read and gdalg_write functions to serialize and deserialize GDALG objects from/to JSON files.

Additionally, it adds gdalg_parse_command_line for tokenizing GDAL command strings and validate_gdalg_file / validate_gdalg to ensure GDALG objects and files conform to a defined schema. This enables programmatic definition, storage, and execution of GDAL algorithms.

…bjects (#9)

For #9

This commit introduces core functionality for handling GDAL Algorithm (GDALG) definition files. It provides `gdalg_read` and `gdalg_write` functions to serialize and deserialize GDALG objects from/to JSON files.

Additionally, it adds `gdalg_parse_command_line` for tokenizing GDAL command strings and `validate_gdalg_file` / `validate_gdalg` to ensure GDALG objects and files conform to a defined schema. This enables programmatic definition, storage, and execution of GDAL algorithms.
@jimbrig jimbrig self-assigned this Jul 2, 2026
@jimbrig jimbrig added the enhancement New feature or request label Jul 2, 2026
Comment thread R/gdal_gdalg.R
# ------------------------------------------------------------------------

# read write ------------------------------------------------------------------------------------------------------

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change

Comment thread R/gdal_gdalg.R
alg_cmd <- paste(command_line_parsed[[1]], command_line_parsed[[2]], sep = " ")
alg_args <- c(
command_line_parsed[-c(1, 2)],
"!", "write", "--output", path, "--output-format", "GDALG",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change
"!", "write", "--output", path, "--output-format", "GDALG",
"!",
"write",
"--output",
path,
"--output-format",
"GDALG",

Comment thread R/gdal_gdalg.R
Comment on lines +34 to +42
res <- rlang::try_fetch({
gdalg_alg <- gdalraster::gdal_alg(cmd = alg_cmd, alg_args, parse = FALSE)
gdalg_alg$run()
}, error = function(e) {
FALSE
}, finally = {
gdalg_alg$close()
gdalg_alg$release()
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change
res <- rlang::try_fetch({
gdalg_alg <- gdalraster::gdal_alg(cmd = alg_cmd, alg_args, parse = FALSE)
gdalg_alg$run()
}, error = function(e) {
FALSE
}, finally = {
gdalg_alg$close()
gdalg_alg$release()
})
res <- rlang::try_fetch(
{
gdalg_alg <- gdalraster::gdal_alg(cmd = alg_cmd, alg_args, parse = FALSE)
gdalg_alg$run()
},
error = function(e) {
FALSE
},
finally = {
gdalg_alg$close()
gdalg_alg$release()
}
)

Comment thread R/gdal_gdalg.R
check_names(x, required = c("type", "command_line", "gdal_version"))
type <- purrr::pluck(x, "type", .default = NA_character_)
if (!identical(type, "gdal_streamed_alg")) {
gdal_abort_check("Provided list must have {.field type} field equal to {.field \"gdal_streamed_alg\"}.", call = call)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change
gdal_abort_check("Provided list must have {.field type} field equal to {.field \"gdal_streamed_alg\"}.", call = call)
gdal_abort_check(
"Provided list must have {.field type} field equal to {.field \"gdal_streamed_alg\"}.",
call = call
)

Comment thread R/gdal_gdalg.R
# constructor -----------------------------------------------------------------------------------------------------

new_gdalg <- function(command_line, relative_paths = TRUE, gdal_version = gdal_version_num(), .path = NULL) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change

Comment thread R/gdal_gdalg.R
Comment on lines 179 to +180

gdalg_parse_command_line <- function(command_line) {
tokens <- strsplit(command_line, "(?<!\\\\)\\s+(?=(?:[^\"]*\"[^\"]*\")*[^\"]*$)", perl = TRUE)[[1]]
gsub("^\"|\"$", "", tokens)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change
gdalg_parse_command_line <- function(command_line) {
tokens <- strsplit(command_line, "(?<!\\\\)\\s+(?=(?:[^\"]*\"[^\"]*\")*[^\"]*$)", perl = TRUE)[[1]]
gsub("^\"|\"$", "", tokens)
}

Comment thread R/utils_checks.R
}
invisible(x)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change

Comment thread R/utils_json.R
#
# ------------------------------------------------------------------------


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change

Comment thread R/utils_json.R
gdal_abort_check(msg = "Provided {.arg x} is not a valid JSON file path or string", call = rlang::caller_env())
}


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change

Comment thread R/utils_json.R
Comment on lines +28 to +29


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change

These new GDALG definitions enable streamlined download and processing of TIGER/Line 2025 boundary data from the US Census Bureau. They include pipelines to filter out non-contiguous territories, select relevant attributes, ensure geometry validity, reproject to EPSG:4326, and sort for optimized spatial indexing.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements core read, write, parsing, and validation functionality for GDALG files, along with several new utility check functions for names, CRS, and GDALG classes. The review feedback highlights several critical issues: a potential crash in the finally block of gdalg_write() if initialization fails, an overly restrictive name check in as_gdalg.list() for the optional gdal_version field, a lack of defensive validation in gdalg_parse_command_line(), a stubbed implementation of validate_gdalg(), leftover commented-out code in check_file(), and a potential 'length zero' error in check_crs_epsg() when the EPSG is NULL.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread R/gdal_gdalg.R
Comment on lines +34 to +42
res <- rlang::try_fetch({
gdalg_alg <- gdalraster::gdal_alg(cmd = alg_cmd, alg_args, parse = FALSE)
gdalg_alg$run()
}, error = function(e) {
FALSE
}, finally = {
gdalg_alg$close()
gdalg_alg$release()
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If gdalraster::gdal_alg() fails to initialize and throws an error, the variable gdalg_alg will not be defined. When the finally block executes, calling gdalg_alg$close() will throw an "object 'gdalg_alg' not found" error, masking the original error and causing an unexpected crash.

Initialize gdalg_alg to NULL outside the try_fetch block, and check if it is non-NULL before calling $close() and $release().

  gdalg_alg <- NULL
  res <- rlang::try_fetch({
    gdalg_alg <<- gdalraster::gdal_alg(cmd = alg_cmd, alg_args, parse = FALSE)
    gdalg_alg$run()
  }, error = function(e) {
    FALSE
  }, finally = {
    if (!is.null(gdalg_alg)) {
      gdalg_alg$close()
      gdalg_alg$release()
    }
  })

Comment thread R/gdal_gdalg.R
# check_names(x, required = c("command_line", "gdal_version"))
# TODO
as_gdalg.list <- function(x, ..., .path = NULL, call = rlang::caller_env()) {
check_names(x, required = c("type", "command_line", "gdal_version"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The gdal_version field is optional in the GDALG schema (and has a fallback default on line 114). Requiring it in check_names() will cause validation to fail for valid GDALG lists that do not specify a version. Please remove "gdal_version" from the required names list.

  check_names(x, required = c("type", "command_line"))

Comment thread R/gdal_gdalg.R

# parse -----------------------------------------------------------------------------------------------------------

gdalg_parse_command_line <- function(command_line) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If command_line is NULL, NA, or not a character string, strsplit() will throw an error. Since new_gdalg() uses purrr::compact(), a NULL command_line will be omitted from the list, resulting in x$command_line being NULL when passed to gdalg_write().

Add a check_string(command_line) call at the beginning of gdalg_parse_command_line() to ensure defensive input validation.

gdalg_parse_command_line <- function(command_line) {
  check_string(command_line)

Comment thread R/gdal_gdalg.R
Comment on lines +156 to +158
validate_gdalg <- function(x, schema = pkg_sys_schemas("gdalg.schema.json"), ..., call = rlang::caller_env()) {
check_gdalg(x)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The validate_gdalg function is currently a stub that only calls check_gdalg(x) (which checks class inheritance). It does not perform any schema validation, despite accepting a schema argument. If the intent is to validate the in-memory GDALG object against the schema, you should serialize it to JSON (or use an appropriate list validator) and run the schema validation on it.

Comment thread R/utils_checks.R
Comment on lines +457 to +469
# ext <- gsub(".", "", ext, fixed = TRUE)
ext <- gsub("^\\.", "", ext)
# ext_parts <- strsplit(ext, ".", fixed = TRUE)[[1]]
# if (length(ext_parts) > 1L) {
# # for each ext_part, check, in order, it against the initial path, then the extension of the initial path sans ext, ...
# for (i in seq_along(ext_parts)) {
# ext_part <- ext_parts[i]
# if (tolower(tools::file_ext(x)) == ext_part) {
# return(invisible(x))
# }
# x <- sub(paste0("\\.", tools::file_ext(x), "$"), "", x)
# }
# }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a significant amount of commented-out code here. To maintain code cleanliness and readability, please remove these commented-out lines.

  ext <- gsub("^\\.", "", ext)

Comment thread R/utils_checks.R
Comment on lines +762 to +763
epsg <- sf::st_crs(x)$epsg
if (is.na(epsg) || epsg != expected_epsg) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If sf::st_crs(x)$epsg returns NULL or is empty, is.na(epsg) will evaluate to logical(0), causing the if condition to throw an "argument is of length zero" error.

Use a more defensive check such as is.null(epsg) || is.na(epsg) || epsg != expected_epsg to handle this safely.

  epsg <- sf::st_crs(x)$epsg
  if (is.null(epsg) || is.na(epsg) || epsg != expected_epsg) {

Comment thread R/utils_validation.R
Comment on lines +45 to +46


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[air] reported by reviewdog 🐶

Suggested change

jimbrig added 6 commits July 2, 2026 12:14
Captures research and design decisions for a future vector introspection layer, particularly detailing the complex nuances of Feature ID (FID) handling in GDAL. This document aims to prevent re-derivation of hard-won knowledge, guiding future implementation efforts.
…bjects (#9)

For #9

This commit introduces core functionality for handling GDAL Algorithm (GDALG) definition files. It provides `gdalg_read` and `gdalg_write` functions to serialize and deserialize GDALG objects from/to JSON files.

Additionally, it adds `gdalg_parse_command_line` for tokenizing GDAL command strings and `validate_gdalg_file` / `validate_gdalg` to ensure GDALG objects and files conform to a defined schema. This enables programmatic definition, storage, and execution of GDAL algorithms.
These new GDALG definitions enable streamlined download and processing of TIGER/Line 2025 boundary data from the US Census Bureau. They include pipelines to filter out non-contiguous territories, select relevant attributes, ensure geometry validity, reproject to EPSG:4326, and sort for optimized spatial indexing.
@jimbrig jimbrig closed this Jul 3, 2026
@jimbrig
jimbrig deleted the feature/gdalg branch July 3, 2026 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant