Skip to content

Commit c4046f7

Browse files
committed
Fix missing parentId and ancestor filters in paginateSubRows
df backend: Sub-group headers were missing parentId in their __state. Add parentId to sub-group header state when depth > 0. DuckDB backend: duckdbCollectPageRows only included the current level's filter when querying leaf sub-rows, missing ancestor filters. For multi-level groupBy, this returned wrong rows from other groups sharing the same sub-group value. Thread parentFilters through recursive calls to accumulate ancestor filter clauses at each level. Add test assertions for both fixes.
1 parent a70b6f6 commit c4046f7

4 files changed

Lines changed: 20 additions & 4 deletions

File tree

‎R/backend-df.R‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,9 @@ dfCollectPageRows <- function(groupedDf, flatSizes, pageStart, pageEnd, groupBy,
303303
grouped = TRUE,
304304
subRowCount = subRowCount
305305
)
306+
if (!is.null(parentId)) {
307+
headerRow[["__state"]]$parentId <- parentId
308+
}
306309
rows <- c(rows, list(headerRow))
307310
}
308311

‎R/backend-duckdb.R‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,8 @@ duckdbFlatSizes <- function(groupTree, expandedMap, groupBy, depth, parentId) {
465465
# Collect rows that fall within [pageStart, pageEnd) from the flattened group stream.
466466
duckdbCollectPageRows <- function(con, groupTree, flatSizes, pageStart, pageEnd,
467467
groupBy, baseWhere, sortBy, columns,
468-
depth, parentId, expandedMap) {
468+
depth, parentId, expandedMap,
469+
parentFilters = list(clauses = character(0), params = list())) {
469470
groupCol <- groupBy[[depth + 1]]
470471
isLeafLevel <- depth + 1 >= length(groupBy)
471472
escapedGroupCol <- duckdbQuoteIdentifier(groupCol)
@@ -528,8 +529,8 @@ duckdbCollectPageRows <- function(con, groupTree, flatSizes, pageStart, pageEnd,
528529

529530
if (subSliceEnd > subSliceStart) {
530531
childFilters <- list(
531-
clauses = paste0(escapedGroupCol, " = ?"),
532-
params = list(groupValue)
532+
clauses = c(parentFilters$clauses, paste0(escapedGroupCol, " = ?")),
533+
params = c(parentFilters$params, list(groupValue))
533534
)
534535
subQuery <- buildDuckdbSubRowSql("reactable_data", baseWhere, childFilters, sortBy)
535536
# Add LIMIT/OFFSET to the sub-row query
@@ -557,11 +558,16 @@ duckdbCollectPageRows <- function(con, groupTree, flatSizes, pageStart, pageEnd,
557558
depth = depth + 1, parentId = groupId)
558559
childPageStart <- pageStart - (nodeStart + 1L)
559560
childPageEnd <- pageEnd - (nodeStart + 1L)
561+
levelFilters <- list(
562+
clauses = c(parentFilters$clauses, paste0(escapedGroupCol, " = ?")),
563+
params = c(parentFilters$params, list(groupValue))
564+
)
560565
childRows <- duckdbCollectPageRows(con, childTree, childSizes,
561566
childPageStart, childPageEnd,
562567
groupBy, baseWhere, sortBy, columns,
563568
depth = depth + 1, parentId = groupId,
564-
expandedMap = expandedMap)
569+
expandedMap = expandedMap,
570+
parentFilters = levelFilters)
565571
if (!is.null(childRows) && nrow(childRows) > 0) {
566572
rows <- c(rows, list(childRows))
567573
}

‎tests/testthat/test-backend-df.R‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -897,6 +897,9 @@ test_that("dfPaginateSubRows multi-level with both levels expanded", {
897897
expect_equal(result$data[["__state"]]$id,
898898
c("mfr:Acura", "mfr:Acura.type:Small", "0",
899899
"mfr:Acura.type:Midsize", "mfr:Audi"))
900+
# Sub-group headers have parentId
901+
expect_equal(result$data[["__state"]]$parentId[2], "mfr:Acura")
902+
expect_equal(result$data[["__state"]]$parentId[4], "mfr:Acura")
900903
# Leaf sub-row has parentId
901904
expect_equal(result$data[["__state"]]$parentId[3], "mfr:Acura.type:Small")
902905
expect_equal(result$data$model[3], "Integra")

‎tests/testthat/test-backend-duckdb.R‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,10 @@ test_that("backendDuckdb - paginateSubRows multi-level both levels expanded", {
839839
# Leaf rows should have parentId pointing to their sub-group
840840
leafRows <- which(!is.na(result$data$`__state`$index))
841841
expect_true(all(result$data$`__state`$parentId[leafRows] == "mfr:Acura.type:Small"))
842+
843+
# Leaf rows should only be from Acura's Small group (Integra, Legend), not Audi's
844+
leafModels <- sort(result$data$model[leafRows])
845+
expect_equal(leafModels, c("Integra", "Legend"))
842846
})
843847

844848
test_that("backendDuckdb - paginateSubRows multi-level page boundary inside expanded children", {

0 commit comments

Comments
 (0)