dolthub/dolt#5862: Add ignore system table#10227
Conversation
Add new dolt_status_ignored system table that extends dolt_status with an "ignored" column to show which unstaged tables match dolt_ignore patterns. This provides SQL equivalent of dolt status --ignored functionality. Implementation includes: - StatusIgnoredTable type with schema containing table_name, staged, status, and ignored columns - Registration in system table constants and generated table names lists - Helper function getStatusTableRootsProvider to eliminate duplicate switch case logic between StatusTableName and StatusIgnoredTableName in database.go - Shared getStatusRowsData function used by both dolt_status and dolt_status_ignored to collect status data from roots and working set - Unexported helper functions in status_ignored_table.go for ignore pattern checking: getIgnorePatterns, buildUnstagedTableNameSet, and checkIfIgnored - bats integration tests verifying behavior The ignored column is only set for unstaged tables that match patterns in dolt_ignore. Staged tables, constraint violations, merge conflicts, and schema conflicts always have ignored = 0. Refs: dolthub#5862
|
It looks like the refactor may have changed the behavior of One alternative to having two system tables that share code would be to make |
Fix bugs introduced in initial dolt_status_ignored implementation commit and refactor to eliminate duplicate code between status tables. fixes: - Fix schema conflict status incorrectly set to "merged" instead of "schema conflict" in getStatusRowsData - Add dolt_status_ignored to expected system tables list in DOLT_SHOW_SYSTEM_TABLES test (should fix existing test failures) Refs: dolthub#5862
Address PR review feedback for dolt_status_ignored implementation with refactors to error handling, adapter pattern, test coverage, and copyright year. Changes: - Fix copyright year from 2020 to 2025 in status_ignored_table.go - Add adapter pattern to NewStatusIgnoredTable; matches StatusTable implementation. - Add NewStatusIgnoredTableWithNoAdapter for default behavior - Fix error handling in checkIfIgnored to return errors instead of silently returning byte(0) - Propagate ignore pattern errors up the call stack Test coverage additions: - Empty dolt_ignore table test verifying all tables show ignored=0 - Conflicting patterns test with wildcard (test_*=true) vs specific override (test_special=false) - Update ls.bats to expect 27 system tables instead of 26 Error handling refactored to now matches patterns in status.go, commit .go, and diff.go where IsTableNameIgnored errors are properly propagated up rather than swallowed. Refs: dolthub#5862
|
I think this is good to be reviewed again now |
| } | ||
|
|
||
| // statusRowData contains the data for a single status row. | ||
| type statusRowData struct { |
There was a problem hiding this comment.
This is identical to statusTableRow declared immediately above it. I don't think we need it.
| rows := make([]statusIgnoredTableRow, len(statusRows)) | ||
| for i, row := range statusRows { | ||
| ignored := byte(0) | ||
| // Only check ignore patterns for unstaged tables |
There was a problem hiding this comment.
We also should only check the ignore patterns for tables whose status is being added, not tables being modified or removed. That's what git does, and we want to match git here.
| result, err := ignorePatterns.IsTableNameIgnored(tblNameObj) | ||
| if err != nil { | ||
| return byte(0) | ||
| } |
There was a problem hiding this comment.
We don't want to silently swallow the error here. I'm not sure it makes sense to outline this function if it's small and only gets called in one place.
There was a problem hiding this comment.
Propagating the error is good, but I'm still skeptical that this should be in it's own function, since it only gets called in one place, and casting it to a byte only really makes sense in the context of the status table.
| result, err := ignorePatterns.IsTableNameIgnored(tblNameObj) | ||
| if err != nil { | ||
| return byte(0) | ||
| } |
There was a problem hiding this comment.
Propagating the error is good, but I'm still skeptical that this should be in it's own function, since it only gets called in one place, and casting it to a byte only really makes sense in the context of the status table.
| tableName: tbl.String(), | ||
| isStaged: byte(0), | ||
| status: mergedStatus, | ||
| status: "schema conflict", |
There was a problem hiding this comment.
This and "constraint violation" should probably be stored in a constant, like mergeConflictStatus does below.
There was a problem hiding this comment.
oddly enough I thought I made this change, but not sure what happened. good call out. I'll fix this and the other comments you made.
|
Thanks Nick, I'll update accordingly. Was almost done ambit computer messed up so I called it a night. I'll push the fixes tomorrow |
Refactor dolt_status_ignored to match Git ignore behavior and improve code quality. Logic fix: - Only check ignore patterns for tables with "new table" status Code improvements: - Add constants for status strings (schemaConflictStatus, constraintViolationStatus, newTableStatus) to match existing pattern used by mergeConflictStatus and mergedStatus - Inline checkIfIgnored function since it was only called once - Change ignored field from byte to bool in statusIgnoredTableRow since wire protocol workaround only applies to statusTableRow Refs: dolthub#5862
Update test assertions in dolt_status_ignored tests to use boolean values (true/false) instead of byte values (byte(0)/byte(1)) for the ignored column in statusIgnoredTableRow. Refs: dolthub#5862
| tableName string | ||
| status string | ||
| isStaged byte // not a bool bc wire protocol confuses bools and tinyint(1) | ||
| ignored bool |
There was a problem hiding this comment.
@nicktobey I wasn't sure I read your comment correctly about "... casting it to a byte only really makes sense in the context of the status table...". I read interpreted it as: since statusIgnoredTableRow is a separate struct from statusTableRow, the ignored field doesn't need the byte workaround for wire protocol issues, so I changed ignored from byte to bool. Is this what you meant? or if not, then I can change back to byte
|
WARNING: please don't merge. there is a failing bats tests that appears tied to my changes that I missed. I'll work on fixing that and adding a comment when its fixed |
Update test assert to check for "false" bool instead of int "0" for the ignored column. This is to align with change to use boolean type for the ignored column in the dolt_status_ignored system table. The other tests were previously fixed to handle this type change, but this one slipped through the cracks. Refs: dolthub#5862
…e/5862/make-ignore-system-table
Add adapter and test expectations for the new dolt_status_ignored system table from PR dolthub/dolt#10227. This table provides the same information as dolt_status plus an additional ignored column indicating whether unstaged new tables match a pattern in dolt_ignore. Changes: - Add DoltgresDoltStatusIgnoredTableAdapter in status_ignored.go - Register DoltgresDoltStatusIgnoredTableAdapter in init.go - Update pgcatalog_test.go with new dolt_status_ignore table to expected tables - Add new dolt_status_ignore table to sequences_test.go expected tables Refs: dolthub/dolt#5862, dolthub/dolt#10227
Add adapter and test expectations for the new dolt_status_ignored system table from PR dolthub/dolt#10227. This table provides the same information as dolt_status plus an additional ignored column indicating whether unstaged new tables match a pattern in dolt_ignore. Changes: - Add DoltgresDoltStatusIgnoredTableAdapter in status_ignored.go - Register DoltgresDoltStatusIgnoredTableAdapter in init.go - Update pgcatalog_test.go with new dolt_status_ignore table to expected tables - Add new dolt_status_ignore table to sequences_test.go expected tables Refs: dolthub/dolt#5862, dolthub/dolt#10227
Due to codeaucafe's contribution being on a fork updating the doltgresql's go.mod to point to the updates in the dolt PR, dolthub/dolt#10227 this requires a replace directive. This will at least allow us to ensure this DoltgreSQL's PR can run CI. Refs: dolthub/dolt#5862, dolthub/dolt#10227
Update pg_catalog test expectations to include new status_ignored system table in the dolt schema. Refs: dolthub/dolt#5862, dolthub/dolt#10227
…2-dolt-status-ignored-adapter dolthub/dolt#10227: add dolt_status_ignored dolt system table
Remove temporary replace directive that pointed to codeaucafe fork and update to latest dolthub/dolt/go version containing the merged dolt_status_ignored system table implementation from PR dolthub/dolt#10227. Refs: dolthub/dolt#10227
Summary
Adds a new
dolt_status_ignoredsystem table that extendsdolt_statusfunctionality by including anignoredcolumn to identify which unstaged tables match patterns defined indolt_ignore. This provides a SQL interface equivalent todolt status --ignored.Changes
New System Table
dolt_status_ignored- Same schema asdolt_statusplus anignoredcolumn:table_name(text): Name of the tablestaged(boolean): Whether the table is stagedstatus(text): Status of the table (e.g., "new table", "modified", "conflict")ignored(boolean): Whether the table matches adolt_ignorepatternImplementation Details
The implementation started with logic similar to
status_table.goand was then refactored to share common code between both tables while adding the ignore-checking functionality.go/libraries/doltcore/doltdb/system_table.go
StatusIgnoredTableNameconstantGeneratedSystemTableNames()andDoltGeneratedTableNamesgo/libraries/doltcore/sqle/database.go
StatusIgnoredTableNameingetTableInsensitiveWithRoot()getStatusTableRootsProvider()helper to eliminate duplicate logic betweendolt_statusanddolt_status_ignoredswitch casesgo/libraries/doltcore/sqle/dtables/status_table.go
getStatusRowsData()function using existingstatusTableRowstruct to share status collection logic between both tablesnewStatusItr()to use shared functiongo/libraries/doltcore/sqle/dtables/status_ignored_table.go (new file)
StatusIgnoredTableandStatusIgnoredItrgetStatusRowsData()fromstatus_table.goDoltTableAdapterRegistrysimilar toStatusTablegetIgnorePatterns(): Fetches patterns fromdolt_ignorebuildUnstagedTableNameSet(): Creates set for quick lookupcheckIfIgnored(): Checks if table matches ignore pattern (returns error on failure)Behavior
ignored=1if table name matches a pattern indolt_ignore,ignored=0otherwiseignored=0(staging overrides ignore patterns, matching git behavior)ignored=0Tests
BATS integration tests
integration-tests/bats/system-tables.bats:
ignoredcolumn correctly identifies ignored tables while non-ignored tables haveignored=0ignored=0regardless of ignore patternsdolt ls --systemintegration-tests/bats/ls.bats:
dolt_status_ignored(27 tables instead of 26)Go enginetests
go/libraries/doltcore/sqle/enginetest/dolt_queries.go:
Closes #5862