Skip to content

Support auxiliary hydrofabric attribute tables as source for catchment attributes - #1017

Draft
robertbartel wants to merge 14 commits into
NOAA-OWP:masterfrom
robertbartel:f/gpkg-aux-attribute-tables/main
Draft

Support auxiliary hydrofabric attribute tables as source for catchment attributes#1017
robertbartel wants to merge 14 commits into
NOAA-OWP:masterfrom
robertbartel:f/gpkg-aux-attribute-tables/main

Conversation

@robertbartel

Copy link
Copy Markdown
Contributor

Some hydrofabric files (or, supplemental files) may be created that contain per-divide model params for different formulations (e.g., from regionalization tools). These may be in tables named something other than divide-attributes - the previous standard - or even multiple tables. This PR adds configuration and functional capabilities to join data from these auxiliary tables with catchment features data from divides table. This then makes the auxiliary data available to the existing mechanisms for setting formulation module parameter values via the realization config; i.e., with the model_params "source": "hydrofabric" syntax. For example:

"model_params": {
    "bexp": { "source": "hydrofabric", "from": "reg.b" }
}

The PR introduces a new, optional, top-level key for the realization config: auxiliary_hydrofabric_attributes. It can be used as follows:

"auxiliary_hydrofabric_attributes": [                     
    {
        "table": "cfexnom__v___regn_jul26_cfexnom_clust_test2_gower_agglomerative_k15",
        "alias": "reg",              // optional; prefix defaults to the table name
        "file": "path/to/other.gpkg",// optional; defaults to the CLI catchment data file
        "key_column": "divide_id",   // optional; this is the default used to join table to `divides`
        "required": false            // optional; this is the default
    }
]

Tables are expected to be in the catchment data hydrofabric file provided on the command line, but optionally another file can be provided via the config.

A configured table must exist. Rows only must exist if "required": true within that table's entry in "auxiliary_hydrofabric_attributes"; if false, params will be skipped for missing-row catchments.

…les work

Set up an isolated serial build environment for the auxiliary GeoPackage
attribute tables effort: worktree-local submodules, a dedicated task
virtualenv, and a dedicated CMake build directory mirroring the main
serial build's feature flags.

No repository sources are changed. Baseline verified before any feature
work: test_unit passes (146 passed, 2 non-MPI skips) and test_geopackage
passes (11 passed).
example_aux.gpkg copies example.gpkg and adds two attributes tables: one
on the default divide_id key with a column per supported type, a NULL
cell, a divide with no row, and a row for a divide absent from the layer;
the other on a non-default key with full coverage and column names shared
with the first. Ships the derivation SQL, which *.sql no longer ignores
under test/data.
Add a realization config value type and parser for the optional top-level
auxiliary_hydrofabric_attributes key: a list of attribute tables to join onto
the catchment features, each with a table name, optional alias, file and key
column, and a required flag. Effective prefixes (alias, else table name) must
be unique across entries so joined columns cannot collide.

The module is free of file and database I/O; reading the tables is the
joiner's job.
Add ngen::geopackage::join_attributes, which reads a GeoPackage attribute
table and publishes each row's columns onto the matching feature as
<prefix>.<column> properties. An absent table or key column is an error;
a feature without a row warns, or errors when the table is required.

The reader's SQLite-to-property conversion moves into the ngen::geopackage
namespace so the join shares it rather than restating the type mapping.
Read the auxiliary_hydrofabric_attributes key from the realization config and
apply each declared table to the catchment collection before the formulations
are built, so joined columns resolve like any other hydrofabric property.

Entries without a 'file' read from the catchment data file given on the command
line, which must then be a GeoPackage. Declaring tables in a build without
SQLite support is a runtime error rather than a silent no-op.
…_params

