Enable functional index support#2642
Merged
Merged
Conversation
Contributor
|
Contributor
|
fulghum
force-pushed
the
fulghum/function_expr_index
branch
from
df4a5d9 to
f389aca
Compare
fulghum
force-pushed
the
fulghum/function_expr_index
branch
3 times, most recently
from
3a3e1cd to
cf03e18
Compare
fulghum
force-pushed
the
fulghum/function_expr_index
branch
from
4f849f0 to
1b44049
Compare
fulghum
force-pushed
the
fulghum/function_expr_index
branch
from
1b44049 to
7c0ccb2
Compare
Ito Test Report ❌13 test cases ran. 3 failed, 1 additional finding, 9 passed. Overall, the run showed good baseline stability and partial feature health—several expression-index and deep-expression scenarios completed successfully without server crashes, information_schema.columns correctly excluded hidden system columns, and enginetest CREATE/DROP index command-tag handling passed—but the aggregate result is not clean due to confirmed product bugs. The most important findings are high-severity expression-index contract mismatches where parser/analyzer acceptance still ends in execution-time “unsupported expression” failures (including mixed named+expression cases that suppress expected missing-column validation), plus inconsistent metadata exposure in pg_catalog.pg_indexes.indexdef leaking hidden internal backing names and a medium-severity SQL formatting bug where parenthesized cast expressions miss the intended implicit alias. ❌ Failed (3)
|
| Category | Summary | Screenshot |
|---|---|---|
| Formatting | No real bug reproduced. Reserved-word-like/case-sensitive identifiers remained unambiguous in output and executed successfully. | N/A |
| Formatting | Cast-expression auto aliases remained stable across quoted/case-sensitive identifier scenarios; equivalent forms produced consistent output and no semantic drift was observed in query execution. | N/A |
| Harness | Infrastructure blockage was resolved and TestIndexedExpressions passed, including CREATE/DROP lifecycle checks consistent with command-tag mapping support. |
 |
| Harness | Indexed-expression harness signal check passed; skipped queries were unsupported syntax only, while relevant lifecycle and behavioral checks executed successfully. |  |
| Index | CREATE INDEX on concat(v1,v1) executed successfully and idx_expr_ok was present in pg_indexes metadata. |  |
| Index | A deep concat expression index was created successfully and subsequent SELECT/pg_isready checks confirmed the server stayed responsive. |  |
| Index | A valid expression index traversed layers and executed successfully, while an invalid expression was rejected with a consistent analyzer-level column error; server remained responsive. |  |
| Validation | Syntactically valid expression-index DDL executed to backend and was rejected with clear deterministic error ('expression index attribute is not yet supported'); post-failure metadata remained clean with no index side effects. |  |
| Visibility | Confirmed information_schema.columns only returns user-visible columns and excludes hidden system columns. |
 |
ℹ️ Additional Findings (1)
These findings are unrelated to the current changes but were observed during testing.
| Category | Summary | Screenshot |
|---|---|---|
| Formatting | 🟠 Real bug confirmed: parenthesized cast expressions do not receive the intended implicit alias, and output falls back to full expression text. | N/A |
🟠 Parenthesized cast expression misses implicit alias
- What failed: The result label is emitted as the full expression text ("CaseSensitive"::TEXT) instead of deriving the implicit alias from the cast input expression.
- Impact: SQL clients and tooling that rely on stable implicit column labels receive inconsistent headers for equivalent cast expressions. This can break downstream query parsing or metadata matching workflows that assume PostgreSQL-like alias behavior.
- Steps to reproduce:
- Create a table with a quoted identifier column such as "CaseSensitive".
- Run SELECT ("CaseSensitive"::TEXT) FROM t_fmt_alias LIMIT 1 without an AS alias.
- Inspect the returned column header in the query output.
- Compare the label against the expected implicit alias derived from the cast input expression.
- Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code analysis: I inspected cast alias derivation in server/ast/select.go and expression conversion in server/ast/expr.go. The implicit alias branch runs only when expr is directly a *tree.CastExpr; parenthesized casts are represented as *tree.ParenExpr, so alias derivation is skipped and the fallback label remains the full expression text.
- Why this is likely a bug: The code path for implicit cast aliasing does not handle parenthesized casts, creating inconsistent labeling for semantically equivalent expressions in production SQL behavior.
Relevant code:
server/ast/select.go (lines 132-160)
vitessExpr, err := nodeExpr(ctx, expr)
if err != nil {
return nil, err
}
if ce, ok := expr.(*tree.CastExpr); ok && node.As == "" {
hasConst := false
_, _ = tree.SimpleVisit(expr, func(visitingExpr tree.Expr) (recurse bool, newExpr tree.Expr, err error) {
switch visitingExpr.(type) {
case tree.Constant:
hasConst = true
return false, visitingExpr, nil
}
return true, visitingExpr, nil
})server/ast/select.go (lines 156-165)
// cast type is not part of column name
// e.g. `id::INT2` should create column name as `id`.
node.As = tree.UnrestrictedName(tree.AsString(ce.Expr))
}
return &vitess.AliasedExpr{
Expr: vitessExpr,
As: vitess.NewColIdent(string(node.As)),
InputExpression: inputExpressionForSelectExpr(node),
}, nilserver/ast/expr.go (lines 735-742)
case *tree.ParenExpr:
expr, err := nodeExpr(ctx, node.Expr)
if err != nil {
return nil, err
}
return &vitess.ParenExpr{
Expr: expr,
}, nilCommit: 7c0ccb2
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.
Minor changes to enable functional index support and run Dolt's functional index enginetests.
There are three regressions reported in this PR. One is a valid issue where
CREATE TABLE ... INHERITScan't copy a table with a functional index. I'll tackle this one in a follow-up PR. There's another recovered panic that seems to have to do with polymorphic array handling and doesn't seem directly related to this PR. I tested the last regression listed (fromvaccum) and it's an issue with returning elements from a wrapped function, which also doesn't appear directly related to this PR.Related to #2298