diff --git a/browse/fixture_test.go b/browse/fixture_test.go index 3d25b6288ff4172f811dda481a343771b72cef87..e3f98990ac4765390a10f40e97478052df7ebc75 100644 --- a/browse/fixture_test.go +++ b/browse/fixture_test.go @@ -149,13 +149,21 @@ {"branch", "dev"}, // dev head = C2 {"sql", "-q", "update users set name='alicia' where id=1; insert into users values (4,'dave')"}, {"commit", "-Am", msgModify}, // C4 adds the empty items table and, in the same commit, a docs table - // carrying one >4KB longtext row — the out-of-line-text fixture. Folding - // it into C4 keeps the commit topology (and every commit-count assertion) - // unchanged; TestTables looks tables up by name, so the extra table is - // invisible to it while TestRowsResolvesLongText reads docs directly. + // carrying one >4KB longtext row — the out-of-line-text fixture — plus a + // nulls table mixing real NULLs with the stored text "NULL". Folding them + // into C4 keeps the commit topology (and every commit-count assertion) + // unchanged; TestTables looks tables up by name, so the extra tables are + // invisible to it while TestRowsResolvesLongText reads docs and the + // null-mask tests read nulls directly. {"sql", "-q", "create table items (sku varchar(16) primary key, qty int)"}, {"sql", "-q", "create table docs (id int primary key, body longtext)"}, {"sql", "-q", fmt.Sprintf("insert into docs values (1, '%s')", longDocBody)}, + {"sql", "-q", "create table nulls (id int primary key, note varchar(64))"}, + // id=2 has no value at all; id=3 stores the four characters N,U,L,L. + // Both render as "NULL", so only the mask can tell them apart. Six rows + // so a page can start in the middle and still be a full page. + {"sql", "-q", "insert into nulls values " + + "(1,'alpha'),(2,NULL),(3,'NULL'),(4,'delta'),(5,NULL),(6,'zeta')"}, {"commit", "-Am", msgAddItem}, {"push", "origin", "main"}, {"push", "origin", "dev"}, diff --git a/browse/rows_null_test.go b/browse/rows_null_test.go new file mode 100644 index 0000000000000000000000000000000000000000..6e232a5cc59a6651b17b5b0edefdd7b5cea43bc2 --- /dev/null +++ b/browse/rows_null_test.go @@ -0,0 +1,117 @@ +package browse + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The nulls fixture table, in primary-key order. note is a real NULL at id=2 +// and id=5, and the literal four-character string "NULL" at id=3. +// +// id | note +// 1 | 'alpha' +// 2 | NULL +// 3 | 'NULL' +// 4 | 'delta' +// 5 | NULL +// 6 | 'zeta' + +// TestRowsNullMaskSeparatesStoredNULLText is the whole point of the mask: the +// rendered strings of a real NULL and of a stored "NULL" are equal, and only +// Nulls tells a caller which is which. +func TestRowsNullMaskSeparatesStoredNULLText(t *testing.T) { + db := openFixture(t) + ctx := context.Background() + + page, err := db.Rows(ctx, "main", "nulls", 0, 4) + require.NoError(t, err) + require.Len(t, page.Rows, 4) + require.Equal(t, []string{"id", "note"}, page.Columns) + + realNull := page.Rows[1] // id=2 + storedText := page.Rows[2] // id=3 + assert.Equal(t, placeholderNull, realNull[1]) + assert.Equal(t, placeholderNull, storedText[1]) + assert.Equal(t, realNull[1], storedText[1], + "both must still render identically; the rendered strings are not allowed to change") + + require.Len(t, page.Nulls, len(page.Rows)) + assert.True(t, page.Nulls[1][1], "id=2 stores no value: the mask must say so") + assert.False(t, page.Nulls[2][1], "id=3 stores the text \"NULL\": the mask must not claim it is null") + + // Ordinary values and primary keys are never null. + assert.False(t, page.Nulls[0][0]) + assert.False(t, page.Nulls[0][1]) + assert.False(t, page.Nulls[1][0], "a primary key cannot be null") + assert.False(t, page.Nulls[3][1]) +} + +// TestRowsNullMaskAllFalseWhenPopulated: a table with no NULL in it yields a +// mask that is present, correctly shaped, and entirely false. +func TestRowsNullMaskAllFalseWhenPopulated(t *testing.T) { + db := openFixture(t) + ctx := context.Background() + + page, err := db.Rows(ctx, "main", "users", 0, 10) + require.NoError(t, err) + require.Len(t, page.Rows, 4) + require.Len(t, page.Nulls, len(page.Rows)) + + for i, mask := range page.Nulls { + for j, isNull := range mask { + assert.Falsef(t, isNull, "users row %d cell %d (%q) must not be null", i, j, page.Rows[i][j]) + } + } +} + +// TestRowsNullMaskShape: every mask row is as long as its row, which is as long +// as Columns — so a caller may index Nulls with any index valid for Rows. +func TestRowsNullMaskShape(t *testing.T) { + db := openFixture(t) + ctx := context.Background() + + for _, table := range []string{"users", "nulls", "docs", "items"} { + page, err := db.Rows(ctx, "main", table, 0, 10) + require.NoErrorf(t, err, "table %q", table) + require.Lenf(t, page.Nulls, len(page.Rows), "table %q: one mask per row", table) + for i := range page.Rows { + assert.Lenf(t, page.Nulls[i], len(page.Rows[i]), + "table %q row %d: mask width must equal row width", table, i) + assert.Lenf(t, page.Nulls[i], len(page.Columns), + "table %q row %d: mask width must equal column count", table, i) + } + } +} + +// TestRowsNullMaskPagingAlignment reads a page that neither starts at the first +// row nor ends at the last one: an off-by-one between the mask and the rows +// would show up here and nowhere else. +func TestRowsNullMaskPagingAlignment(t *testing.T) { + db := openFixture(t) + ctx := context.Background() + + // Rows 3, 4, 5 of six: stored "NULL", 'delta', real NULL. + page, err := db.Rows(ctx, "main", "nulls", 2, 3) + require.NoError(t, err) + require.Equal(t, 2, page.Offset) + require.Equal(t, 6, page.Total) + require.Equal(t, [][]string{ + {"3", placeholderNull}, + {"4", "delta"}, + {"5", placeholderNull}, + }, page.Rows) + assert.Equal(t, [][]bool{ + {false, false}, + {false, false}, + {false, true}, + }, page.Nulls, "the mask must be aligned with the rows this page actually returned") + + // A page past the end has no rows and therefore no mask rows. + past, err := db.Rows(ctx, "main", "nulls", 6, 3) + require.NoError(t, err) + assert.Empty(t, past.Rows) + assert.Empty(t, past.Nulls) +} diff --git a/browse/tables.go b/browse/tables.go index a9909d43dea94b056e4d8ab83107bb98c40829f3..953c9b3eeb7cb2fbe04a197fa7cc9c8f060336be 100644 --- a/browse/tables.go +++ b/browse/tables.go @@ -38,11 +38,38 @@ // RowPage is a paginated slice of a table's rows rendered to strings. Columns // lists the column names in the same order as each row's cells. For keyed // tables columns are primary-key columns first, then the rest. +// +// # Why a NULL renders as "NULL" and there is a mask beside it +// +// A cell that holds no value renders as the string "NULL" in Rows, which is +// exactly what a row that genuinely stores the four characters N,U,L,L renders +// as. That flattening stays: Rows is what pages and projections put on screen, +// where "NULL" is the conventional and readable answer, and every consumer of +// this package reads Rows as display strings — encoding nullness into the +// string instead (a sentinel, an empty string, a marker) would ripple through +// every template and every projection for no gain to a reader. +// +// Nulls is for the callers where the two are not interchangeable: an API that +// hands rows to a machine (the MCP row reader) gives out strings with no schema +// beside them, so without the mask an agent cannot tell an absent value from +// the text "NULL" — and answering "is there a value here?" is not something it +// can recover from the rendered string afterwards. +// +// Nulls is parallel to Rows: Nulls[i][j] reports whether Rows[i][j] was a real +// SQL NULL, so any index valid for Rows is valid for Nulls. It costs one bool +// per cell, which is what makes it affordable on a full page. +// +// Only NULL gets this treatment. The other two placeholders, "" and +// "", answer what a value *is*; NULL answers whether there is one +// at all, and only that question is unanswerable from the rendered string. type RowPage struct { Columns []string Rows [][]string - Offset int - Total int + // Nulls[i][j] is true when Rows[i][j] is a real NULL rather than a value + // that happens to render like one. Same shape as Rows. + Nulls [][]bool + Offset int + Total int } // Tables lists the tables in the committed root at ref (a branch name or @@ -206,6 +233,7 @@ page := &RowPage{ Columns: colNames, Rows: [][]string{}, + Nulls: [][]bool{}, Offset: offset, Total: int(total), } @@ -244,14 +272,18 @@ return nil, fmt.Errorf("browse: read row of %q: %w", table, err) } row := make([]string, len(refs)) + // One bool per cell, appended in lockstep with the row so the two can + // never drift apart — including on a page that starts mid-table. + nulls := make([]bool, len(refs)) for i, r := range refs { if r.fromKey { - row[i] = renderCell(ctx, ns, keyDesc, r.idx, key) + row[i], nulls[i] = renderCell(ctx, ns, keyDesc, r.idx, key) } else { - row[i] = renderCell(ctx, ns, valDesc, r.idx, value) + row[i], nulls[i] = renderCell(ctx, ns, valDesc, r.idx, value) } } page.Rows = append(page.Rows, row) + page.Nulls = append(page.Nulls, nulls) } return page, nil @@ -302,20 +334,27 @@ // StringAdaptiveEnc which is inline-or-address in the same field). Those are // resolved through ns to their real content — without this they would render as // "" (addr) or a raw hash string (adaptive), losing every long // description, close reason, comment body, and audit payload. -func renderCell(ctx context.Context, ns tree.NodeStore, td *val.TupleDesc, i int, tup val.Tuple) (out string) { +// +// isNull is true only when the field holds no value. It is the answer the +// rendered string cannot carry, since a stored "NULL" renders the same way; see +// the RowPage doc. It comes from the nullness test the renderer already had to +// do, so reporting it costs no extra read of the tuple. A field that is +// unreadable (out of range, or a panic recovered here) is not null: nothing is +// known about it, which is a different answer from "there is no value". +func renderCell(ctx context.Context, ns tree.NodeStore, td *val.TupleDesc, i int, tup val.Tuple) (out string, isNull bool) { defer func() { if r := recover(); r != nil { - out = placeholderUnreadable + out, isNull = placeholderUnreadable, false } }() if i < 0 || i >= td.Count() { // Column not present in this tuple (e.g. a virtual/dropped column that // isn't materialized): degrade rather than index out of range. - return placeholderUnreadable + return placeholderUnreadable, false } if td.IsNull(i, tup) { - return placeholderNull + return placeholderNull, true } switch td.Types[i].Enc { @@ -328,47 +367,49 @@ val.BytesAddrEnc, val.JSONAddrEnc, val.GeomAddrEnc, val.CommitAddrEnc, val.BytesAdaptiveEnc, val.GeomAdaptiveEnc: // Genuine binary / opaque out-of-band values: never dump raw bytes. - return placeholderBinary + return placeholderBinary, false } - return td.FormatValue(ctx, i, td.GetField(i, tup)) + return td.FormatValue(ctx, i, td.GetField(i, tup)), false } // resolveStringAddr dereferences a StringAddrEnc field (text/longtext always -// stored out-of-line) to its full content. -func resolveStringAddr(ctx context.Context, ns tree.NodeStore, td *val.TupleDesc, i int, tup val.Tuple) string { +// stored out-of-line) to its full content. It returns the same (string, +// isNull) pair as renderCell. +func resolveStringAddr(ctx context.Context, ns tree.NodeStore, td *val.TupleDesc, i int, tup val.Tuple) (string, bool) { h, ok := td.GetStringAddr(i, tup) if !ok { - return placeholderNull + return placeholderNull, true } s, err := val.NewTextStorage(h, ns).Unwrap(ctx) if err != nil { - return placeholderUnreadable + return placeholderUnreadable, false } - return s + return s, false } // resolveStringAdaptive reads a StringAdaptiveEnc field, which stores its value // either inline (returned as a string) or out-of-line (returned as a -// *val.TextStorage to Unwrap) in the same field. -func resolveStringAdaptive(ctx context.Context, ns tree.NodeStore, td *val.TupleDesc, i int, tup val.Tuple) string { +// *val.TextStorage to Unwrap) in the same field. It returns the same (string, +// isNull) pair as renderCell. +func resolveStringAdaptive(ctx context.Context, ns tree.NodeStore, td *val.TupleDesc, i int, tup val.Tuple) (string, bool) { v, ok, err := td.GetStringAdaptiveValue(ctx, i, ns, tup) if err != nil || !ok { if err != nil { - return placeholderUnreadable + return placeholderUnreadable, false } - return placeholderNull + return placeholderNull, true } switch s := v.(type) { case string: - return s + return s, false case *val.TextStorage: str, err := s.Unwrap(ctx) if err != nil { - return placeholderUnreadable + return placeholderUnreadable, false } - return str + return str, false default: - return placeholderUnreadable + return placeholderUnreadable, false } }