Cover the path from a GeoPackage auxiliary attribute table to a BMI module's
parameters: a namespaced 'from' resolves to the joined column, and a divide the
table has no row for warns and leaves the parameter unset.
Describe the optional top-level key: each entry field and its default, the
<prefix>.<column> namespacing rule joined columns land under, the per-entry
strictness behavior, and the separate-file option. Includes a worked example
pairing an aliased table declaration with model_params entries that read from
it, plus the MPI and error-condition notes.
Final verification pass over the auxiliary attribute tables branch: clean
rebuild with test_unit, test_geopackage and test_realization_config all
passing, and an unmodified realization config still running unchanged.
A joined value could previously be lost or invented without a word: a
composed property name already on the feature was dropped, a second row
keyed the same won by scan order, a BLOB cell became the string "null",
and only the first feature of a shared id was joined at all.

Fixture gains a BLOB table and a duplicate-keyed table to cover these.
…EN_QUIET

A failed open of an auxiliary GeoPackage now names the file and the table
instead of a bare SQLite result code, and the driver adds the config entry
that asked for it. The missing-row warning goes through logging::warning so
a quiet build silences it.

read.cpp's check_table_name becomes ngen::geopackage::quote_table_name,
validating and quoting in one place for both the read and the join path;
this restores the sqlite_ internal-table rejection on the join path and
quotes the read path's interpolated layer name.
Guard the quiet-build assertions with `#if !NGEN_QUIET` rather than
`#ifndef`: NGenConfig.h defines the macro as 0 in normal builds, so the
`#ifndef` form skipped the assertions it was meant to protect. Release
stderr captures through a scope guard so a throwing test cannot abort the
binary, and bounds-check the fixture-drift column lookup.
Spell out the auto locals the review flagged in the aux-attribute join,
its config parser, and the driver wiring; rename the two Formulation_Manager
cases onto the file's short-suffix pattern; and cover
ngen::geopackage::get_property directly, including its default arm.
database::iterator::find returns -1 for a column that is not there, and
types() hands back a span whose operator[] is unchecked. Three assertions
fed that -1 straight into the span, so a fixture that drifted would have
been undefined behaviour rather than a reported failure -- and could have
passed, since SQLITE_BLOB is a small integer.

Extends the guard already applied to one such site to the ones that share
the pattern.
@robertbartel robertbartel added the enhancement New feature or request label Aug 20, 2026
@aaraney

aaraney commented Aug 20, 2026

Copy link
Copy Markdown
Member

This is something i've wanted for a while! This is great!

Having just glanced at the semantics, I was a little confused that the source remains hydrofabric when pulling from an auxiliary. What was your thinking in keeping source: hydrofabric over source: reg / the full table name?

@robertbartel

Copy link
Copy Markdown
Contributor Author

The design works by joining the aux data to divide data from the hydrofabric. So, we are appending something to the data already exposed to things when source: hydrofabric is used. In that sense, it isn’t a different source.

At a higher level, I was anticipating the/a common use case would be for the aux tables to actually be included in the hydrofabric file (largely because I was working with an example file where this was the case). In that situation also, source: hydrofabric seemed natural and appropriate.

@robertbartel
robertbartel marked this pull request as draft August 20, 2026 17:57
@robertbartel

Copy link
Copy Markdown
Contributor Author

Converting to draft for the moment. The design of #960 (and potentially further refinements to it) will likely impact the details of this significantly. Some initial review is fine, but full formal review should probably wait until #960 is settled.

@PhilMiller

Copy link
Copy Markdown
Contributor

I haven't looked at the code yet, but from the description, am I right to understand that these values must still be in the same file as the hydrofabric itself? I think we can pretty clearly see to a future where that won't be desirable - Hydrofabric v.N is finalized, and then we're doing various runs to generate auxiliary parameter values associated with it. We should expect to reference the hydrofabric from a read-only file, and load other data that references it from elsewhere (e.g. another SQLite file).

@PhilMiller

Copy link
Copy Markdown
Contributor

That "multiple SQLite file" thing would even still allow full database query functionality:
https://stackoverflow.com/questions/6824717/sqlite-how-do-you-join-tables-from-different-databases

@PhilMiller

Copy link
Copy Markdown
Contributor

Oops, I'm apparently illiterate today. Your description showed a "file: other.gpkg" line, and I totally missed it.

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.

3 participants