bug fix for index miss on dolt_diff_* tables - #3055
Open
zachmu wants to merge 1 commit into
Open
Conversation
Contributor
|
|
SummaryThe run covers core committed-change retrieval, boundary and endpoint filtering, index use, type conversion safety, rollback behavior, and invalid or unusual membership filters. Happy-path and safety behaviors are healthy, but ordering comparisons lose efficient history access and several valid membership queries fail during planning rather than returning results. Not safe to merge yet — this PR introduces multiple user-visible query failures across IN and NOT IN filtering, along with a broad performance regression for ordered commit filters. These are concentrated in important diff-query behavior and require correction before merging. 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}
| @@ -188,19 +188,34 @@ func (b *BinaryOperator) Right() sql.Expression { | |||
|
|
|||
| // IndexScanOperation implements the sql.IndexComparisonExpression interface. | |||
There was a problem hiding this comment.
Ordering filters skip commit indexes
What failed: The filters returned the expected boundary rows, but their query plans did not use the commit index. Each plan used a filter over the diff table without IndexedTableAccess or a commit range.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Ordering filters on commit history return the right rows but may scan all history, making queries slower as repositories grow.
- Steps to Reproduce:
- Create a table with three committed snapshots: add one row, add a second row, then update the first row.
- Run diff-table queries with to_commit <, <=, >, and >= a commit hash.
- Run EXPLAIN for each query and check whether the plan uses the matching commit index and a bounded range.
- Compare the returned rows with the expected commit boundary.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR changes BinaryOperator.IndexScanOperation in server/expression/binary_operator.go:189-203 so equality and all four ordering operators return the values from unwrapIndexScanTarget. That helper only removes GMSCast when its child is a direct *expression.GetField (server/expression/binary_operator.go:210-220), which is intended to expose the indexed diff-table commit column to the planner. The active regression coverage documents that dolt_diff_ is indexed on to_commit and from_commit and requires indexed access (testing/go/dolt_tables_test.go:1335-1338); its EXPLAIN expectations show IndexedTableAccess with a commit range (testing/go/dolt_tables_test.go:1372-1383). However, the recorded EXPLAIN for all four ordering predicates showed only a Filter over dolt_diff_test, with neither IndexedTableAccess nor a commit range. This means the planner's ordering comparison path is not producing the indexed range that the changed code is supposed to enable. The smallest practical fix is to trace the ordering predicate shape through analysis and make the direct cast-unwrapped GetField reach the index range builder, or revert the specific operand normalization if that change is what prevents range construction; do not change result filtering or the diff table schema.
- Why this is likely a bug: The failure is an application query-planning defect, not a setup problem: the same local run produced valid boundary-filtered rows, while EXPLAIN consistently showed the wrong access path for all four supported ordering operators. The repository explicitly requires diff-table commit filters to read only the relevant indexed history, and the PR directly changes the code responsible for advertising those operators to the index planner. A full history scan preserves correctness today but creates a predictable performance regression as commit history grows; restoring the expected bounded index range is the targeted fix.
Relevant code
server/expression/binary_operator.go:189-203
func (b *BinaryOperator) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
left := unwrapIndexScanTarget(b.Left())
right := unwrapIndexScanTarget(b.Right())
switch b.operator {
case framework.Operator_BinaryLessThan:
return sql.IndexScanOpLt, left, right, true
case framework.Operator_BinaryLessOrEqual:
return sql.IndexScanOpLte, left, right, true
case framework.Operator_BinaryGreaterThan:
return sql.IndexScanOpGt, left, right, true
case framework.Operator_BinaryGreaterOrEqual:
return sql.IndexScanOpGte, left, right, trueserver/expression/binary_operator.go:210-220
func unwrapIndexScanTarget(expr sql.Expression) sql.Expression {
if cast, ok := expr.(*GMSCast); ok {
if gf, ok := cast.Child().(*expression.GetField); ok {
return gf
}
}
return expr
}testing/go/dolt_tables_test.go:1372-1383
// The to_commit filter must be pushed into an index lookup rather than scanning
// every commit in history
Query: `EXPLAIN SELECT to_id FROM dolt_diff_test WHERE to_commit = '0123456789abcdefghij0123456789ab'`,
Expected: []sql.Row{
{"Project"},
{" └─ Filter"},
{" └─ IndexedTableAccess(dolt_diff_test)"},
{" ├─ index: [dolt_diff_test.to_commit]"},
{" └─ filters: [{[0123456789abcdefghij0123456789ab, 0123456789abcdefghij0123456789ab]}]
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Ordering filters skip commit indexes**
**What failed:** The filters returned the expected boundary rows, but their query plans did not use the commit index. Each plan used a filter over the diff table without IndexedTableAccess or a commit range.
- **Impact:** Ordering filters on commit history return the right rows but may scan all history, making queries slower as repositories grow.
- **Steps to reproduce:**
1. Create a table with three committed snapshots: add one row, add a second row, then update the first row.
2. Run diff-table queries with to_commit <, <=, >, and >= a commit hash.
3. Run EXPLAIN for each query and check whether the plan uses the matching commit index and a bounded range.
4. Compare the returned rows with the expected commit boundary.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR changes BinaryOperator.IndexScanOperation in server/expression/binary_operator.go:189-203 so equality and all four ordering operators return the values from unwrapIndexScanTarget. That helper only removes GMSCast when its child is a direct *expression.GetField (server/expression/binary_operator.go:210-220), which is intended to expose the indexed diff-table commit column to the planner. The active regression coverage documents that dolt_diff_<table> is indexed on to_commit and from_commit and requires indexed access (testing/go/dolt_tables_test.go:1335-1338); its EXPLAIN expectations show IndexedTableAccess with a commit range (testing/go/dolt_tables_test.go:1372-1383). However, the recorded EXPLAIN for all four ordering predicates showed only a Filter over dolt_diff_test, with neither IndexedTableAccess nor a commit range. This means the planner's ordering comparison path is not producing the indexed range that the changed code is supposed to enable. The smallest practical fix is to trace the ordering predicate shape through analysis and make the direct cast-unwrapped GetField reach the index range builder, or revert the specific operand normalization if that change is what prevents range construction; do not change result filtering or the diff table schema.
- **Why this is likely a bug:** The failure is an application query-planning defect, not a setup problem: the same local run produced valid boundary-filtered rows, while EXPLAIN consistently showed the wrong access path for all four supported ordering operators. The repository explicitly requires diff-table commit filters to read only the relevant indexed history, and the PR directly changes the code responsible for advertising those operators to the index planner. A full history scan preserves correctness today but creates a predictable performance regression as commit history grows; restoring the expected bounded index range is the targeted fix.
**Relevant code:**
`server/expression/binary_operator.go:189-203`
~~~go
func (b *BinaryOperator) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
left := unwrapIndexScanTarget(b.Left())
right := unwrapIndexScanTarget(b.Right())
switch b.operator {
case framework.Operator_BinaryLessThan:
return sql.IndexScanOpLt, left, right, true
case framework.Operator_BinaryLessOrEqual:
return sql.IndexScanOpLte, left, right, true
case framework.Operator_BinaryGreaterThan:
return sql.IndexScanOpGt, left, right, true
case framework.Operator_BinaryGreaterOrEqual:
return sql.IndexScanOpGte, left, right, true
~~~
`server/expression/binary_operator.go:210-220`
~~~go
func unwrapIndexScanTarget(expr sql.Expression) sql.Expression {
if cast, ok := expr.(*GMSCast); ok {
if gf, ok := cast.Child().(*expression.GetField); ok {
return gf
}
}
return expr
}
~~~
`testing/go/dolt_tables_test.go:1372-1383`
~~~go
// The to_commit filter must be pushed into an index lookup rather than scanning
// every commit in history
Query: `EXPLAIN SELECT to_id FROM dolt_diff_test WHERE to_commit = '0123456789abcdefghij0123456789ab'`,
Expected: []sql.Row{
{"Project"},
{" └─ Filter"},
{" └─ IndexedTableAccess(dolt_diff_test)"},
{" ├─ index: [dolt_diff_test.to_commit]"},
{" └─ filters: [{[0123456789abcdefghij0123456789ab, 0123456789abcdefghij0123456789ab]}]
}
~~~
| @@ -282,5 +282,5 @@ func (it *InTuple) Right() sql.Expression { | |||
|
|
|||
| // IndexScanOperation implements the sql.IndexComparisonExpression interface. | |||
There was a problem hiding this comment.
IN query cannot return multiple commits
What failed: The filtered query shows a planner error and returns no results, even though the same diff table returns the expected modified and added rows without the IN filter.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Users cannot run a valid membership-filtered diff query, so the requested commit groups are not returned. The unfiltered diff still works, and there is no evidence of data loss or corruption.
- Steps to Reproduce:
- Create a test table, commit an inserted row, commit a second inserted row, and commit an update to the first row.
- Run SELECT to_commit, to_id FROM dolt_diff('HEAD
2','HEAD','test') WHERE to_commit IN (HASHOF('HEAD'), HASHOF('HEAD1')). - Observe that the query fails with an unresolved plan error instead of returning the rows for the requested commit groups.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The failing SQL shape is handled by server/expression/in_tuple.go:283-285, where InTuple implements sql.IndexComparisonExpression and returns sql.IndexScanOpInSet together with unwrapIndexScanTarget(it.leftExpr) and it.rightExpr. The PR changed line 285 from returning it.leftExpr to returning the unwrapped target, and added unwrapIndexScanTarget in server/expression/binary_operator.go:210-220. That helper only removes a direct GMSCast around expression.GetField, so the left side becomes a bare field as intended, but the complete indexed membership plan still contains a Project node that is not resolved by the local Doltgres planner. The baseline query against dolt_diff('HEAD~2','HEAD','test') returns the modified row (to_id 1, to_val 3) and added row (to_id 2, to_val 2), proving the history and diff table are usable; both EXPLAIN and the filtered IN query fail with
plan is not resolved because of node '*plan.Project'. The smallest practical fix is to correct the IN-set planning path so its projected diff-table plan is resolved after target normalization, or to fall back to the ordinary filter when that indexed plan cannot be resolved, while preserving the existing direct-cast unwrapping behavior for valid index targets. - Why this is likely a bug: This is a valid user query against a working local Dolt history, not a setup or authentication failure. The unfiltered diff returns the expected rows, while the only added condition, a normal IN membership predicate over to_commit, causes planning to abort with an unresolved internal node. The PR explicitly changed this predicate's index-operation path and added support intended to make casted commit fields indexable, so the failure is a production planner defect introduced by the changed path rather than a limitation of the test harness.
Relevant code
server/expression/in_tuple.go:283-285
// IndexScanOperation implements the sql.IndexComparisonExpression interface.
func (it *InTuple) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
return sql.IndexScanOpInSet, unwrapIndexScanTarget(it.leftExpr), it.rightExpr, true
}server/expression/binary_operator.go:210-220
// unwrapIndexScanTarget removes a GMSCast wrapper from an expression when the cast wraps a GetField.
func unwrapIndexScanTarget(expr sql.Expression) sql.Expression {
if cast, ok := expr.(*GMSCast); ok {
if gf, ok := cast.Child().(*expression.GetField); ok {
return gf
}
}
return expr
}testing/go/dolt_tables_test.go:1331-1405
The PR adds dolt diff index-lookup regression coverage for exact commit pairs, one-sided commit filters, and membership filters over a three-commit test history. MEMBERSHIP-1 exercises the corresponding multi-commit IN behavior against that history.Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — IN query cannot return multiple commits**
**What failed:** The filtered query shows a planner error and returns no results, even though the same diff table returns the expected modified and added rows without the IN filter.
- **Impact:** Users cannot run a valid membership-filtered diff query, so the requested commit groups are not returned. The unfiltered diff still works, and there is no evidence of data loss or corruption.
- **Steps to reproduce:**
1. Create a test table, commit an inserted row, commit a second inserted row, and commit an update to the first row.
2. Run SELECT to_commit, to_id FROM dolt_diff('HEAD~2','HEAD','test') WHERE to_commit IN (HASHOF('HEAD'), HASHOF('HEAD~1')).
3. Observe that the query fails with an unresolved plan error instead of returning the rows for the requested commit groups.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The failing SQL shape is handled by server/expression/in_tuple.go:283-285, where InTuple implements sql.IndexComparisonExpression and returns sql.IndexScanOpInSet together with unwrapIndexScanTarget(it.leftExpr) and it.rightExpr. The PR changed line 285 from returning it.leftExpr to returning the unwrapped target, and added unwrapIndexScanTarget in server/expression/binary_operator.go:210-220. That helper only removes a direct GMSCast around expression.GetField, so the left side becomes a bare field as intended, but the complete indexed membership plan still contains a Project node that is not resolved by the local Doltgres planner. The baseline query against dolt_diff('HEAD~2','HEAD','test') returns the modified row (to_id 1, to_val 3) and added row (to_id 2, to_val 2), proving the history and diff table are usable; both EXPLAIN and the filtered IN query fail with `plan is not resolved because of node '*plan.Project'`. The smallest practical fix is to correct the IN-set planning path so its projected diff-table plan is resolved after target normalization, or to fall back to the ordinary filter when that indexed plan cannot be resolved, while preserving the existing direct-cast unwrapping behavior for valid index targets.
- **Why this is likely a bug:** This is a valid user query against a working local Dolt history, not a setup or authentication failure. The unfiltered diff returns the expected rows, while the only added condition, a normal IN membership predicate over to_commit, causes planning to abort with an unresolved internal node. The PR explicitly changed this predicate's index-operation path and added support intended to make casted commit fields indexable, so the failure is a production planner defect introduced by the changed path rather than a limitation of the test harness.
**Relevant code:**
`server/expression/in_tuple.go:283-285`
~~~go
// IndexScanOperation implements the sql.IndexComparisonExpression interface.
func (it *InTuple) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
return sql.IndexScanOpInSet, unwrapIndexScanTarget(it.leftExpr), it.rightExpr, true
}
~~~
`server/expression/binary_operator.go:210-220`
~~~go
// unwrapIndexScanTarget removes a GMSCast wrapper from an expression when the cast wraps a GetField.
func unwrapIndexScanTarget(expr sql.Expression) sql.Expression {
if cast, ok := expr.(*GMSCast); ok {
if gf, ok := cast.Child().(*expression.GetField); ok {
return gf
}
}
return expr
}
~~~
`testing/go/dolt_tables_test.go:1331-1405`
~~~go
The PR adds dolt diff index-lookup regression coverage for exact commit pairs, one-sided commit filters, and membership filters over a three-commit test history. MEMBERSHIP-1 exercises the corresponding multi-commit IN behavior against that history.
~~~
There was a problem hiding this comment.
Filtering commits with NOT IN fails
What failed: The diff table showed the expected baseline modified and added rows, including a NULL previous-row ID for the added row. Adding the valid NOT IN filter caused both EXPLAIN and the query to fail with plan is not resolved because of node '*plan.Project', so excluded commits and NULL handling could not be evaluated.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Users who use NOT IN to filter local diff results cannot run the query. The database stops during planning instead of returning the rows outside the excluded commits.
- Steps to Reproduce:
- Create a local test table, make three commits, and leave an added row whose previous-row ID is NULL.
- Run SELECT from_id, diff_type FROM dolt_diff('HEAD
2','HEAD','test') WHERE from_commit NOT IN ('HEAD','HEAD1'). - Run EXPLAIN for the same filter and inspect the query result.
- Observe that both planning and execution stop with an unresolved planner node instead of returning the filtered rows.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR modifies server/expression/not.go at lines 112-120 so Not.IndexScanOperation recognizes a child *InTuple and returns sql.IndexScanOpNotInSet, unwrapIndexScanTarget(it.Left()), and it.Right(). The same PR changes server/expression/in_tuple.go at lines 283-285 to unwrap the IN target, while server/expression/binary_operator.go lines 210-221 define a helper that removes only a direct *GMSCast around an *expression.GetField. Dolt diff commit columns are GMS-typed and are wrapped during analysis, so this is the intended bridge from the analyzed expression to the index target. However, the valid local NOT IN query still reaches an unresolved *plan.Project before executor evaluation, showing that the newly advertised NOT IN index path does not produce a resolved plan for the projected nullable diff expression. The practical fix is to make the NOT IN path return a planner-compatible target and preserve the projection, or to decline the index optimization for this unresolved shape so the normal filter can execute; this should be covered by the nullable from_id regression case.
- Why this is likely a bug: The baseline diff query succeeds and returns the expected modified and added rows, while the only added condition is a valid NOT IN predicate over an indexed commit column. The same unresolved *plan.Project error appears in both EXPLAIN and execution, so this is not an empty-result assertion or a browser/setup issue. Source inspection ties the failure to the PR's new Not.IndexScanOperation path: it opts into IndexScanOpNotInSet and relies on the new unwrapping helper, but the resulting plan is not resolved when the nullable diff projection is present. A targeted correction to that changed path, or a safe fallback to ordinary filtering for this plan shape, restores the requested query without requiring a broad planner rewrite.
Relevant code
server/expression/not.go:112-120
func (n *Not) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
it, ok := n.child.(*InTuple)
if !ok {
return 0, nil, nil, false
}
return sql.IndexScanOpNotInSet, unwrapIndexScanTarget(it.Left()), it.Right(), true
}server/expression/binary_operator.go:210-221
func unwrapIndexScanTarget(expr sql.Expression) sql.Expression {
if cast, ok := expr.(*GMSCast); ok {
if gf, ok := cast.Child().(*expression.GetField); ok {
return gf
}
}
return expr
}server/expression/in_tuple.go:283-286
func (it *InTuple) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
return sql.IndexScanOpInSet, unwrapIndexScanTarget(it.leftExpr), it.rightExpr, true
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Filtering commits with NOT IN fails**
**What failed:** The diff table showed the expected baseline modified and added rows, including a NULL previous-row ID for the added row. Adding the valid NOT IN filter caused both EXPLAIN and the query to fail with `plan is not resolved because of node '*plan.Project'`, so excluded commits and NULL handling could not be evaluated.
- **Impact:** Users who use NOT IN to filter local diff results cannot run the query. The database stops during planning instead of returning the rows outside the excluded commits.
- **Steps to reproduce:**
1. Create a local test table, make three commits, and leave an added row whose previous-row ID is NULL.
2. Run SELECT from_id, diff_type FROM dolt_diff('HEAD~2','HEAD','test') WHERE from_commit NOT IN ('HEAD','HEAD~1').
3. Run EXPLAIN for the same filter and inspect the query result.
4. Observe that both planning and execution stop with an unresolved planner node instead of returning the filtered rows.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR modifies server/expression/not.go at lines 112-120 so Not.IndexScanOperation recognizes a child *InTuple and returns sql.IndexScanOpNotInSet, unwrapIndexScanTarget(it.Left()), and it.Right(). The same PR changes server/expression/in_tuple.go at lines 283-285 to unwrap the IN target, while server/expression/binary_operator.go lines 210-221 define a helper that removes only a direct *GMSCast around an *expression.GetField. Dolt diff commit columns are GMS-typed and are wrapped during analysis, so this is the intended bridge from the analyzed expression to the index target. However, the valid local NOT IN query still reaches an unresolved *plan.Project before executor evaluation, showing that the newly advertised NOT IN index path does not produce a resolved plan for the projected nullable diff expression. The practical fix is to make the NOT IN path return a planner-compatible target and preserve the projection, or to decline the index optimization for this unresolved shape so the normal filter can execute; this should be covered by the nullable from_id regression case.
- **Why this is likely a bug:** The baseline diff query succeeds and returns the expected modified and added rows, while the only added condition is a valid NOT IN predicate over an indexed commit column. The same unresolved *plan.Project error appears in both EXPLAIN and execution, so this is not an empty-result assertion or a browser/setup issue. Source inspection ties the failure to the PR's new Not.IndexScanOperation path: it opts into IndexScanOpNotInSet and relies on the new unwrapping helper, but the resulting plan is not resolved when the nullable diff projection is present. A targeted correction to that changed path, or a safe fallback to ordinary filtering for this plan shape, restores the requested query without requiring a broad planner rewrite.
**Relevant code:**
`server/expression/not.go:112-120`
~~~go
func (n *Not) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
it, ok := n.child.(*InTuple)
if !ok {
return 0, nil, nil, false
}
return sql.IndexScanOpNotInSet, unwrapIndexScanTarget(it.Left()), it.Right(), true
}
~~~
`server/expression/binary_operator.go:210-221`
~~~go
func unwrapIndexScanTarget(expr sql.Expression) sql.Expression {
if cast, ok := expr.(*GMSCast); ok {
if gf, ok := cast.Child().(*expression.GetField); ok {
return gf
}
}
return expr
}
~~~
`server/expression/in_tuple.go:283-286`
~~~go
func (it *InTuple) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
return sql.IndexScanOpInSet, unwrapIndexScanTarget(it.leftExpr), it.rightExpr, true
}
~~~
There was a problem hiding this comment.
NOT IN queries fail before filtering rows
What failed: The diff rows were available without a filter, but adding any tested NOT IN exclusion caused a planner error instead of returning the rows prescribed by SQL three-valued logic.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Users cannot run diff queries that use a NOT IN exclusion, so they cannot exclude selected commits or check NULL behavior with this query. Unfiltered diff queries still work, and there is no evidence of data loss or corruption.
- Steps to Reproduce:
- Create a test table and make three committed snapshots so dolt_diff_test contains one modified row and one added row with a NULL from_id.
- Run the baseline dolt_diff query and confirm that it returns the two diff rows.
- Run the same diff query with from_commit NOT IN ('HEAD', 'HEAD~1'), then repeat with duplicate exclusions, a NULL member, and a value that is not present.
- Observe that each NOT IN query fails with 'plan is not resolved because of node *plan.Project' before rows or SQL NULL results are returned.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR changes server/expression/not.go:112-120. Not.IndexScanOperation recognizes a NOT expression containing an *InTuple and, at line 119, now returns sql.IndexScanOpNotInSet with unwrapIndexScanTarget(it.Left()) as the index target. unwrapIndexScanTarget is defined in server/expression/binary_operator.go:210-220 and replaces a *GMSCast around a GetField with the bare GetField because index costing expects that shape. The recorded query reaches this specialized NOT IN path: the unfiltered dolt_diff query resolves and returns the modified and added rows, but every filtered form fails during planning with 'plan is not resolved because of node *plan.Project'. This establishes a production-code failure in the changed membership index integration, not a browser or setup failure. The smallest practical fix is to make the NOT IN index operation produce a planner-resolvable target/range for dolt_diff projections, or to decline this specialized index operation for the unsupported projected shape and let the normal filter executor evaluate NOT IN and NULL semantics; either fix should be limited to the changed NOT IN path rather than changing general diff behavior.
- Why this is likely a bug: The failure is deterministic on a valid local three-commit history and is isolated by a working baseline: the same dolt_diff source returns rows until a NOT IN predicate is added. The error occurs before filtering, so the application cannot provide either ordinary exclusion results or the required NULL behavior. The changed Not.IndexScanOperation line is the direct entry point for the advertised NOT IN index operation, and the PR's own regression scope expects this operation to be usable on dolt_diff tables; correcting that narrow planner contract or falling back when it cannot resolve is the practical fix.
Relevant code
server/expression/not.go:112-120
func (n *Not) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
it, ok := n.child.(*InTuple)
if !ok {
return 0, nil, nil, false
}
return sql.IndexScanOpNotInSet, unwrapIndexScanTarget(it.Left()), it.Right(), true
}server/expression/binary_operator.go:210-220
func unwrapIndexScanTarget(expr sql.Expression) sql.Expression {
if cast, ok := expr.(*GMSCast); ok {
if gf, ok := cast.Child().(*expression.GetField); ok {
return gf
}
}
return expr
}testing/go/dolt_tables_test.go:1331-1411
The PR adds dolt_diff regression assertions for commit-index filtering, including NOT IN queries with nullable diff columns and comparisons against expected added and modified rows.Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — NOT IN queries fail before filtering rows**
**What failed:** The diff rows were available without a filter, but adding any tested NOT IN exclusion caused a planner error instead of returning the rows prescribed by SQL three-valued logic.
- **Impact:** Users cannot run diff queries that use a NOT IN exclusion, so they cannot exclude selected commits or check NULL behavior with this query. Unfiltered diff queries still work, and there is no evidence of data loss or corruption.
- **Steps to reproduce:**
1. Create a test table and make three committed snapshots so dolt_diff_test contains one modified row and one added row with a NULL from_id.
2. Run the baseline dolt_diff query and confirm that it returns the two diff rows.
3. Run the same diff query with from_commit NOT IN ('HEAD', 'HEAD~1'), then repeat with duplicate exclusions, a NULL member, and a value that is not present.
4. Observe that each NOT IN query fails with 'plan is not resolved because of node *plan.Project' before rows or SQL NULL results are returned.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR changes server/expression/not.go:112-120. Not.IndexScanOperation recognizes a NOT expression containing an *InTuple and, at line 119, now returns sql.IndexScanOpNotInSet with unwrapIndexScanTarget(it.Left()) as the index target. unwrapIndexScanTarget is defined in server/expression/binary_operator.go:210-220 and replaces a *GMSCast around a GetField with the bare GetField because index costing expects that shape. The recorded query reaches this specialized NOT IN path: the unfiltered dolt_diff query resolves and returns the modified and added rows, but every filtered form fails during planning with 'plan is not resolved because of node *plan.Project'. This establishes a production-code failure in the changed membership index integration, not a browser or setup failure. The smallest practical fix is to make the NOT IN index operation produce a planner-resolvable target/range for dolt_diff projections, or to decline this specialized index operation for the unsupported projected shape and let the normal filter executor evaluate NOT IN and NULL semantics; either fix should be limited to the changed NOT IN path rather than changing general diff behavior.
- **Why this is likely a bug:** The failure is deterministic on a valid local three-commit history and is isolated by a working baseline: the same dolt_diff source returns rows until a NOT IN predicate is added. The error occurs before filtering, so the application cannot provide either ordinary exclusion results or the required NULL behavior. The changed Not.IndexScanOperation line is the direct entry point for the advertised NOT IN index operation, and the PR's own regression scope expects this operation to be usable on dolt_diff tables; correcting that narrow planner contract or falling back when it cannot resolve is the practical fix.
**Relevant code:**
`server/expression/not.go:112-120`
~~~go
func (n *Not) IndexScanOperation() (sql.IndexScanOp, sql.Expression, sql.Expression, bool) {
it, ok := n.child.(*InTuple)
if !ok {
return 0, nil, nil, false
}
return sql.IndexScanOpNotInSet, unwrapIndexScanTarget(it.Left()), it.Right(), true
}
~~~
`server/expression/binary_operator.go:210-220`
~~~go
func unwrapIndexScanTarget(expr sql.Expression) sql.Expression {
if cast, ok := expr.(*GMSCast); ok {
if gf, ok := cast.Child().(*expression.GetField); ok {
return gf
}
}
return expr
}
~~~
`testing/go/dolt_tables_test.go:1331-1411`
~~~go
The PR adds dolt_diff regression assertions for commit-index filtering, including NOT IN queries with nullable diff columns and comparisons against expected added and modified rows.
~~~
Contributor
Footnotes
|
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
2 participants
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.
No description provided.