Fielddomain

[edzer]

Adds basic functionality of reading (some of the) field domain information from data sources; see #1640

[Copilot]

Pull request overview

Adds support for exposing GDAL/OGR field domain metadata through st_read() outputs, enabling consumers to inspect per-field domain names and (when supported) dataset-level domain definitions.

Changes:

• Attach a per-field domains attribute when building the intermediate R list from an OGR layer.
• (GDAL >= 3.5) Extract dataset field-domain definitions and attach them as a FieldDomains attribute.
• Preserve/propagate these attributes through process_cpl_read_ogr() into the final sf object (and explicitly NULL them for non-geometry returns).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/gdal_read.cpp	Captures field domain names during layer read and (GDAL >= 3.5) collects field-domain definitions from the dataset.
R/read.R	Propagates domains / FieldDomains attributes onto the returned sf (and clears them for non-geometry returns).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

poFieldDefn->GetDomainName() is compared to "" using !=, which for const char* does a pointer comparison rather than checking string content. This can incorrectly treat empty domain names as present (or miss non-empty ones, depending on implementation). Please check for non-empty content explicitly (e.g., store the returned pointer and test dn && dn[0] != '\0', or convert to std::string and use .empty()).

Suggested change

	if (poFieldDefn->GetDomainName() != "")
	domains[i] = poFieldDefn->GetDomainName();
	const char *domain_name = poFieldDefn->GetDomainName();
	if (domain_name != NULL && domain_name[0] != '\0')
	domains[i] = domain_name;

[Copilot]

get_field_domains() only handles a subset of OGRFieldDomainMergePolicy/SplitPolicy enum values. For any other value, the LogicalVector defaults to FALSE, which will silently misrepresent the policy. Please add a final else branch for both merge and split policies to set NA_LOGICAL (or error) for unhandled enum values to avoid incorrect metadata.

[Copilot]

New behavior attaches domains / FieldDomains attributes to the returned object, but there are no tests asserting the presence/absence and basic structure of these attributes (including the no-geometry branch). Please add a test (likely in tests/testthat/test-read.R) that verifies the attributes are NULL when no domains exist, and (when GDAL supports it and test data is available) that non-empty domains are propagated consistently.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This test unconditionally expects FieldDomains/domains attributes, but the implementation only populates FieldDomains when compiled with GDAL >= 3.5.0 (and domains requires at least one non-NA domain name, which depends on GDAL/driver support). Please gate the expectations with skip_if_not(sf_extSoftVersion()[['GDAL']] >= '3.5.0') (and/or make the assertions conditional) so the test suite passes on builds using older GDAL versions.

Suggested change

	r = read_sf(system.file("gpkg/td.gpkg.zip", package = "sf"), options=c("IMMUTABLE=YES"))
	expect_false(is.null(attr(r, "FieldDomains")))
	expect_false(is.null(attr(r, "domains")))
	skip_if_not(sf_extSoftVersion()[["GDAL"]] >= "3.5.0")
	r = read_sf(system.file("gpkg/td.gpkg.zip", package = "sf"), options=c("IMMUTABLE=YES"))
	field_domains = attr(r, "FieldDomains")
	expect_false(is.null(field_domains))
	field_domain_names = names(field_domains)
	if (!is.null(field_domain_names) && any(!is.na(field_domain_names) & nzchar(field_domain_names))) {
	expect_false(is.null(attr(r, "domains")))
	}