Fix frs_order_child SQL: drop nonexistent stream_order_max column

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: fresh
 Title: Freshwater Referenced Spatial Hydrology
-Version: 0.27.1
+Version: 0.27.2
 Authors@R: c(
     person("Allan", "Irvine", , "al@newgraphenvironment.com", role = c("aut", "cre"),
            comment = c(ORCID = "0000-0002-3495-2128")),

NEWS.md

@@ -1,3 +1,9 @@
+# fresh 0.27.2
+
+Fix `frs_order_child()` SQL referencing nonexistent column `s.stream_order_max`. `fresh.streams` has `stream_order` and `stream_order_parent` from FWA but no `stream_order_max` — the 0.27.0 SQL failed at execution against any real database (only the SQL-shape unit tests passed, and they asserted the broken predicate). Drop the broken predicate; default both `child_order_min` and `child_order_max` to `1L` when neither is set, which matches bcfishpass's hardcoded `stream_order = 1` predicate exactly. Caller-passed bounds still apply unchanged.
+
+Adds a regression test that scans the emitted SQL across every parameter combination for `stream_order_max` and fails if it reappears.
+
 # fresh 0.27.1
 
 Patch follow-up to 0.27.0. `.frs_load_rules` validator now accepts the `channel_width_min_bypass` predicate that link emits to drive `frs_order_child()` post-classify (link#96 wiring). Validates that the field is a named mapping with `stream_order` and `stream_order_parent_min` integer scalars. Without this patch, link's emitted rules.yaml fails fast at `frs_params()` with `unknown predicates: channel_width_min_bypass` before any classification runs.

R/frs_order_child.R

@@ -27,19 +27,21 @@
 #'
 #' @section Direct-child semantics:
 #'
-#' A "direct child" of a large river is a segment whose order matches
-#' the highest order on its blue_line_key — i.e., it flows directly
-#' into the parent river, not as a sub-tributary inside a multi-order
-#' network:
+#' A "direct child" of a large river is a segment whose stream_order
+#' matches the caller's child-order filter (`child_order_min` /
+#' `child_order_max`) AND whose `stream_order_parent` (the order of
+#' the river it flows into) is `>= parent_order_min`:
 #'
 #' ```
-#' s.stream_order = s.stream_order_max  -- (on the same blue_line_key)
 #' s.stream_order_parent >= parent_order_min
+#' [AND s.stream_order >= child_order_min]
+#' [AND s.stream_order <= child_order_max]
 #' ```
 #'
-#' The `stream_order = stream_order_max` test implicitly captures
-#' "stop at order change" — once the segment's order would exceed
-#' `stream_order_max`, you're no longer on the direct-child reach.
+#' Default child filter is `stream_order = 1` (matches bcfishpass's
+#' hard-coded predicate exactly). Pass `child_order_min` / `child_order_max`
+#' to widen — e.g. `child_order_min = 1, child_order_max = 2` for
+#' order-1 and order-2 tributaries.
 #'
 #' @section Distance grain:
 #'
@@ -60,10 +62,9 @@
 #'   AND <habitat>.species_code = '<species>'
 #'   AND <habitat>.accessible = TRUE
 #'   AND <habitat>.<label> IS NOT TRUE
-#'   AND s.stream_order = s.stream_order_max
 #'   AND s.stream_order_parent >= <parent_order_min>
-#'   [AND s.stream_order >= <child_order_min>]
-#'   [AND s.stream_order <= <child_order_max>]
+#'   AND s.stream_order >= <child_order_min>   -- default 1 if both NULL
+#'   AND s.stream_order <= <child_order_max>   -- default 1 if both NULL
 #'   [AND s.downstream_route_measure <= <distance_max>]
 #' ```
 #'
@@ -84,11 +85,13 @@
 #'   direct-child segment to qualify. Default `5L` (matches
 #'   bcfishpass's hard-coded value).
 #' @param child_order_min Integer or `NULL`. If set, segment's
-#'   `stream_order` must be `>= child_order_min`. Default `NULL` (no
-#'   floor — any order accepted).
+#'   `stream_order` must be `>= child_order_min`. Default `NULL`. When
+#'   both `child_order_min` and `child_order_max` are `NULL`, both
+#'   default to `1L` (matches bcfishpass's `stream_order = 1` predicate).
 #' @param child_order_max Integer or `NULL`. If set, segment's
-#'   `stream_order` must be `<= child_order_max`. Default `NULL`
-#'   (capped only by `stream_order_max` per the direct-child filter).
+#'   `stream_order` must be `<= child_order_max`. Default `NULL`. When
+#'   both `child_order_min` and `child_order_max` are `NULL`, both
+#'   default to `1L` (matches bcfishpass's `stream_order = 1` predicate).
 #' @param distance_max Numeric or `NULL`. If set, segment's
 #'   `downstream_route_measure` must be `<= distance_max` (metres from
 #'   tributary mouth). Default `NULL` (whole tributary).
@@ -140,6 +143,14 @@ frs_order_child <- function(conn,
   sp_quoted <- .frs_quote_string(species)
   pom <- as.integer(parent_order_min)
 
+  # Default both child bounds to 1L when neither is set — matches
+  # bcfishpass's `stream_order = 1` predicate exactly. Caller can pass
+  # one or both to widen (e.g. orders 1-2) or narrow.
+  if (is.null(child_order_min) && is.null(child_order_max)) {
+    child_order_min <- 1L
+    child_order_max <- 1L
+  }
+
   child_min_clause <- if (!is.null(child_order_min)) {
     sprintf("AND s.stream_order >= %d", as.integer(child_order_min))
   } else ""
@@ -159,7 +170,6 @@ frs_order_child <- function(conn,
        AND h.species_code = %s
        AND h.accessible = TRUE
        AND h.%s IS NOT TRUE
-       AND s.stream_order = s.stream_order_max
        AND s.stream_order_parent >= %d
        %s %s %s",
     habitat, label,

tests/testthat/test-frs_order_child.R

@@ -48,13 +48,19 @@ test_that("frs_order_child default emits canonical bcfishpass-parity SQL", {
   expect_match(s, "h\\.accessible = TRUE")
   expect_match(s, "h\\.rearing IS NOT TRUE")
 
-  # Direct-child predicate
-  expect_match(s, "s\\.stream_order = s\\.stream_order_max")
+  # bcfp-parity defaults: child stream_order = 1 (both bounds default
+  # to 1L when caller passes neither — matches bcfp's hardcoded
+  # `stream_order = 1` predicate).
+  expect_match(s, "s\\.stream_order >= 1")
+  expect_match(s, "s\\.stream_order <= 1")
   expect_match(s, "s\\.stream_order_parent >= 5")
 
-  # Optional clauses NOT present at default
-  expect_no_match(s, "s\\.stream_order >= ")
-  expect_no_match(s, "s\\.stream_order <= ")
+  # No nonexistent column reference: stream_order_max is NOT a column
+  # on fresh.streams. The original 0.27.0 SQL referenced it and broke
+  # at execution.
+  expect_no_match(s, "stream_order_max")
+
+  # Distance clause not present at default
   expect_no_match(s, "downstream_route_measure")
 })
 
@@ -64,8 +70,8 @@ test_that("frs_order_child emits child_order_min/max bounds when set", {
 
   expect_match(s, "s\\.stream_order >= 2")
   expect_match(s, "s\\.stream_order <= 4")
-  # Direct-child predicate still present (orthogonal)
-  expect_match(s, "s\\.stream_order = s\\.stream_order_max")
+  # No nonexistent column reference
+  expect_no_match(s, "stream_order_max")
 })
 
 test_that("frs_order_child emits distance_max clause when set", {
@@ -74,6 +80,26 @@ test_that("frs_order_child emits distance_max clause when set", {
   expect_match(s, "s\\.downstream_route_measure <= 300")
 })
 
+test_that("frs_order_child SQL never references stream_order_max", {
+  # 0.27.0 docstring + SQL referenced s.stream_order_max which is not
+  # a column on fresh.streams (only stream_order, stream_order_parent
+  # exist). Execution failed at the first call site (link's HORS
+  # pre-flight). Defend against re-introducing the broken reference
+  # in any code path.
+  for (args in list(
+    list(),
+    list(child_order_min = 1L),
+    list(child_order_max = 3L),
+    list(child_order_min = 1L, child_order_max = 2L),
+    list(distance_max = 500),
+    list(parent_order_min = 7L),
+    list(label = "lake_rearing"))) {
+    sql <- do.call(.run_order_child, args)
+    expect_no_match(sql[1], "stream_order_max",
+      info = paste("args:", paste(names(args), collapse = ",")))
+  }
+})
+
 test_that("frs_order_child handles parent_order_min override", {
   sql_log <- .run_order_child(parent_order_min = 7L)
   s <- sql_log[1]