support WHERE predicate in indexes#2743
Merged
Merged
Conversation
Contributor
|
Contributor
|
Ito Test Report ❌13 test cases ran. 2 failed, 11 passed. Overall, 11 of 13 tests passed, confirming expected behavior for partial-index creation and enforcement, planner predicate-aware index usage, migration-batch failure visibility, concurrent duplicate-name index creation, and most catalog and replica-identity compatibility checks under real SQL engine paths (with only auth/bootstrap bypassed for deterministic local setup). The two validated regressions introduced by this PR are: a medium-severity catalog DDL rendering bug that truncates dotted/qualified index expressions in pg_get_indexdef/pg_indexes, and a high-severity ALTER TABLE REPLICA IDENTITY bug where commands return success but never persist state (relreplident remains default 'd'), risking incorrect schema diffing and replication assumptions. ❌ Failed (2)
🟠 Dotted index expressions are truncated in catalog DDL
Relevant code:
cols := make([]string, len(index.Expressions()))
for i, expr := range index.Expressions() {
split := strings.Split(expr, ".")
if len(split) > 1 {
cols[i] = split[1]
} else {
cols[i] = expr
}
}
cols := make([]string, len(index.Expressions()))
for i, expr := range index.Expressions() {
split := strings.Split(expr, ".")
if len(split) > 1 {
cols[i] = split[1]
} else {
cols[i] = expr
}
}
|
| Category | Summary | Screenshot |
|---|---|---|
| Catalog | pg_catalog.pg_indexes returned CREATE INDEX text with a WHERE predicate for idx_catalog_partial. | N/A |
| Catalog | pg_get_indexdef(oid) returned a concrete CREATE INDEX statement for idx_catalog_fn. | N/A |
| Catalog | Full and partial index metadata agreed across pg_get_indexdef, pg_indexes.indexdef, and pg_index.indpred for tested indexes. | N/A |
| Predicate | CREATE UNIQUE INDEX ... WHERE is_active allowed inactive duplicate user_id but rejected second active duplicate with duplicate unique key error. |  |
| Predicate | On localhost:5432, CREATE INDEX idx_partial_ok ON t_partial_ok (a) WHERE a > 1 executed successfully and pg_catalog.pg_indexes returned the idx_partial_ok metadata row. |  |
| Predicate | CREATE INDEX with predicate WHERE (a ^ 2) > 1 failed with conversion error "the power operator is not yet supported", and idx_partial_badexpr was absent from pg_catalog.pg_indexes (count=0). |  |
| Predicate | Planner uses the partial index only when predicate implication is valid. |  |
| Predicate | Batch failure is explicit and leaves detectable partial deployment state. |  |
| Predicate | Concurrent duplicate-name index creation produced one winner with clean catalog state. |  |
| Replica | REPLICA IDENTITY FULL executed without unsupported-command errors, and a follow-up ALTER TABLE ADD COLUMN succeeded. |  |
| Replica | A mixed ALTER TABLE failed on a later invalid clause, and schema inspection confirmed no partial column addition remained. |  |
Commit: 6da430f
Tell us how we did: Give Ito Feedback
Ito Diff Report ❌Tested: Across 10 test cases, the unified run had mixed results with 5 passing and 5 confirmed medium-severity failures: core tuple/record NULL handling now behaved consistently in the covered record-expression scenarios, but significant SQL and introspection defects remain. The key issues were lossy index-definition rendering in pg_get_indexdef/pg_indexes that truncates dotted expressions, ALTER TABLE ... REPLICA IDENTITY succeeding as a silent no-op with no relreplident change, and tuple IN path inconsistencies/unsafety where malformed operands can hit internal interface-conversion failure paths and equivalent subquery or VALUES forms fail with bool-comparison errors while direct tuple forms do not. ❌ Failures (2)
🟠 Index introspection truncates dotted expressions
Relevant code:
cols := make([]string, len(index.Expressions()))
for i, expr := range index.Expressions() {
split := strings.Split(expr, ".")
if len(split) > 1 {
cols[i] = split[1]
} else {
cols[i] = expr
}
}
cols := make([]string, len(index.Expressions()))
for i, expr := range index.Expressions() {
split := strings.Split(expr, ".")
if len(split) > 1 {
cols[i] = split[1]
} else {
cols[i] = expr
}
}🟠 Replica identity command silently performs no operation
Relevant code:
case *tree.AlterTableOwner:
unsupportedWarnings = append(unsupportedWarnings, fmt.Sprintf("ALTER TABLE %s OWNER TO %s", tableName.String(), cmd.Owner))
case *tree.AlterTableComputed:
return nil, nil, errors.New("This command does not currently support multiple actions in one statement")
case *tree.AlterTableSetStatistics:
// is unsupported and ignored
case *tree.AlterTableRowLevelSecurity:
// is unsupported and ignored
case *tree.AlterTableReplicaIdentity:
// is unsupported and ignored
default:
return nil, nil, errors.Errorf("ALTER TABLE with unsupported command type %T", cmd)
// If there are no valid statements return a no-op statement
if len(noOps) > 0 && len(statements) == 0 {
return NewNoOp(noOps...), nil
}
// Otherwise emit warnings now, then return an AlterTable statement
// TODO: we don't have a way to send or store the warnings alongside a valid AlterTable statement. We could either
// get a *sql.Context here and emit warnings, or we could store the warnings in the Context and make the caller
// emit them before it sends |ReadyForQuery|
return &vitess.AlterTable{
Table: tableName,
Statements: statements,
}, nil✅ Verified Passing (5)
↪️ Inherited from Prior Run (11)
|
| Origin | Category | Severity | Summary | Screenshot |
|---|---|---|---|---|
| 🆕 New | Record | 🟠 Medium | Unsupported IN right operand shape triggers an internal interface-conversion failure path instead of a controlled validation error. |  |
| 🆕 New | Record | 🟠 Medium | Equivalent tuple IN forms diverge: baseline tuple returns NULL, while IN-subquery variant fails with a comparison-bool error. |  |
| 🆕 New | Record | 🟠 Medium | Baseline and ROW tuple forms succeed, but VALUES-based equivalent form fails with an IN-subquery comparison-bool error. |  |
🟠 Unsafe composite cast on unsupported IN operand
- What failed: The unsupported-shape query should fail with a controlled operand-validation error, but instead enters a panic-style interface conversion path; a follow-up query still succeeds.
- Impact: Invalid tuple IN input can trigger internal-type failure behavior instead of predictable SQL-level validation errors. This degrades reliability and makes client-side error handling less safe for malformed or generated SQL.
- Steps to reproduce:
- Connect to Doltgres and open a SQL session.
- Run
SELECT (1, NULL) IN (1);. - Run
SELECT (1,NULL) IN ((1,NULL));to confirm the session continues after the first failure path.
- Stub / mock context: The run used a local Doltgres instance with deterministic test credentials and an auth-bypass environment setting for stable startup; SQL behavior was exercised against real engine code paths without API route mocking.
- Code analysis: I traced tuple IN execution through
InTupleevaluation into composite value comparison and found an unchecked type assertion in composite compare. The right operand can be a non-record value in this path, and the current compare implementation does not defensively reject it before assertion. - Why this is likely a bug: Production code allows a scalar right value to proceed and then performs an unchecked
[]RecordValuecast, which directly explains the observed internal interface-conversion failure path.
Relevant code:
server/types/type.go (lines 404-415)
case []RecordValue:
if !t.IsCompositeType() {
return 0, errors.New("record value received in Compare for non composite type")
}
bb := v2.([]RecordValue)
minLength := utils.Min(len(ab), len(bb))
for i := 0; i < minLength; i++ {
dgType, isDgType1 := ab[i].Type.(*DoltgresType)
otherDgType, isDgType2 := bb[i].Type.(*DoltgresType)
if !isDgType1 || !isDgType2 {server/expression/in_tuple.go (lines 107-114)
rightValues, ok := rightInterface.([]any)
if !ok {
// Tuples will return the value directly if it has a length of one, so we'll check for that first
if len(it.rightExpr) == 1 {
rightValues = []any{rightInterface}
} else {
return nil, errors.Errorf("%T: expected right child to return `%T` but returned `%T`", it, []any{}, rightInterface)
}
}🟠 Equivalent tuple IN forms diverge by evaluation path
- What failed: Equivalent query intent diverges by path: one form returns NULL while the IN-subquery form errors with
found equality comparison that does not return a bool. - Impact: Users can get inconsistent behavior for logically equivalent tuple filters depending on query form. This breaks SQL predictability for query generators that normalize predicates into subqueries.
- Steps to reproduce:
- Execute a NULL-containing baseline tuple IN query and note the result.
- Execute a logically equivalent query that routes through an IN subquery form.
- Compare outcomes and observe divergence between NULL result and comparison-bool error.
- Stub / mock context: The run used a local Doltgres instance with deterministic test credentials and an auth-bypass environment setting for stable startup; SQL behavior was exercised against real engine code paths without API route mocking.
- Code analysis: The AST path forces subquery RHS forms into
InSubquery, andInSubquery.WithChildrenfails whenever tuple equality does not compile to a bool-returning operator. UnlikeInTuple, this path does not apply the explicit unknown-type cast fallback, so equivalent expressions can fail only in the subquery-normalized route. - Why this is likely a bug: Production code routes equivalent IN syntax into different evaluator implementations and one implementation rejects valid tuple-comparison semantics with a hard error, causing deterministic, code-path-dependent divergence.
Relevant code:
server/ast/expr.go (lines 375-387)
case tree.In, tree.NotIn:
var innerExpression vitess.InjectedExpr
switch right := right.(type) {
case vitess.ValTuple:
innerExpression = vitess.InjectedExpr{
Expression: pgexprs.NewInTuple(),
Children: vitess.Exprs{left, right},
}
case *vitess.Subquery:
innerExpression = vitess.InjectedExpr{
Expression: pgexprs.NewInSubquery(),
Children: vitess.Exprs{left, right},
}server/expression/in_subquery.go (lines 214-222)
rightLiterals[i] = expression.NewLiteral(nil, rightType)
compFuncs[i] = framework.GetBinaryFunction(framework.Operator_BinaryEqual).Compile(ctx, "internal_in_comparison", leftLiteral, rightLiterals[i])
if compFuncs[i] == nil {
return nil, errors.Errorf("operator does not exist: %s = %s", leftType.String(), rightType.String())
}
if compFuncs[i].Type(ctx).(*pgtypes.DoltgresType).ID != pgtypes.Bool.ID {
// This should never happen, but this is just to be safe
return nil, errors.Errorf("%T: found equality comparison that does not return a bool", in)
}🟠 VALUES tuple form fails via subquery path
- What failed: Baseline and ROW variants complete with NULL semantics, but VALUES-based equivalent fails with IN-subquery bool-comparison error before consistent evaluation.
- Impact: SQL clients that emit VALUES-based or normalized subquery tuple forms can fail even when equivalent tuple predicates work in direct form. This creates compatibility gaps across query-builder output styles.
- Steps to reproduce:
- Run a baseline tuple IN query with NULL literals and confirm behavior.
- Run a ROW constructor variant and confirm it remains consistent.
- Run a VALUES-based equivalent IN form and observe the IN-subquery comparison-bool failure.
- Stub / mock context: The run used a local Doltgres instance with deterministic test credentials and an auth-bypass environment setting for stable startup; SQL behavior was exercised against real engine code paths without API route mocking.
- Code analysis: VALUES-based forms are treated as subquery right operands and go through
InSubquerycompilation checks that require bool-returning equality without the tuple-friendly fallback used by direct tuple IN handling. That implementation mismatch explains why semantically equivalent query forms diverge. - Why this is likely a bug: Equivalent tuple predicates should not fail solely due to VALUES/subquery representation, but production path selection currently causes that exact form-dependent failure.
Relevant code:
server/ast/expr.go (lines 383-387)
case *vitess.Subquery:
innerExpression = vitess.InjectedExpr{
Expression: pgexprs.NewInSubquery(),
Children: vitess.Exprs{left, right},
}server/expression/in_subquery.go (lines 219-222)
if compFuncs[i].Type(ctx).(*pgtypes.DoltgresType).ID != pgtypes.Bool.ID {
// This should never happen, but this is just to be safe
return nil, errors.Errorf("%T: found equality comparison that does not return a bool", in)
}Tell us how we did: Give Ito Feedback
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.