Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to SHOW TABLE STATUS #335

Merged
merged 8 commits into from
Mar 16, 2021
Merged

Conversation

VinaiRachakonda
Copy link
Contributor

No description provided.

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, just a few relatively minor issues. Check back if you have any questions

},
},
{
Query: `SHOW TABLE SATUS FROM mydb LIKE 'othertable'`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

enginetest/queries.go Show resolved Hide resolved
sql/core.go Outdated
@@ -259,6 +259,10 @@ type StatisticsTable interface {
Table
// NumRows returns the unfiltered count of rows contained in the table
NumRows(*Context) (uint64, error)
// NextAutoIncrementValue returns the next autoincrement value for a table and 0 if autoincrement does not apply.
NextAutoIncrementValue(ctx *Context) (int64, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be on AutoIncrementTable (or use the existing interface, should work fine)

case sqlparser.KeywordString(sqlparser.TABLE):
return parseShowTableStatus(ctx, query)
case "table status":
return parseShowTableStatus(ctx, s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use convert, not parse for these methods

default:
return nil, errUnexpectedSyntax.New("one of: FROM, IN, LIKE or WHERE", clause)
db := s.Database
var node sql.Node = plan.NewShowTableStatus(ctx.GetCurrentDatabase())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a weird way to express this. Set db to default to ctx.GetCurrentDatabase, then override it if set in the AST node. Then always set node = plan.NewShowTableStatus(db). Basically, keep the branching logic to the smallest possible unit of information in cases like this


tables, err = db.GetTableNames(ctx)
db, err := s.Catalog.Database(dbName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement sql.Databaser and set it in the analzyer, rather than getting it at runtime

return nil, err
}

aiVal, err := st.NextAutoIncrementValue(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I mentioned earlier, move this out of StatisticsTable and conditionally set this if the table implements AutoIncrementTable

@VinaiRachakonda VinaiRachakonda merged commit 59103b8 into master Mar 16, 2021
@Hydrocharged Hydrocharged deleted the vinai/show-table-status branch December 8, 2021 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants