Resolve WORKING/STAGED in dolt_schema_diff - #1786
Merged
Merged
Conversation
dolt_schema_diff resolved every endpoint with doltliteResolveRef, which
only understands commit refs, so dolt_schema_diff('HEAD','WORKING') (and
'HEAD..WORKING', and STAGED) failed with "to_ref 'WORKING' could not be
resolved" even though the per-table diff surfaces accept those pseudo-refs.
Route both the endpoint resolver (sdResolveOne) and the range-validation
probe through doltliteResolveCatalogHashForRef, the shared catalog-aware
resolver that handles HEAD/WORKING/STAGED and commit refs (AGENTS.md names
it as the one resolver for these). This also collapses the two-step
resolve-then-load-catalog into one call.
Adds fail-before/pass-after coverage: 7 WORKING/STAGED cases (two-arg,
range, self, and staged) that error on the old resolver and pass now.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Version-Control Performance CeilingsRuns: median of 3 executions per benchmark, excluding fixture setup. The
|
Extend vc_oracle_schema_diff_test.sh with five cases exercising the WORKING and STAGED pseudo-refs against real Dolt: added/dropped table via HEAD..WORKING, a no-change EXPECT_EMPTY guard, HEAD..STAGED, and STAGED..WORKING. Confirms doltlite matches Dolt's schema-diff semantics for these refs, which the prior suite (commit-refs only) never checked. Fail-before/pass-after: on the pre-fix engine the four non-empty cases mismatch Dolt (doltlite errored on WORKING/STAGED); with the resolver fix all five pass. Oracle suite goes 60 -> 65 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Sysbench-Style Benchmark: Doltlite vs SQLiteIn-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test; autocommit writes use 9, workload-only timing via host monotonic clock when available. Performance Ceiling Check (2.5x individual, 2x average; autocommit writes: 10x / 5x)All tests within ceilings. |
Contributor
Sysbench-Style Benchmark (composite PK): Doltlite vs SQLiteCompanion to the classic Sysbench-Style Benchmark. Every workload here In-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test; autocommit writes use 9, workload-only timing via host monotonic clock when available. Performance Ceiling Check (2.5x individual, 2x average; autocommit writes: 10x / 5x)All tests within ceilings. |
Contributor
Sysbench-Style Benchmark (BLOB PK): Doltlite vs SQLiteCompanion to the classic Sysbench-Style Benchmark. Every workload here In-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test; autocommit writes use 9, workload-only timing via host monotonic clock when available. Performance Ceiling Check (2.5x individual, 2x average; autocommit writes: 10x / 5x)All tests within ceilings. |
Contributor
Sysbench-Style Benchmark (TEXT PK): Doltlite vs SQLiteCompanion to the classic Sysbench-Style Benchmark. Every workload here In-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test; autocommit writes use 9, workload-only timing via host monotonic clock when available. Performance Ceiling Check (2.5x individual, 2x average; autocommit writes: 10x / 5x)All tests within ceilings. |
|
SummaryCoverage spans schema comparisons across committed, staged, and working states, including additions, ranges, branches, recovery after errors, repeated scans, and committed-history regression behavior. It also exercises invalid-reference edge cases; the broader behavior is healthy, with one minor error-reporting gap for malformed range inputs. Safe to merge — the only PR-attributable issue is minor: invalid range references are rejected but reported with an unhelpful generic error, while no schema data is returned incorrectly. This is a diagnostic usability caveat rather than a merge-blocking behavior failure. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
| int rc; | ||
|
|
||
| rc = doltliteResolveRef(db, zRef, &commitHash); | ||
| int rc = doltliteResolveCatalogHashForRef(db, zRef, pCatHash); |
There was a problem hiding this comment.
Invalid range endpoints hide the failing reference
What failed: Both invalid ranges correctly returned no schema rows and failed validation, but each emitted only ‘unknown operation’ instead of an endpoint-resolution error naming ‘does-not-exist’.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Minor
- Impact: Users who provide a nonexistent range endpoint receive a generic error instead of being told which endpoint could not be resolved, making the query failure harder to diagnose. The invalid query is still rejected and no schema rows are returned.
- Steps to Reproduce:
- Create a database with a committed schema and run dolt_schema_diff with HEAD..does-not-exist.
- Run the inverse range does-not-exist..HEAD.
- Inspect the errors returned for both queries.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The explicit two-reference path calls sdResolveOne at src/doltlite_schema_diff.c:764-766, which formats failures as ‘dolt_schema_diff: '' could not be resolved’. The PR-changed range path at src/doltlite_schema_diff.c:821-826 calls doltliteResolveCatalogHashForRef for each endpoint and returns the raw rc, so it bypasses that formatter; the smallest fix is to route each parsed endpoint through the existing endpoint-aware error handling while retaining the catalog-aware resolver.
- Why this is likely a bug: The local execution reproduced the same generic error for both invalid endpoint directions, and the source shows a deterministic production path that discards the endpoint context. This is not caused by browser setup or test data; preserving the resolver error while assigning the same endpoint-specific message used by explicit references would correct the behavior.
Relevant code
src/doltlite_schema_diff.c:736-742
if( rc!=SQLITE_OK ){
sqlite3_free(pVtab->zErrMsg);
pVtab->zErrMsg = sqlite3_mprintf(
"dolt_schema_diff: %s '%s' could not be resolved", zWhich, zRef);
return SQLITE_ERROR;
}src/doltlite_schema_diff.c:821-826
rc = doltliteResolveCatalogHashForRef(db, zRangeFrom, &probe);
if( rc==SQLITE_OK ) rc = doltliteResolveCatalogHashForRef(db, zRangeTo, &probe);
if( rc!=SQLITE_OK ){
sqlite3_free(zRangeFrom);
sqlite3_free(zRangeTo);
return rc;
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Minor severity — Invalid range endpoints hide the failing reference**
**What failed:** Both invalid ranges correctly returned no schema rows and failed validation, but each emitted only ‘unknown operation’ instead of an endpoint-resolution error naming ‘does-not-exist’.
- **Impact:** Users who provide a nonexistent range endpoint receive a generic error instead of being told which endpoint could not be resolved, making the query failure harder to diagnose. The invalid query is still rejected and no schema rows are returned.
- **Steps to reproduce:**
1. Create a database with a committed schema and run dolt_schema_diff with HEAD..does-not-exist.
2. Run the inverse range does-not-exist..HEAD.
3. Inspect the errors returned for both queries.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The explicit two-reference path calls sdResolveOne at src/doltlite_schema_diff.c:764-766, which formats failures as ‘dolt_schema_diff: <which> '<ref>' could not be resolved’. The PR-changed range path at src/doltlite_schema_diff.c:821-826 calls doltliteResolveCatalogHashForRef for each endpoint and returns the raw rc, so it bypasses that formatter; the smallest fix is to route each parsed endpoint through the existing endpoint-aware error handling while retaining the catalog-aware resolver.
- **Why this is likely a bug:** The local execution reproduced the same generic error for both invalid endpoint directions, and the source shows a deterministic production path that discards the endpoint context. This is not caused by browser setup or test data; preserving the resolver error while assigning the same endpoint-specific message used by explicit references would correct the behavior.
**Relevant code:**
`src/doltlite_schema_diff.c:736-742`
~~~c
if( rc!=SQLITE_OK ){
sqlite3_free(pVtab->zErrMsg);
pVtab->zErrMsg = sqlite3_mprintf(
"dolt_schema_diff: %s '%s' could not be resolved", zWhich, zRef);
return SQLITE_ERROR;
}
~~~
`src/doltlite_schema_diff.c:821-826`
~~~c
rc = doltliteResolveCatalogHashForRef(db, zRangeFrom, &probe);
if( rc==SQLITE_OK ) rc = doltliteResolveCatalogHashForRef(db, zRangeTo, &probe);
if( rc!=SQLITE_OK ){
sqlite3_free(zRangeFrom);
sqlite3_free(zRangeTo);
return rc;
}
~~~Ito QA flagged that a malformed range endpoint (e.g. 'x..HEAD') failed validation with a generic "unknown operation" instead of naming the bad ref, unlike the two-arg form which reports "dolt_schema_diff: to_ref 'x' could not be resolved". The range probe returned the raw resolver rc without setting zErrMsg, so SQLite surfaced a default message. Set a from_ref/to_ref-specific message naming the failing endpoint, matching sdResolveOne. Fail-before/pass-after: two range cases asserting the named-endpoint text (fail on the generic message, pass now). Suite 56 -> 58. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Diff SummaryThe run covered schema comparison across committed, working, staged, and range-based states, including normal additions and recovery after invalid endpoints. It exercised business logic and adversarial edge cases around endpoint validation, error attribution, filtering, repeated queries, and clean recovery without stale results or crashes. Safe to merge — all exercised behaviors passed, including the broader schema-diff regression coverage, with no PR-attributable regressions, new failures, or previously flagged failures. Several previously passing areas were not exercised in this run, but they are coverage caveats rather than merge blockers. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug (found by Ito QA on #1781, but pre-existing on master)
dolt_schema_diff('HEAD','WORKING')errored withdolt_schema_diff: to_ref 'WORKING' could not be resolved, even though the per-table diff surfaces (dolt_diff_<t>('HEAD','WORKING')) accept those pseudo-refs. Confirmed on clean master (independent of #1781).Cause
sdResolveOneresolved every endpoint withdoltliteResolveRef(commit-refs only) +doltliteCommitCatalogHash. The range (..) path's validation probe did the same. Neither understandsWORKING/STAGED.Fix
Route both the endpoint resolver and the range probe through
doltliteResolveCatalogHashForRef— the shared catalog-aware resolver that handlesHEAD/WORKING/STAGEDand commit refs (AGENTS.md names it as the resolver for these). Also collapses the two-step resolve-then-load-catalog into one call (−14 lines in source).Testing (fail-before / pass-after)
Added 7 WORKING/STAGED cases to
test/doltlite_schema_diff.sh(two-arg,HEAD..WORKINGrange,WORKING/WORKINGself, andHEAD/STAGED):49 passed, 7 failed).56 passed, 0 failed.60 passed, 0 failed.-Werror.🤖 Generated with Claude Code