From 64960009a9a0a5f04d91687d9d9731923c8dfebd Mon Sep 17 00:00:00 2001 From: Robin Ince Date: Fri, 27 Mar 2026 08:09:39 +0000 Subject: [PATCH 1/3] implement column sorting --- internal/engine/engine.go | 312 +++++++++++++++++---------- internal/engine/engine_test.go | 109 ++++++++++ internal/engine/test_helpers_test.go | 4 +- internal/ui/model.go | 219 +++++++++++++++---- internal/ui/model_test.go | 190 +++++++++++++++- 5 files changed, 663 insertions(+), 171 deletions(-) diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 217e782..9a343e6 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -19,6 +19,18 @@ type Engine struct { internalRowIDCol string } +type SortDirection int + +const ( + SortAsc SortDirection = iota + SortDesc +) + +type SortTerm struct { + Column string + Direction SortDirection +} + // New creates a new engine, opens the file, and creates query objects. func New(path string) (*Engine, error) { db, err := sql.Open("duckdb", "") @@ -102,28 +114,17 @@ func (e *Engine) TotalRows() int64 { } // Preview fetches rows for the table view along with their immutable row ids. -func (e *Engine) Preview(ctx context.Context, colNames []string, rowFilter string, limit, offset int) ([]int64, [][]string, error) { - var ( - q string - nCols int +func (e *Engine) Preview(ctx context.Context, colNames []string, rowFilter string, limit, offset int, sorts ...SortTerm) ([]int64, [][]string, error) { + projection := e.previewProjection(colNames) + q := fmt.Sprintf( + "SELECT %s FROM (%s) AS q ORDER BY %s LIMIT %d OFFSET %d", + e.previewSelectList(projection), + e.orderedRowsQuery(rowFilter, projection, sorts), + quoteIdent(orderedOffsetCol), + limit, + offset, ) - if len(colNames) == 0 { - q = fmt.Sprintf("SELECT %s, * EXCLUDE (%s) FROM t_base", quoteIdent(e.internalRowIDCol), quoteIdent(e.internalRowIDCol)) - nCols = len(e.columns) - } else { - var proj strings.Builder - proj.WriteString(quoteIdent(e.internalRowIDCol)) - for _, c := range colNames { - proj.WriteString(", ") - proj.WriteString(quoteIdent(c)) - } - q = fmt.Sprintf("SELECT %s FROM t_base", proj.String()) - nCols = len(colNames) - } - if rowFilter != "" { - q += " WHERE " + rowFilter - } - q += fmt.Sprintf(" ORDER BY %s LIMIT %d OFFSET %d", quoteIdent(e.internalRowIDCol), limit, offset) + nCols := len(projection) rows, err := e.db.QueryContext(ctx, q) if err != nil { @@ -350,22 +351,27 @@ func numericProfileExpr(quotedCol string, mode missing.Mode) string { } // FirstNullRow returns the internal row id of the first missing-like value in a column, or 0 if none. -func (e *Engine) FirstNullRow(ctx context.Context, colName string, filterCols []string, mode missing.Mode) (int64, error) { +func (e *Engine) FirstNullRow(ctx context.Context, colName string, filterCols []string, mode missing.Mode, sorts ...SortTerm) (int64, error) { rowFilter := "" if len(filterCols) > 0 { rowFilter = "(" + buildMissingFilter(filterCols, mode) + ")" } - return e.FirstNullRowWithFilter(ctx, colName, rowFilter, mode) + return e.FirstNullRowWithFilter(ctx, colName, rowFilter, mode, sorts...) } -func (e *Engine) FirstNullRowWithFilter(ctx context.Context, colName, rowFilter string, mode missing.Mode) (int64, error) { - col := quoteIdent(colName) - q := fmt.Sprintf("SELECT min(%s) FROM t_base WHERE %s", quoteIdent(e.internalRowIDCol), mode.SQLPredicate(col)) - if rowFilter != "" { - q += " AND (" + rowFilter + ")" - } +func (e *Engine) FirstNullRowWithFilter(ctx context.Context, colName, rowFilter string, mode missing.Mode, sorts ...SortTerm) (int64, error) { + q := fmt.Sprintf( + `SELECT q.%s FROM (%s) AS q WHERE %s ORDER BY %s LIMIT 1`, + quoteIdent(e.internalRowIDCol), + e.orderedRowsQuery(rowFilter, []string{colName}, sorts), + mode.SQLPredicate(quoteIdent(colName)), + quoteIdent(orderedOffsetCol), + ) var rn sql.NullInt64 if err := e.db.QueryRowContext(ctx, q).Scan(&rn); err != nil { + if err == sql.ErrNoRows { + return 0, nil + } return 0, err } if !rn.Valid { @@ -375,53 +381,59 @@ func (e *Engine) FirstNullRowWithFilter(ctx context.Context, colName, rowFilter } // OffsetForRowID returns the row offset of rowID in the active (optionally filtered) view. -func (e *Engine) OffsetForRowID(ctx context.Context, rowID int64, filterCols []string, mode missing.Mode) (int64, error) { +func (e *Engine) OffsetForRowID(ctx context.Context, rowID int64, filterCols []string, mode missing.Mode, sorts ...SortTerm) (int64, error) { rowFilter := "" if len(filterCols) > 0 { rowFilter = "(" + buildMissingFilter(filterCols, mode) + ")" } - return e.OffsetForRowIDWithFilter(ctx, rowID, rowFilter) + return e.OffsetForRowIDWithFilter(ctx, rowID, rowFilter, sorts...) } -func (e *Engine) OffsetForRowIDWithFilter(ctx context.Context, rowID int64, rowFilter string) (int64, error) { - if rowID <= 1 { - return 0, nil +func (e *Engine) OffsetForRowIDWithFilter(ctx context.Context, rowID int64, rowFilter string, sorts ...SortTerm) (int64, error) { + if rowID <= 0 { + return -1, nil } - q := fmt.Sprintf("SELECT count(*) FROM t_base WHERE %s < ?", quoteIdent(e.internalRowIDCol)) - if rowFilter != "" { - q += " AND (" + rowFilter + ")" - } + q := fmt.Sprintf( + "SELECT %s FROM (%s) AS q WHERE q.%s = ?", + quoteIdent(orderedOffsetCol), + e.orderedRowsQuery(rowFilter, nil, sorts), + quoteIdent(e.internalRowIDCol), + ) var offset int64 if err := e.db.QueryRowContext(ctx, q, rowID).Scan(&offset); err != nil { + if err == sql.ErrNoRows { + return -1, nil + } return 0, err } return offset, nil } // RowIDForOffset returns the internal row id at a filtered-view offset. -func (e *Engine) RowIDForOffset(ctx context.Context, offset int64, filterCols []string, mode missing.Mode) (int64, error) { +func (e *Engine) RowIDForOffset(ctx context.Context, offset int64, filterCols []string, mode missing.Mode, sorts ...SortTerm) (int64, error) { rowFilter := "" if len(filterCols) > 0 { rowFilter = "(" + buildMissingFilter(filterCols, mode) + ")" } - return e.RowIDForOffsetWithFilter(ctx, offset, rowFilter) + return e.RowIDForOffsetWithFilter(ctx, offset, rowFilter, sorts...) } -func (e *Engine) RowIDForOffsetWithFilter(ctx context.Context, offset int64, rowFilter string) (int64, error) { +func (e *Engine) RowIDForOffsetWithFilter(ctx context.Context, offset int64, rowFilter string, sorts ...SortTerm) (int64, error) { if offset < 0 { return 0, nil } - q := fmt.Sprintf("SELECT %s FROM t_base", quoteIdent(e.internalRowIDCol)) - if rowFilter != "" { - q += " WHERE (" + rowFilter + ")" - } - q += fmt.Sprintf(" ORDER BY %s LIMIT 1 OFFSET %d", quoteIdent(e.internalRowIDCol), offset) + q := fmt.Sprintf( + "SELECT q.%s FROM (%s) AS q WHERE q.%s = ?", + quoteIdent(e.internalRowIDCol), + e.orderedRowsQuery(rowFilter, nil, sorts), + quoteIdent(orderedOffsetCol), + ) var rowID int64 - err := e.db.QueryRowContext(ctx, q).Scan(&rowID) + err := e.db.QueryRowContext(ctx, q, offset).Scan(&rowID) if err == sql.ErrNoRows { return 0, nil } @@ -433,92 +445,30 @@ func (e *Engine) RowIDForOffsetWithFilter(ctx context.Context, offset int64, row // NextNullRow returns the next missing-like row id in a column after rowID in the active // (optionally filtered) view. When no later row exists, it wraps and returns the first one. -func (e *Engine) NextNullRow(ctx context.Context, colName string, filterCols []string, mode missing.Mode, rowID int64) (nextRowID int64, wrapped bool, err error) { +func (e *Engine) NextNullRow(ctx context.Context, colName string, filterCols []string, mode missing.Mode, rowID int64, sorts ...SortTerm) (nextRowID int64, wrapped bool, err error) { rowFilter := "" if len(filterCols) > 0 { rowFilter = "(" + buildMissingFilter(filterCols, mode) + ")" } - return e.NextNullRowWithFilter(ctx, colName, rowFilter, mode, rowID) + return e.NextNullRowWithFilter(ctx, colName, rowFilter, mode, rowID, sorts...) } -func (e *Engine) NextNullRowWithFilter(ctx context.Context, colName, rowFilter string, mode missing.Mode, rowID int64) (nextRowID int64, wrapped bool, err error) { - col := quoteIdent(colName) - conds := []string{mode.SQLPredicate(col)} - if rowFilter != "" { - conds = append(conds, "("+rowFilter+")") - } - - baseWhere := strings.Join(conds, " AND ") - baseQuery := fmt.Sprintf("SELECT min(%s) FROM t_base WHERE %s", quoteIdent(e.internalRowIDCol), baseWhere) - q := baseQuery - if rowID > 0 { - q += fmt.Sprintf(" AND %s > %d", quoteIdent(e.internalRowIDCol), rowID) - } - - var rn sql.NullInt64 - if err := e.db.QueryRowContext(ctx, q).Scan(&rn); err != nil { - return 0, false, err - } - if rn.Valid { - return rn.Int64, false, nil - } - if rowID <= 0 { - return 0, false, nil - } - - qWrap := baseQuery - if err := e.db.QueryRowContext(ctx, qWrap).Scan(&rn); err != nil { - return 0, false, err - } - if !rn.Valid || rn.Int64 == rowID { - return 0, false, nil - } - return rn.Int64, true, nil +func (e *Engine) NextNullRowWithFilter(ctx context.Context, colName, rowFilter string, mode missing.Mode, rowID int64, sorts ...SortTerm) (nextRowID int64, wrapped bool, err error) { + return e.nextNullRowWithFilter(ctx, colName, rowFilter, mode, rowID, false, sorts) } // PrevNullRow returns the previous missing-like row id in a column before rowID in the active // (optionally filtered) view. When no earlier row exists, it wraps and returns the last one. -func (e *Engine) PrevNullRow(ctx context.Context, colName string, filterCols []string, mode missing.Mode, rowID int64) (prevRowID int64, wrapped bool, err error) { +func (e *Engine) PrevNullRow(ctx context.Context, colName string, filterCols []string, mode missing.Mode, rowID int64, sorts ...SortTerm) (prevRowID int64, wrapped bool, err error) { rowFilter := "" if len(filterCols) > 0 { rowFilter = "(" + buildMissingFilter(filterCols, mode) + ")" } - return e.PrevNullRowWithFilter(ctx, colName, rowFilter, mode, rowID) + return e.PrevNullRowWithFilter(ctx, colName, rowFilter, mode, rowID, sorts...) } -func (e *Engine) PrevNullRowWithFilter(ctx context.Context, colName, rowFilter string, mode missing.Mode, rowID int64) (prevRowID int64, wrapped bool, err error) { - col := quoteIdent(colName) - conds := []string{mode.SQLPredicate(col)} - if rowFilter != "" { - conds = append(conds, "("+rowFilter+")") - } - - baseWhere := strings.Join(conds, " AND ") - baseQuery := fmt.Sprintf("SELECT max(%s) FROM t_base WHERE %s", quoteIdent(e.internalRowIDCol), baseWhere) - q := baseQuery - if rowID > 0 { - q += fmt.Sprintf(" AND %s < %d", quoteIdent(e.internalRowIDCol), rowID) - } - - var rn sql.NullInt64 - if err := e.db.QueryRowContext(ctx, q).Scan(&rn); err != nil { - return 0, false, err - } - if rn.Valid { - return rn.Int64, false, nil - } - if rowID <= 0 { - return 0, false, nil - } - - qWrap := baseQuery - if err := e.db.QueryRowContext(ctx, qWrap).Scan(&rn); err != nil { - return 0, false, err - } - if !rn.Valid || rn.Int64 == rowID { - return 0, false, nil - } - return rn.Int64, true, nil +func (e *Engine) PrevNullRowWithFilter(ctx context.Context, colName, rowFilter string, mode missing.Mode, rowID int64, sorts ...SortTerm) (prevRowID int64, wrapped bool, err error) { + return e.nextNullRowWithFilter(ctx, colName, rowFilter, mode, rowID, true, sorts) } // BuildMissingFilter builds a SQL WHERE clause for rows with missing-like values in the given columns. @@ -583,3 +533,131 @@ func buildMissingFilter(colNames []string, mode missing.Mode) string { } return strings.Join(parts, " OR ") } + +const orderedOffsetCol = "__pv_ord" + +func (e *Engine) previewProjection(colNames []string) []string { + if len(colNames) > 0 { + return colNames + } + names := make([]string, len(e.columns)) + for i, c := range e.columns { + names[i] = c.Name + } + return names +} + +func (e *Engine) previewSelectList(projection []string) string { + var b strings.Builder + b.WriteString(quoteIdent(e.internalRowIDCol)) + for _, c := range projection { + b.WriteString(", ") + b.WriteString(quoteIdent(c)) + } + return b.String() +} + +func (e *Engine) orderedRowsQuery(rowFilter string, projection []string, sorts []SortTerm) string { + selectCols := append([]string{e.internalRowIDCol}, projection...) + var b strings.Builder + b.WriteString("SELECT ") + b.WriteString(joinQuotedUnique(selectCols)) + b.WriteString(", row_number() OVER (ORDER BY ") + b.WriteString(e.orderByExpr(sorts)) + b.WriteString(") - 1 AS ") + b.WriteString(quoteIdent(orderedOffsetCol)) + b.WriteString(" FROM t_base") + if rowFilter != "" { + b.WriteString(" WHERE ") + b.WriteString(rowFilter) + } + return b.String() +} + +func (e *Engine) orderByExpr(sorts []SortTerm) string { + parts := make([]string, 0, len(sorts)+1) + for _, sort := range sorts { + dir := "ASC" + if sort.Direction == SortDesc { + dir = "DESC" + } + parts = append(parts, fmt.Sprintf("%s %s NULLS LAST", quoteIdent(sort.Column), dir)) + } + parts = append(parts, fmt.Sprintf("%s ASC", quoteIdent(e.internalRowIDCol))) + return strings.Join(parts, ", ") +} + +func joinQuotedUnique(cols []string) string { + seen := make(map[string]struct{}, len(cols)) + parts := make([]string, 0, len(cols)) + for _, col := range cols { + if _, ok := seen[col]; ok { + continue + } + seen[col] = struct{}{} + parts = append(parts, quoteIdent(col)) + } + return strings.Join(parts, ", ") +} + +func (e *Engine) nextNullRowWithFilter(ctx context.Context, colName, rowFilter string, mode missing.Mode, rowID int64, reverse bool, sorts []SortTerm) (int64, bool, error) { + currentOffset, err := e.OffsetForRowIDWithFilter(ctx, rowID, rowFilter, sorts...) + if err != nil { + return 0, false, err + } + + comparator := ">" + direction := "ASC" + if reverse { + comparator = "<" + direction = "DESC" + } + + baseQuery := fmt.Sprintf( + `SELECT q.%s FROM (%s) AS q WHERE %s`, + quoteIdent(e.internalRowIDCol), + e.orderedRowsQuery(rowFilter, []string{colName}, sorts), + mode.SQLPredicate(quoteIdent(colName)), + ) + if currentOffset < 0 { + qWrap := fmt.Sprintf("%s ORDER BY q.%s %s LIMIT 1", baseQuery, quoteIdent(orderedOffsetCol), direction) + var rn sql.NullInt64 + if err := e.db.QueryRowContext(ctx, qWrap).Scan(&rn); err != nil { + if err == sql.ErrNoRows { + return 0, false, nil + } + return 0, false, err + } + if !rn.Valid { + return 0, false, nil + } + return rn.Int64, true, nil + } + q := fmt.Sprintf("%s AND q.%s %s ? ORDER BY q.%s %s LIMIT 1", + baseQuery, + quoteIdent(orderedOffsetCol), + comparator, + quoteIdent(orderedOffsetCol), + direction, + ) + + var rn sql.NullInt64 + if err := e.db.QueryRowContext(ctx, q, currentOffset).Scan(&rn); err != nil && err != sql.ErrNoRows { + return 0, false, err + } + if rn.Valid { + return rn.Int64, false, nil + } + + qWrap := fmt.Sprintf("%s ORDER BY q.%s %s LIMIT 1", baseQuery, quoteIdent(orderedOffsetCol), direction) + if err := e.db.QueryRowContext(ctx, qWrap).Scan(&rn); err != nil { + if err == sql.ErrNoRows { + return 0, false, nil + } + return 0, false, err + } + if !rn.Valid || rn.Int64 == rowID { + return 0, false, nil + } + return rn.Int64, true, nil +} diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index f77684a..95bf1d7 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -243,6 +243,115 @@ func TestPreviewOrderStableByOffset(t *testing.T) { } } +func TestPreviewSupportsSingleAndStackedSorts(t *testing.T) { + dir := t.TempDir() + path := mustWriteCSV(t, dir, "sorted.csv", strings.Join([]string{ + "id,date,cost", + "r1,2,10", + "r2,1,", + "r3,2,5", + "r4,2,5", + "r5,3,7", + }, "\n")+"\n") + eng, err := New(path) + if err != nil { + t.Fatalf("New: %v", err) + } + t.Cleanup(func() { _ = eng.Close() }) + + rows := mustPreview(t, eng, []string{"id"}, "", 5, 0, SortTerm{Column: "date", Direction: SortAsc}) + wantAsc := []string{"r2", "r1", "r3", "r4", "r5"} + for i, want := range wantAsc { + if rows[i][0] != want { + t.Fatalf("date asc row %d: got %q want %q", i, rows[i][0], want) + } + } + + rows = mustPreview(t, eng, []string{"id"}, "", 5, 0, + SortTerm{Column: "date", Direction: SortDesc}, + SortTerm{Column: "cost", Direction: SortAsc}, + ) + wantStacked := []string{"r5", "r3", "r4", "r1", "r2"} + for i, want := range wantStacked { + if rows[i][0] != want { + t.Fatalf("stacked sort row %d: got %q want %q", i, rows[i][0], want) + } + } +} + +func TestPreviewSortNullsLastForBothDirections(t *testing.T) { + dir := t.TempDir() + path := mustWriteCSV(t, dir, "nulls_last.csv", strings.Join([]string{ + "id,cost", + "r1,10", + "r2,", + "r3,5", + "r4,5", + "r5,7", + }, "\n")+"\n") + eng, err := New(path) + if err != nil { + t.Fatalf("New: %v", err) + } + t.Cleanup(func() { _ = eng.Close() }) + + rows := mustPreview(t, eng, []string{"id", "cost"}, "", 5, 0, SortTerm{Column: "cost", Direction: SortAsc}) + if rows[len(rows)-1][0] != "r2" || rows[len(rows)-1][1] != "NULL" { + t.Fatalf("expected NULL row last for ascending sort, got %#v", rows[len(rows)-1]) + } + + rows = mustPreview(t, eng, []string{"id", "cost"}, "", 5, 0, SortTerm{Column: "cost", Direction: SortDesc}) + if rows[len(rows)-1][0] != "r2" || rows[len(rows)-1][1] != "NULL" { + t.Fatalf("expected NULL row last for descending sort, got %#v", rows[len(rows)-1]) + } +} + +func TestSortedOffsetAndRowIDLookup(t *testing.T) { + dir := t.TempDir() + path := mustWriteCSV(t, dir, "lookup.csv", strings.Join([]string{ + "id,date,cost", + "r1,2,10", + "r2,1,", + "r3,2,5", + "r4,2,5", + "r5,3,7", + }, "\n")+"\n") + eng, err := New(path) + if err != nil { + t.Fatalf("New: %v", err) + } + t.Cleanup(func() { _ = eng.Close() }) + ctx := bg() + sorts := []SortTerm{ + {Column: "date", Direction: SortDesc}, + {Column: "cost", Direction: SortAsc}, + } + + offset, err := eng.OffsetForRowIDWithFilter(ctx, 1, "", sorts...) + if err != nil { + t.Fatalf("OffsetForRowIDWithFilter: %v", err) + } + if offset != 3 { + t.Fatalf("expected row 1 at sorted offset 3, got %d", offset) + } + + rowID, err := eng.RowIDForOffsetWithFilter(ctx, 1, "", sorts...) + if err != nil { + t.Fatalf("RowIDForOffsetWithFilter: %v", err) + } + if rowID != 3 { + t.Fatalf("expected sorted offset 1 to hold row 3, got %d", rowID) + } + + offset, err = eng.OffsetForRowIDWithFilter(ctx, 2, `"cost" IS NOT NULL`, sorts...) + if err != nil { + t.Fatalf("OffsetForRowIDWithFilter(filtered out): %v", err) + } + if offset != -1 { + t.Fatalf("expected filtered-out row to report offset -1, got %d", offset) + } +} + func TestOpenCSV(t *testing.T) { eng := openSampleCSV(t) requireOpenHasData(t, eng) diff --git a/internal/engine/test_helpers_test.go b/internal/engine/test_helpers_test.go index 3308b58..cfdae3e 100644 --- a/internal/engine/test_helpers_test.go +++ b/internal/engine/test_helpers_test.go @@ -58,9 +58,9 @@ func requireOpenHasData(t *testing.T, eng *Engine) { } // mustPreview calls Preview and fails the test on error. -func mustPreview(t *testing.T, eng *Engine, cols []string, filter string, limit, offset int) [][]string { +func mustPreview(t *testing.T, eng *Engine, cols []string, filter string, limit, offset int, sorts ...SortTerm) [][]string { t.Helper() - _, rows, err := eng.Preview(bg(), cols, filter, limit, offset) + _, rows, err := eng.Preview(bg(), cols, filter, limit, offset, sorts...) if err != nil { t.Fatalf("Preview: %v", err) } diff --git a/internal/ui/model.go b/internal/ui/model.go index 84bac3c..6fc19af 100644 --- a/internal/ui/model.go +++ b/internal/ui/model.go @@ -93,7 +93,6 @@ type jumpRowMsg struct { requestedRowID int64 rowID int64 offset int64 - approximate bool token uint64 reqID uint64 err error @@ -159,9 +158,10 @@ type Model struct { tableDataOffset int tableRowHasMissing []bool tableCols []string // column names in current projection - tableOffset int // row offset for pagination - tableRowCursor int // row cursor position within visible page - tableColOffHint int // preferred column offset; -1 = auto + tableSorts []engine.SortTerm + tableOffset int // row offset for pagination + tableRowCursor int // row cursor position within visible page + tableColOffHint int // preferred column offset; -1 = auto tableWide bool tableColWidths map[string]int // per-column width overrides (data pane projection columns) showSelected bool // show only selected columns in data pane @@ -304,6 +304,7 @@ func (m *Model) resetLoadedDataState() { m.tableDataOffset = 0 m.tableRowHasMissing = nil m.tableCols = nil + m.tableSorts = nil m.tableOffset = 0 m.tableRowCursor = 0 m.tableColOffHint = -1 @@ -407,6 +408,105 @@ func (m *Model) syncActiveMissingFilter(before []string) bool { return true } +func sortDirectionGlyph(direction engine.SortDirection) string { + if direction == engine.SortDesc { + return "↓" + } + return "↑" +} + +func sortSummaryLabel(sorts []engine.SortTerm) string { + if len(sorts) == 0 { + return "" + } + parts := make([]string, 0, len(sorts)) + for _, sort := range sorts { + parts = append(parts, fmt.Sprintf("%s %s", sort.Column, sortDirectionGlyph(sort.Direction))) + } + return "Sort: " + strings.Join(parts, ", ") +} + +func joinStatusParts(parts ...string) string { + filtered := parts[:0] + for _, part := range parts { + if part == "" { + continue + } + filtered = append(filtered, part) + } + return strings.Join(filtered, "; ") +} + +func (m Model) tableSortSummary() string { + return sortSummaryLabel(m.tableSorts) +} + +func (m Model) tableSortIndex(colName string) int { + for i, sort := range m.tableSorts { + if sort.Column == colName { + return i + } + } + return -1 +} + +func (m Model) tableSortMarker(colName string) string { + idx := m.tableSortIndex(colName) + if idx < 0 { + return "" + } + return fmt.Sprintf(" %s%d", sortDirectionGlyph(m.tableSorts[idx].Direction), idx+1) +} + +func (m *Model) updateTableSort(colName string, direction engine.SortDirection) { + idx := m.tableSortIndex(colName) + if idx >= 0 { + m.tableSorts[idx].Direction = direction + return + } + m.tableSorts = append(m.tableSorts, engine.SortTerm{Column: colName, Direction: direction}) +} + +func (m *Model) pruneTableSortsToProjection() []string { + if len(m.tableSorts) == 0 { + return nil + } + projection := m.projectionCols() + allowed := make(map[string]struct{}, len(projection)) + for _, col := range projection { + allowed[col] = struct{}{} + } + kept := m.tableSorts[:0] + var dropped []string + for _, sort := range m.tableSorts { + if _, ok := allowed[sort.Column]; ok { + kept = append(kept, sort) + continue + } + dropped = append(dropped, sort.Column) + } + if len(kept) == 0 { + m.tableSorts = nil + } else { + m.tableSorts = kept + } + return dropped +} + +func droppedSortsStatus(dropped []string) string { + if len(dropped) == 0 { + return "" + } + quoted := make([]string, len(dropped)) + for i, col := range dropped { + quoted[i] = fmt.Sprintf("%q", col) + } + if len(quoted) == 1 { + return "Dropped sort for column " + quoted[0] + } + return "Dropped sorts for columns " + strings.Join(quoted, ", ") +} + func (m Model) activePredicateFilter() string { return buildPredicateFilter(m.predicates) } @@ -1106,14 +1206,10 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case msg.err != nil: m.statusMsg = fmt.Sprintf("Error: %v", msg.err) case msg.rowID == 0: - m.statusMsg = fmt.Sprintf("Row %d is not visible in current result", msg.requestedRowID) + m.statusMsg = fmt.Sprintf("R%d not in current result", msg.requestedRowID) default: m.setTablePositionForOffset(max(0, int(msg.offset))) - if msg.approximate { - m.statusMsg = fmt.Sprintf("Row %d not visible in current result; jumped to row %d", msg.requestedRowID, msg.rowID) - } else { - m.statusMsg = fmt.Sprintf("Jumped to row %d", msg.rowID) - } + m.statusMsg = fmt.Sprintf("Jumped to row %d", msg.rowID) return m, m.nextPreviewCmd() } return m, nil @@ -1239,10 +1335,14 @@ func (m Model) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { m.clearTableJumpState() filterColsBefore := m.activeFilterCols() m.showSelected = !m.showSelected + droppedSorts := m.pruneTableSortsToProjection() m.tableColOffHint = -1 if !m.syncActiveMissingFilter(filterColsBefore) { m.tableRowCursor = 0 } + if status := droppedSortsStatus(droppedSorts); status != "" { + m.statusMsg = status + } return m, m.nextPreviewCmd() case "v", "V": m.clearTableJumpState() @@ -2050,6 +2150,7 @@ func (m Model) handleColumnsKey(key string) (tea.Model, tea.Cmd) { if dataAutoOff { m.showSelected = false } + droppedSorts := m.pruneTableSortsToProjection() var parts []string if colsAutoOff { parts = append(parts, "cols selected-list off (no columns selected)") @@ -2057,8 +2158,11 @@ func (m Model) handleColumnsKey(key string) (tea.Model, tea.Cmd) { if dataAutoOff { parts = append(parts, "show-selected off (no columns selected)") } + if dropped := droppedSortsStatus(droppedSorts); dropped != "" { + parts = append(parts, dropped) + } if len(parts) > 0 { - m.statusMsg = strings.Join(parts, "; ") + m.statusMsg = joinStatusParts(parts...) } m.syncActiveMissingFilter(filterColsBefore) return m, m.nextPreviewCmd() @@ -2078,6 +2182,7 @@ func (m Model) handleColumnsKey(key string) (tea.Model, tea.Cmd) { } m.sel.AddAll(names) if m.showSelected { + m.pruneTableSortsToProjection() m.syncActiveMissingFilter(filterColsBefore) return m, m.nextPreviewCmd() } @@ -2103,12 +2208,20 @@ func (m Model) handleColumnsKey(key string) (tea.Model, tea.Cmd) { if m.showSelected { if dataAutoOff { m.showSelected = false - var parts []string + } + droppedSorts := m.pruneTableSortsToProjection() + var parts []string + if dataAutoOff { if colsAutoOff { parts = append(parts, "cols selected-list off (no columns selected)") } parts = append(parts, "show-selected off (no columns selected)") - m.statusMsg = strings.Join(parts, "; ") + } + if dropped := droppedSortsStatus(droppedSorts); dropped != "" { + parts = append(parts, dropped) + } + if len(parts) > 0 { + m.statusMsg = joinStatusParts(parts...) } m.syncActiveMissingFilter(filterColsBefore) return m, m.nextPreviewCmd() @@ -2126,6 +2239,7 @@ func (m Model) handleColumnsKey(key string) (tea.Model, tea.Cmd) { m.updateFilteredCols() } if m.showSelected { + m.pruneTableSortsToProjection() m.syncActiveMissingFilter(filterColsBefore) return m, m.nextPreviewCmd() } @@ -2143,13 +2257,17 @@ func (m Model) handleColumnsKey(key string) (tea.Model, tea.Cmd) { } if m.showSelected { m.showSelected = false + droppedSorts := m.pruneTableSortsToProjection() var parts []string if colsAutoOff { parts = append(parts, "cols selected-list off (no columns selected)") } parts = append(parts, "show-selected off (no columns selected)") + if dropped := droppedSortsStatus(droppedSorts); dropped != "" { + parts = append(parts, dropped) + } if len(parts) > 0 { - m.statusMsg = strings.Join(parts, "; ") + m.statusMsg = joinStatusParts(parts...) } m.syncActiveMissingFilter(filterColsBefore) return m, m.nextPreviewCmd() @@ -2263,7 +2381,10 @@ func (m Model) fitWidthForActiveColumn() (int, bool) { return 0, false } maxValueW, _, _ := m.visibleColumnDisplayStats(colIdx) - headerW := lipgloss.Width(m.tableCols[colIdx]) + 1 + headerW := lipgloss.Width(m.tableCols[colIdx]) + lipgloss.Width(m.tableSortMarker(m.tableCols[colIdx])) + 1 + if _, ok := m.predicates[m.tableCols[colIdx]]; ok { + headerW += predicateMarkerWidth() + } if s, ok := m.summaries[m.tableCols[colIdx]]; ok && s.Loaded && s.MissingCount > 0 { headerW += tableHeaderNullDotWidth() } else { @@ -2715,6 +2836,21 @@ func (m Model) handleTableKey(key string) (tea.Model, tea.Cmd) { m.tableOffset = 0 m.tableRowCursor = 0 return m, m.nextPreviewCmd() + case "<", ">": + colName := m.selectedColName + if colName == "" { + m.statusMsg = "No active column" + return m, nil + } + direction := engine.SortAsc + if key == ">" { + direction = engine.SortDesc + } + m.updateTableSort(colName, direction) + m.tableOffset = 0 + m.tableRowCursor = 0 + m.statusMsg = m.tableSortSummary() + return m, m.nextPreviewCmd() case "=": colName := m.selectedColName if colName == "" { @@ -2761,15 +2897,17 @@ func (m Model) handleTableKey(key string) (tea.Model, tea.Cmd) { m.statusMsg = fmt.Sprintf("Cleared filter for %q", colName) return m, m.nextPreviewCmd() case "U": - if len(m.predicates) == 0 { - m.statusMsg = "No filters to clear" + if len(m.predicates) == 0 && !m.missingFilterActive && len(m.tableSorts) == 0 { + m.statusMsg = "No table transforms to clear" return m, nil } m.predicates = make(map[string]columnPredicate) + m.missingFilterActive = false + m.tableSorts = nil m.filterRows = -1 m.tableOffset = 0 m.tableRowCursor = 0 - m.statusMsg = "Cleared all filters" + m.statusMsg = "Cleared table transforms" return m, m.nextPreviewCmd() case "r": if m.jumpToNextMissingInRow(false) { @@ -3124,7 +3262,7 @@ func (m Model) loadPreviewCmd(seq uint64) tea.Cmd { return func() tea.Msg { ctx := context.Background() - rowIDs, rows, err := eng.Preview(ctx, colNames, rowFilter, limit, offset) + rowIDs, rows, err := eng.Preview(ctx, colNames, rowFilter, limit, offset, m.tableSorts...) if err != nil { return previewDoneMsg{seq: seq, token: m.dataToken, err: err} } @@ -3155,30 +3293,23 @@ func (m Model) jumpToRowCmd(rowID int64, reqID uint64) tea.Cmd { missingMode := m.missingMode return func() tea.Msg { ctx := context.Background() - offset, err := eng.OffsetForRowID(ctx, rowID, filterCols, missingMode) + offset, err := eng.OffsetForRowID(ctx, rowID, filterCols, missingMode, m.tableSorts...) if err != nil { return jumpRowMsg{requestedRowID: rowID, token: m.dataToken, reqID: reqID, err: err} } + if offset < 0 { + return jumpRowMsg{requestedRowID: rowID, token: m.dataToken, reqID: reqID} + } - landedRowID, err := eng.RowIDForOffset(ctx, offset, filterCols, missingMode) + landedRowID, err := eng.RowIDForOffset(ctx, offset, filterCols, missingMode, m.tableSorts...) if err != nil { return jumpRowMsg{requestedRowID: rowID, token: m.dataToken, reqID: reqID, err: err} } landedOffset := offset - approximate := landedRowID != 0 && landedRowID != rowID - if landedRowID == 0 && offset > 0 { - landedOffset = offset - 1 - landedRowID, err = eng.RowIDForOffset(ctx, landedOffset, filterCols, missingMode) - if err != nil { - return jumpRowMsg{requestedRowID: rowID, token: m.dataToken, reqID: reqID, err: err} - } - approximate = landedRowID != 0 && landedRowID != rowID - } return jumpRowMsg{ requestedRowID: rowID, rowID: landedRowID, offset: landedOffset, - approximate: approximate, token: m.dataToken, reqID: reqID, } @@ -3267,11 +3398,11 @@ func (m Model) jumpToFirstNull(colName, rowFilter string) tea.Cmd { missingMode := m.missingMode return func() tea.Msg { ctx := context.Background() - rowID, err := eng.FirstNullRowWithFilter(ctx, colName, rowFilter, missingMode) + rowID, err := eng.FirstNullRowWithFilter(ctx, colName, rowFilter, missingMode, m.tableSorts...) if err != nil || rowID == 0 { return firstNullMsg{rowID: rowID, token: m.dataToken, err: err} } - offset, err := eng.OffsetForRowIDWithFilter(ctx, rowID, rowFilter) + offset, err := eng.OffsetForRowIDWithFilter(ctx, rowID, rowFilter, m.tableSorts...) return firstNullMsg{rowID: rowID, offset: offset, token: m.dataToken, err: err} } } @@ -3284,16 +3415,16 @@ func (m Model) jumpToNextNullInColumn(colName, rowFilter string, currentOffset i missingMode := m.missingMode return func() tea.Msg { ctx := context.Background() - rowID, err := eng.RowIDForOffsetWithFilter(ctx, currentOffset, rowFilter) + rowID, err := eng.RowIDForOffsetWithFilter(ctx, currentOffset, rowFilter, m.tableSorts...) if err != nil { return nextNullMsg{token: m.dataToken, colName: colName, reverse: reverse, err: err} } var nextRowID int64 var wrapped bool if reverse { - nextRowID, wrapped, err = eng.PrevNullRowWithFilter(ctx, colName, rowFilter, missingMode, rowID) + nextRowID, wrapped, err = eng.PrevNullRowWithFilter(ctx, colName, rowFilter, missingMode, rowID, m.tableSorts...) } else { - nextRowID, wrapped, err = eng.NextNullRowWithFilter(ctx, colName, rowFilter, missingMode, rowID) + nextRowID, wrapped, err = eng.NextNullRowWithFilter(ctx, colName, rowFilter, missingMode, rowID, m.tableSorts...) } if err != nil || nextRowID == 0 { return nextNullMsg{ @@ -3306,7 +3437,7 @@ func (m Model) jumpToNextNullInColumn(colName, rowFilter string, currentOffset i err: err, } } - offset, err := eng.OffsetForRowIDWithFilter(ctx, nextRowID, rowFilter) + offset, err := eng.OffsetForRowIDWithFilter(ctx, nextRowID, rowFilter, m.tableSorts...) return nextNullMsg{ rowID: nextRowID, offset: offset, @@ -3490,7 +3621,7 @@ func (m Model) viewBottomBar() string { case m.focus == FocusColumns: hints = "Ctrl+O:open jk/↑↓:move Space/C-f/C-b:page C-d/u:half gG/HML:jump m:missing-mode /:search v:sel-list x:toggle a/d/y:sel" default: - hints = "Ctrl+O:open hjkl:move y:copy-cell =:filter p:pin -/U:clear gg/G/[count]G:row m:missing-mode w/W:reader f:filter r/R:row± c/C:col± drag:divider Ctrl+L:redraw" + hints = "Ctrl+O:open Ctrl+L:redraw hjkl:move <>:sort y:copy =/p:filter -:clear U:reset gG/[n]G:row m:missing w/W:reader f:miss-filter r/R:row± c/C:col± drag:divider" } status := fmt.Sprintf(" Sel: %d/%d", selCount, len(m.columns)) if m.showSelected { @@ -3650,7 +3781,7 @@ func (m Model) viewTable(w, h int) string { var lines []string // Header (space prefix for alignment with row null dots) - header := " " + rowNumStyle.Render(fmt.Sprintf("%*s", tableRowNumW, "#")) + header := " " + rowNumStyle.Render(fmt.Sprintf("%*s", tableRowNumW, "row#")) for i := startCol; i < endCol; i++ { colW := m.columnWidth(m.tableCols[i], colAreaWidth) nameBudget := max(0, colW-2) @@ -3659,8 +3790,10 @@ func (m Model) viewTable(w, h int) string { nameBudget = max(0, nameBudget-predicateMarkerWidth()) predicateGlyph = " " + predicateMarkerChar } + sortGlyph := m.tableSortMarker(m.tableCols[i]) + nameBudget = max(0, nameBudget-lipgloss.Width(sortGlyph)) name := truncateDisplayMiddle(m.tableCols[i], nameBudget) - nameStr := " " + padDisplayRight(name+predicateGlyph, max(0, colW-2)) + nameStr := " " + padDisplayRight(name+predicateGlyph+sortGlyph, max(0, colW-2)) // Check if column has missing values from profiling. hasMissing := false if s, ok := m.summaries[m.tableCols[i]]; ok && s.Loaded && s.MissingCount > 0 { @@ -3839,6 +3972,9 @@ func (m Model) viewTableFooter() string { parts = append(parts, fmt.Sprintf("R%d", rowID)) } } + if summary := m.tableSortSummary(); summary != "" { + parts = append(parts, summary) + } return strings.Join(parts, " ") } @@ -4144,10 +4280,11 @@ func (m Model) viewHelp() string { {"Ctrl+F / Space", "Page down"}, {"Ctrl+B", "Page up"}, {"Ctrl+D / Ctrl+U", "Half page down / up"}, + {"< / >", "Sort active column ascending / descending"}, {"=", "Edit predicate for active column"}, {"p", "Pin active cell as exact-match filter"}, {"-", "Clear predicate for active column"}, - {"U", "Clear all predicates"}, + {"U", "Reset table transforms"}, {"f", "Toggle missing-row filter"}, {"r", "Next missing in current row"}, {"R", "Previous missing in current row"}, diff --git a/internal/ui/model_test.go b/internal/ui/model_test.go index 20693a1..8f6b930 100644 --- a/internal/ui/model_test.go +++ b/internal/ui/model_test.go @@ -2071,7 +2071,8 @@ func TestReaderPagingPastBufferPreservesCompatibleModeUntilPreviewLoads(t *testi t.Fatalf("expected reader mode to remain JSON pretty while row is unavailable, got %v", m.readerMode) } - nextRows := append(rows[1:], []string{ + nextRows := rows[1:] + nextRows = append(nextRows, []string{ fmt.Sprintf("%d", len(rows)+1), fmt.Sprintf(`{"row":%d,"kind":"reloaded"}`, len(rows)+1), }) @@ -2346,7 +2347,7 @@ func TestHandleTableKeyGWithZeroVisibleRowsStaysWithinBounds(t *testing.T) { } } -func TestJumpToRowFallsForwardInFilteredResult(t *testing.T) { +func TestJumpToRowReportsFilteredOutRow(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "rows.csv") data := "id,keep\n1,a\n2,\n3,b\n4,\n5,c\n" @@ -2371,14 +2372,11 @@ func TestJumpToRowFallsForwardInFilteredResult(t *testing.T) { if msg.err != nil { t.Fatalf("unexpected jump error: %v", msg.err) } - if msg.rowID != 4 { - t.Fatalf("expected filtered jump to land on row 4, got %d", msg.rowID) + if msg.rowID != 0 { + t.Fatalf("expected filtered-out jump to report no visible row, got %d", msg.rowID) } - if msg.offset != 1 { - t.Fatalf("expected filtered jump offset 1, got %d", msg.offset) - } - if !msg.approximate { - t.Fatal("expected filtered jump to be marked approximate") + if msg.offset != 0 { + t.Fatalf("expected filtered-out jump offset 0, got %d", msg.offset) } } @@ -2400,6 +2398,151 @@ func TestJumpRowMsgIgnoresStaleRequestID(t *testing.T) { } } +func TestHandleTableKeySortKeysStackAndUpdateInPlace(t *testing.T) { + m := newCmdTestModel() + m.tableCols = []string{"date", "cost"} + m.selectedColName = "date" + m.tableOffset = 9 + m.tableRowCursor = 2 + + updated, cmd := m.handleTableKey(">") + if cmd == nil { + t.Fatal("expected preview refresh after descending sort") + } + m = updated.(Model) + if got := m.tableSorts; len(got) != 1 || got[0].Column != "date" || got[0].Direction != engine.SortDesc { + t.Fatalf("unexpected sort stack after >: %#v", got) + } + if m.tableOffset != 0 || m.tableRowCursor != 0 { + t.Fatalf("expected table position reset after sorting, got offset=%d cursor=%d", m.tableOffset, m.tableRowCursor) + } + + m.selectedColName = "cost" + updated, cmd = m.handleTableKey("<") + if cmd == nil { + t.Fatal("expected preview refresh after stacked sort") + } + m = updated.(Model) + if got := m.tableSorts; len(got) != 2 || got[1].Column != "cost" || got[1].Direction != engine.SortAsc { + t.Fatalf("unexpected stacked sorts after <: %#v", got) + } + + m.selectedColName = "date" + updated, cmd = m.handleTableKey("<") + if cmd == nil { + t.Fatal("expected preview refresh after re-sorting existing column") + } + m = updated.(Model) + if got := m.tableSorts; len(got) != 2 || got[0].Column != "date" || got[0].Direction != engine.SortAsc || got[1].Column != "cost" { + t.Fatalf("expected direction update in place, got %#v", got) + } +} + +func TestHandleTableKeyUClearsTableTransforms(t *testing.T) { + m := newCmdTestModel() + m.predicates["alpha"] = columnPredicate{Column: "alpha", Display: "= 1"} + m.missingFilterActive = true + m.tableSorts = []engine.SortTerm{{Column: "alpha", Direction: engine.SortAsc}} + + updated, cmd := m.handleTableKey("U") + if cmd == nil { + t.Fatal("expected preview refresh when clearing transforms") + } + m = updated.(Model) + if len(m.predicates) != 0 { + t.Fatalf("expected predicates cleared, got %#v", m.predicates) + } + if m.missingFilterActive { + t.Fatal("expected missing-row filter cleared") + } + if len(m.tableSorts) != 0 { + t.Fatalf("expected sort stack cleared, got %#v", m.tableSorts) + } +} + +func TestToggleShowSelectedDropsSortForRemovedProjectionColumn(t *testing.T) { + m := newCmdTestModel() + m.focus = FocusTable + m.columns = []types.ColumnInfo{{Name: "alpha"}, {Name: "beta"}} + m.sel = selection.New([]string{"alpha", "beta"}) + m.sel.Add("alpha") + m.tableSorts = []engine.SortTerm{{Column: "beta", Direction: engine.SortDesc}} + + updated, cmd := m.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'s'}}) + if cmd == nil { + t.Fatal("expected preview refresh when toggling selected-only data view") + } + m = updated.(Model) + if !m.showSelected { + t.Fatal("expected showSelected enabled") + } + if len(m.tableSorts) != 0 { + t.Fatalf("expected sort for unprojected column to be dropped, got %#v", m.tableSorts) + } + if !strings.Contains(m.statusMsg, `"beta"`) { + t.Fatalf("expected status to mention dropped sort column, got %q", m.statusMsg) + } +} + +func TestJumpToRowUsesSortedOffsets(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "sorted_jump.csv") + data := "id,group\n1,b\n2,a\n3,c\n4,a\n" + if err := os.WriteFile(path, []byte(data), 0o644); err != nil { + t.Fatalf("write csv: %v", err) + } + + eng := openTestEngine(t, path) + m := NewModel(eng, filepath.Base(path), dir) + m.tableSorts = []engine.SortTerm{{Column: "group", Direction: engine.SortAsc}} + m.jumpReqID = 1 + + cmd := m.jumpToRowCmd(1, m.jumpReqID) + if cmd == nil { + t.Fatal("expected jump command") + } + raw := cmd() + msg, ok := raw.(jumpRowMsg) + if !ok { + t.Fatalf("expected jumpRowMsg, got %T", raw) + } + if msg.err != nil { + t.Fatalf("unexpected jump error: %v", msg.err) + } + if msg.rowID != 1 || msg.offset != 2 { + t.Fatalf("expected row 1 at sorted offset 2, got row=%d offset=%d", msg.rowID, msg.offset) + } +} + +func TestJumpToNextNullInColumnUsesSortedOrder(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "sorted_nulls.csv") + data := "id,grp,score\n1,b,1\n2,a,\n3,c,\n4,a,2\n" + if err := os.WriteFile(path, []byte(data), 0o644); err != nil { + t.Fatalf("write csv: %v", err) + } + + eng := openTestEngine(t, path) + m := NewModel(eng, filepath.Base(path), dir) + m.tableSorts = []engine.SortTerm{{Column: "grp", Direction: engine.SortAsc}} + + cmd := m.jumpToNextNullInColumn("score", "", 1, false) + if cmd == nil { + t.Fatal("expected next-null command") + } + raw := cmd() + msg, ok := raw.(nextNullMsg) + if !ok { + t.Fatalf("expected nextNullMsg, got %T", raw) + } + if msg.err != nil { + t.Fatalf("unexpected next-null error: %v", msg.err) + } + if msg.rowID != 3 || msg.offset != 3 { + t.Fatalf("expected next missing row to follow sorted order to row 3 at offset 3, got row=%d offset=%d", msg.rowID, msg.offset) + } +} + func TestHandleTableKeyGRejectsOverflowJumpCount(t *testing.T) { m := newCmdTestModel() m.tableJumpCount = strings.Repeat("9", 32) @@ -3416,6 +3559,31 @@ func TestViewTableFooterIncludesCellValue(t *testing.T) { } } +func TestViewTableShowsSortMarkersAndSummary(t *testing.T) { + m := newTestModel() + m.width = 100 + m.height = 10 + m.tableCols = []string{"date", "cost"} + m.selectedColName = "date" + m.tableData = [][]string{{"2024-01-02", "10"}} + m.tableSorts = []engine.SortTerm{ + {Column: "date", Direction: engine.SortDesc}, + {Column: "cost", Direction: engine.SortAsc}, + } + + w, h := m.tablePaneDimensions() + out := m.viewTable(w, h) + if !strings.Contains(out, "row#") { + t.Fatalf("expected table header to label immutable row ids, got %q", out) + } + if !strings.Contains(out, "↓1") || !strings.Contains(out, "↑2") { + t.Fatalf("expected header sort markers, got %q", out) + } + if !strings.Contains(out, "Sort: date ↓, cost ↑") { + t.Fatalf("expected footer sort summary, got %q", out) + } +} + func TestViewTableFooterShowsEllipsisWhenProfilingInProgress(t *testing.T) { m := newTestModel() m.tableCols = []string{"a"} @@ -4510,10 +4678,10 @@ func TestHelpAndBottomBarIncludeMouseDividerAndCtrlL(t *testing.T) { if !strings.Contains(bottom, "Ctrl+L:redraw") { t.Fatalf("expected bottom bar to include ctrl+l hint, got %q", bottom) } - if !strings.Contains(bottom, "m:missing-mode") { + if !strings.Contains(bottom, "m:missing") { t.Fatalf("expected bottom bar to include missing mode hint, got %q", bottom) } - if !strings.Contains(bottom, "y:copy-cell") { + if !strings.Contains(bottom, "y:copy") { t.Fatalf("expected bottom bar to include copy-cell hint, got %q", bottom) } From 1a8f32eab438858c5cd8f9b146d664ca02101382 Mon Sep 17 00:00:00 2001 From: Robin Ince Date: Fri, 27 Mar 2026 08:33:23 +0000 Subject: [PATCH 2/3] address review comments --- internal/engine/engine.go | 2 +- internal/engine/engine_test.go | 24 ++++++++++++++++++++++++ internal/ui/model.go | 2 +- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 9a343e6..d55db22 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -631,7 +631,7 @@ func (e *Engine) nextNullRowWithFilter(ctx context.Context, colName, rowFilter s if !rn.Valid { return 0, false, nil } - return rn.Int64, true, nil + return rn.Int64, rowID > 0, nil } q := fmt.Sprintf("%s AND q.%s %s ? ORDER BY q.%s %s LIMIT 1", baseQuery, diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index 95bf1d7..6464cbb 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -1168,6 +1168,30 @@ func TestPrevNullRowWrap(t *testing.T) { } } +func TestNextNullRowWithoutCurrentPositionDoesNotReportWrap(t *testing.T) { + eng := openSampleParquet(t) + ctx := bg() + + first, err := eng.FirstNullRow(ctx, "score", nil, missing.ModeNullAndNaN) + if err != nil { + t.Fatalf("FirstNullRow: %v", err) + } + if first == 0 { + t.Fatal("expected at least one missing score row") + } + + next, wrapped, err := eng.NextNullRow(ctx, "score", nil, missing.ModeNullAndNaN, 0) + if err != nil { + t.Fatalf("NextNullRow(no current row): %v", err) + } + if wrapped { + t.Fatal("expected wrapped=false when there is no current position") + } + if next != first { + t.Fatalf("expected first missing row without wrap, got %d want %d", next, first) + } +} + func TestFirstNullRowModeNullOnly(t *testing.T) { dir := t.TempDir() // Row 2 is NULL, row 3 is NaN — ModeNullOnly should find only row 2. diff --git a/internal/ui/model.go b/internal/ui/model.go index 6fc19af..d10827b 100644 --- a/internal/ui/model.go +++ b/internal/ui/model.go @@ -427,7 +427,7 @@ func sortSummaryLabel(sorts []engine.SortTerm) string { } func joinStatusParts(parts ...string) string { - filtered := parts[:0] + filtered := make([]string, 0, len(parts)) for _, part := range parts { if part == "" { continue From f35eb9894bcad8ab3d7152ed37e4b71dc9d51bee Mon Sep 17 00:00:00 2001 From: Robin Ince Date: Fri, 27 Mar 2026 08:45:33 +0000 Subject: [PATCH 3/3] fix: potential column name collision --- internal/engine/engine.go | 33 ++++++++++++++++++--------------- internal/engine/engine_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 15 deletions(-) diff --git a/internal/engine/engine.go b/internal/engine/engine.go index d55db22..4debbd7 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -17,6 +17,7 @@ type Engine struct { totalRows int64 columns []types.ColumnInfo internalRowIDCol string + orderedOffsetCol string } type SortDirection int @@ -44,11 +45,16 @@ func New(path string) (*Engine, error) { return nil, err } - internalRowIDCol, err := uniqueInternalRowIDCol(db, sourceExpr) + internalRowIDCol, err := uniqueInternalColName(db, sourceExpr, "__pv_rowid") if err != nil { _ = db.Close() return nil, fmt.Errorf("resolve internal row id column: %w", err) } + orderedOffsetCol, err := uniqueInternalColName(db, sourceExpr, "__pv_ord") + if err != nil { + _ = db.Close() + return nil, fmt.Errorf("resolve ordered offset column: %w", err) + } query := fmt.Sprintf(` CREATE TABLE t_base AS @@ -62,7 +68,7 @@ func New(path string) (*Engine, error) { return nil, fmt.Errorf("create base objects: %w", err) } - e := &Engine{db: db, internalRowIDCol: internalRowIDCol} + e := &Engine{db: db, internalRowIDCol: internalRowIDCol, orderedOffsetCol: orderedOffsetCol} if err := e.loadSchema(); err != nil { _ = db.Close() @@ -120,7 +126,7 @@ func (e *Engine) Preview(ctx context.Context, colNames []string, rowFilter strin "SELECT %s FROM (%s) AS q ORDER BY %s LIMIT %d OFFSET %d", e.previewSelectList(projection), e.orderedRowsQuery(rowFilter, projection, sorts), - quoteIdent(orderedOffsetCol), + quoteIdent(e.orderedOffsetCol), limit, offset, ) @@ -365,7 +371,7 @@ func (e *Engine) FirstNullRowWithFilter(ctx context.Context, colName, rowFilter quoteIdent(e.internalRowIDCol), e.orderedRowsQuery(rowFilter, []string{colName}, sorts), mode.SQLPredicate(quoteIdent(colName)), - quoteIdent(orderedOffsetCol), + quoteIdent(e.orderedOffsetCol), ) var rn sql.NullInt64 if err := e.db.QueryRowContext(ctx, q).Scan(&rn); err != nil { @@ -396,7 +402,7 @@ func (e *Engine) OffsetForRowIDWithFilter(ctx context.Context, rowID int64, rowF q := fmt.Sprintf( "SELECT %s FROM (%s) AS q WHERE q.%s = ?", - quoteIdent(orderedOffsetCol), + quoteIdent(e.orderedOffsetCol), e.orderedRowsQuery(rowFilter, nil, sorts), quoteIdent(e.internalRowIDCol), ) @@ -429,7 +435,7 @@ func (e *Engine) RowIDForOffsetWithFilter(ctx context.Context, offset int64, row "SELECT q.%s FROM (%s) AS q WHERE q.%s = ?", quoteIdent(e.internalRowIDCol), e.orderedRowsQuery(rowFilter, nil, sorts), - quoteIdent(orderedOffsetCol), + quoteIdent(e.orderedOffsetCol), ) var rowID int64 @@ -496,7 +502,7 @@ func escapeSQLString(s string) string { return strings.ReplaceAll(s, "'", "''") } -func uniqueInternalRowIDCol(db *sql.DB, sourceExpr string) (string, error) { +func uniqueInternalColName(db *sql.DB, sourceExpr, base string) (string, error) { rows, err := db.Query(fmt.Sprintf("DESCRIBE SELECT * FROM %s", sourceExpr)) if err != nil { return "", err @@ -516,7 +522,6 @@ func uniqueInternalRowIDCol(db *sql.DB, sourceExpr string) (string, error) { return "", err } - base := "__pv_rowid" candidate := base for i := 1; ; i++ { if _, exists := used[strings.ToLower(candidate)]; !exists { @@ -534,8 +539,6 @@ func buildMissingFilter(colNames []string, mode missing.Mode) string { return strings.Join(parts, " OR ") } -const orderedOffsetCol = "__pv_ord" - func (e *Engine) previewProjection(colNames []string) []string { if len(colNames) > 0 { return colNames @@ -565,7 +568,7 @@ func (e *Engine) orderedRowsQuery(rowFilter string, projection []string, sorts [ b.WriteString(", row_number() OVER (ORDER BY ") b.WriteString(e.orderByExpr(sorts)) b.WriteString(") - 1 AS ") - b.WriteString(quoteIdent(orderedOffsetCol)) + b.WriteString(quoteIdent(e.orderedOffsetCol)) b.WriteString(" FROM t_base") if rowFilter != "" { b.WriteString(" WHERE ") @@ -620,7 +623,7 @@ func (e *Engine) nextNullRowWithFilter(ctx context.Context, colName, rowFilter s mode.SQLPredicate(quoteIdent(colName)), ) if currentOffset < 0 { - qWrap := fmt.Sprintf("%s ORDER BY q.%s %s LIMIT 1", baseQuery, quoteIdent(orderedOffsetCol), direction) + qWrap := fmt.Sprintf("%s ORDER BY q.%s %s LIMIT 1", baseQuery, quoteIdent(e.orderedOffsetCol), direction) var rn sql.NullInt64 if err := e.db.QueryRowContext(ctx, qWrap).Scan(&rn); err != nil { if err == sql.ErrNoRows { @@ -635,9 +638,9 @@ func (e *Engine) nextNullRowWithFilter(ctx context.Context, colName, rowFilter s } q := fmt.Sprintf("%s AND q.%s %s ? ORDER BY q.%s %s LIMIT 1", baseQuery, - quoteIdent(orderedOffsetCol), + quoteIdent(e.orderedOffsetCol), comparator, - quoteIdent(orderedOffsetCol), + quoteIdent(e.orderedOffsetCol), direction, ) @@ -649,7 +652,7 @@ func (e *Engine) nextNullRowWithFilter(ctx context.Context, colName, rowFilter s return rn.Int64, false, nil } - qWrap := fmt.Sprintf("%s ORDER BY q.%s %s LIMIT 1", baseQuery, quoteIdent(orderedOffsetCol), direction) + qWrap := fmt.Sprintf("%s ORDER BY q.%s %s LIMIT 1", baseQuery, quoteIdent(e.orderedOffsetCol), direction) if err := e.db.QueryRowContext(ctx, qWrap).Scan(&rn); err != nil { if err == sql.ErrNoRows { return 0, false, nil diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index 6464cbb..44cd7db 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -646,6 +646,40 @@ func TestInternalRowIDNameCollision(t *testing.T) { } } +func TestOrderedOffsetNameCollisionDoesNotBreakSortedQueries(t *testing.T) { + dir := t.TempDir() + csv := "__pv_ord,value,group\nuser-1,10,b\nuser-2,5,a\nuser-3,5,a\n" + path := mustWriteCSV(t, dir, "ordered_offset_collision.csv", csv) + + eng, err := New(path) + if err != nil { + t.Fatalf("New: %v", err) + } + t.Cleanup(func() { _ = eng.Close() }) + + if eng.orderedOffsetCol == "__pv_ord" { + t.Fatalf("expected ordered offset column to avoid collision, got %q", eng.orderedOffsetCol) + } + + rows := mustPreview(t, eng, []string{"__pv_ord", "value", "group"}, "", 3, 0, + SortTerm{Column: "group", Direction: SortAsc}, + SortTerm{Column: "value", Direction: SortAsc}, + ) + requirePreviewShape(t, rows, 3, 3) + if rows[0][0] != "user-2" || rows[1][0] != "user-3" || rows[2][0] != "user-1" { + t.Fatalf("unexpected sorted rows with __pv_ord collision: %#v", rows) + } + + ctx := bg() + offset, err := eng.OffsetForRowIDWithFilter(ctx, 1, "", SortTerm{Column: "group", Direction: SortAsc}) + if err != nil { + t.Fatalf("OffsetForRowIDWithFilter: %v", err) + } + if offset != 2 { + t.Fatalf("expected row 1 at sorted offset 2, got %d", offset) + } +} + func TestOpenLargeCSVOptIn(t *testing.T) { if os.Getenv("PARQVIEW_LARGE_TEST") != "1" { t.Skip("set PARQVIEW_LARGE_TEST=1 to run large-file regression test")