diff --git a/.golangci.yml b/.golangci.yml index 7fdc6617..f3b2a607 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -50,7 +50,7 @@ formatters: goimports: # put imports beginning with prefix after 3rd-party packages; # it's a comma-separated list of prefixes - local-prefixes: + local-prefixes: - github.com/secretflow/scql linters: @@ -68,7 +68,7 @@ linters: linters: [govet] settings: staticcheck: - # All supported checks can be enabled with "all". + # All supported checks can be enabled with "all". # To disable checks, prefix them with a minus sign. # Example: [ "all", "-SA1000", "-SA1001"] checks: ["all", "-ST1000", "-ST1003", "-ST1016", "-ST1020", "-ST1021", "-ST1022", "-QF1003", "-QF1006", "-QF1008"] diff --git a/pkg/planner/core/preprocess.go b/pkg/planner/core/preprocess.go index bbd53083..8d558e87 100644 --- a/pkg/planner/core/preprocess.go +++ b/pkg/planner/core/preprocess.go @@ -60,6 +60,8 @@ const ( parentIsJoin // inRepairTable is set when visiting a repair table statement. inRepairTable + // inShowColumns is set when visiting SHOW COLUMNS/DESCRIBE statement. + inShowColumns ) // preprocessor is an ast.Visitor that preprocess @@ -88,6 +90,9 @@ func (p *preprocessor) Enter(in ast.Node) (out ast.Node, skipChildren bool) { p.flag |= inCreateOrDropTable //p.checkDropTableGrammar(node) case *ast.ShowStmt: + if node.Tp == ast.ShowColumns { + p.flag |= inShowColumns + } p.resolveShowStmt(node) case *ast.UnionSelectList: p.checkUnionSelectList(node) @@ -107,6 +112,10 @@ func (p *preprocessor) Leave(in ast.Node) (out ast.Node, ok bool) { //p.checkContainDotColumn(x) case *ast.DropTableStmt, *ast.AlterTableStmt, *ast.RenameTableStmt: p.flag &= ^inCreateOrDropTable + case *ast.ShowStmt: + if x.Tp == ast.ShowColumns { + p.flag &= ^inShowColumns + } case *ast.TableName: p.handleTableName(x) case *ast.Join: @@ -146,8 +155,9 @@ func (p *preprocessor) handleTableName(tn *ast.TableName) { } tn.Schema = model.NewCIStr(currentDB) } - if p.flag&inCreateOrDropTable > 0 { - // The table may not exist in create table or drop table statement. + if p.flag&(inCreateOrDropTable|inShowColumns) > 0 { + // The table may not exist in create/drop table statement. + // For SHOW COLUMNS, we don't need InfoSchema in preprocess, will query storage directly in executor. return } diff --git a/pkg/scdb/executor/ddl.go b/pkg/scdb/executor/ddl.go index 8d8c9246..44375094 100644 --- a/pkg/scdb/executor/ddl.go +++ b/pkg/scdb/executor/ddl.go @@ -199,13 +199,14 @@ func (e *DDLExec) executeCreateTable(s *ast.CreateTableStmt) (err error) { if len(lowerColumnNames) != len(sliceutil.SliceDeDup(lowerColumnNames)) { return fmt.Errorf("ddl.executeCreateTable: duplicate column names in table %s", tblName) } - for _, c := range s.Cols { + for i, c := range s.Cols { // TODO: fill description field result = tx.Create(&storage.Column{ - Db: dbName, - TableName: tblName, - ColumnName: c.Name.String(), - Type: c.Type, + Db: dbName, + TableName: tblName, + ColumnName: c.Name.String(), + OrdinalPosition: uint(i), + Type: strings.ToLower(c.Type), }) if result.Error != nil { return fmt.Errorf("ddl.executeCreateTable: %v", result.Error) diff --git a/pkg/scdb/executor/executor_test.go b/pkg/scdb/executor/executor_test.go index 48d3ec28..89a5edde 100644 --- a/pkg/scdb/executor/executor_test.go +++ b/pkg/scdb/executor/executor_test.go @@ -39,7 +39,6 @@ import ( "github.com/secretflow/scql/pkg/scdb/storage" "github.com/secretflow/scql/pkg/sessionctx" "github.com/secretflow/scql/pkg/sessionctx/stmtctx" - "github.com/secretflow/scql/pkg/types" ) type userAuth struct { @@ -1439,43 +1438,6 @@ func TestShow(t *testing.T) { } func TestDescribe(t *testing.T) { - is := infoschema.MockInfoSchema( - map[string][]*model.TableInfo{ - "da": { - { - Name: model.NewCIStr("t1"), - Columns: []*model.ColumnInfo{ - { - Name: model.NewCIStr("c1"), - FieldType: *types.NewFieldType(mysql.TypeString), - Comment: "string value", - }, - { - Name: model.NewCIStr("c2"), - FieldType: *types.NewFieldType(mysql.TypeLong), - Comment: "long value", - }, - }, - }, - }, - "db": { - { - Name: model.NewCIStr("t1"), - Columns: []*model.ColumnInfo{ - { - Name: model.NewCIStr("c1"), - FieldType: *types.NewFieldType(mysql.TypeString), - Comment: "string value", - }, - { - Name: model.NewCIStr("c2"), - FieldType: *types.NewFieldType(mysql.TypeLong), - Comment: "long value", - }, - }, - }, - }, - }) r := require.New(t) ctx := setupTestEnv(t) @@ -1496,28 +1458,30 @@ func TestDescribe(t *testing.T) { _, err = runSQL(ctx, `GRANT DESCRIBE ON db.* to bob`) r.NoError(err) + // Root creates db.t1 for bob to describe later + _, err = runSQL(ctx, `CREATE TABLE db.t1 (c1 int, c2 int) REF_TABLE=d1.t1 DB_TYPE='mysql'`) + r.NoError(err) + // switch to user alice r.NoError(switchUser(ctx, userAlice)) _, err = runSQL(ctx, `CREATE TABLE da.t1 (c1 int, c2 int) REF_TABLE=d1.t1 DB_TYPE='mysql'`) r.NoError(err) - // alice> - rt, err := Run(ctx, `DESCRIBE da.t1`, is) + // alice> DESCRIBE uses storage directly, no need for InfoSchema + rt, err := runSQL(ctx, `DESCRIBE da.t1`) r.NoError(err) r.Equal(int64(2), rt[0].GetShape().GetDim()[0].GetDimValue()) - _, err = Run(ctx, `DESCRIBE db.t1`, is) - r.Error(err) - // alice> (switch to user bob) r.NoError(switchUser(ctx, userBob)) - // bob> - rt, err = Run(ctx, `DESCRIBE db.t1`, is) + // bob> can describe db.t1 + rt, err = runSQL(ctx, `DESCRIBE db.t1`) r.NoError(err) r.Equal(int64(2), rt[0].GetShape().GetDim()[0].GetDimValue()) - _, err = Run(ctx, `DESCRIBE da.t1`, is) + // bob cannot describe da.t1 (no DESCRIBE privilege) + _, err = runSQL(ctx, `DESCRIBE da.t1`) r.Error(err) } @@ -1701,32 +1665,15 @@ func TestDescribeTable(t *testing.T) { r := require.New(t) ctx := setupTestEnv(t) - is := infoschema.MockInfoSchema( - map[string][]*model.TableInfo{ - "da": { - { - Name: model.NewCIStr("t1"), - Columns: []*model.ColumnInfo{ - { - Name: model.NewCIStr("c1"), - FieldType: *types.NewFieldType(mysql.TypeString), - Comment: "string value", - }, - { - Name: model.NewCIStr("c2"), - FieldType: *types.NewFieldType(mysql.TypeLong), - Comment: "long value", - }, - }, - }, - }, - }) - - // fail due to scdb not support explain stmt - _, err := Run(ctx, "EXPLAIN SELECT * from da.t1", is) - r.Error(err) + // Create database and table in storage + _, err := runSQL(ctx, `CREATE DATABASE da`) + r.NoError(err) + + _, err = runSQL(ctx, `CREATE TABLE da.t1 (c1 string, c2 int) REF_TABLE=d1.t1 DB_TYPE='mysql'`) + r.NoError(err) - result, err := Run(ctx, "DESCRIBE da.t1", is) + // DESCRIBE now queries storage directly, no need for InfoSchema + result, err := runSQL(ctx, "DESCRIBE da.t1") r.NoError(err) r.Equal(2, len(result)) @@ -1735,6 +1682,7 @@ func TestDescribeTable(t *testing.T) { r.Equal("Type", result[1].GetName()) r.Equal(2, len(result[1].GetStringData())) + // Type should be lowercase as stored in storage r.ElementsMatch([]string{"string", "int"}, result[1].GetStringData()) } diff --git a/pkg/scdb/executor/show.go b/pkg/scdb/executor/show.go index b8340507..896710ed 100644 --- a/pkg/scdb/executor/show.go +++ b/pkg/scdb/executor/show.go @@ -34,7 +34,6 @@ import ( "github.com/secretflow/scql/pkg/privilege" "github.com/secretflow/scql/pkg/scdb/storage" "github.com/secretflow/scql/pkg/sessionctx" - "github.com/secretflow/scql/pkg/table" "github.com/secretflow/scql/pkg/types" "github.com/secretflow/scql/pkg/util/chunk" "github.com/secretflow/scql/pkg/util/mathutil" @@ -427,16 +426,17 @@ func getAllDatabaseNames(ctx sessionctx.Context) ([]string, error) { } func (e *ShowExec) fetchShowColumns(ctx context.Context) error { - tb, err := e.getTable() - - if err != nil { - return errors.Trace(err) + if e.Table == nil { + return errors.New("table not found") } + dbName := e.Table.Schema.O + tableName := e.Table.Name.O + // check database is visible checker := privilege.GetPrivilegeManager(e.ctx) if checker != nil && e.ctx.GetSessionVars().User != nil { - ok, err := checker.DBIsVisible(e.ctx.GetSessionVars().ActiveRoles, e.DBName.O, mysql.DescribePriv) + ok, err := checker.DBIsVisible(e.ctx.GetSessionVars().ActiveRoles, dbName, mysql.DescribePriv) if err != nil { return fmt.Errorf("showExec.fetchShowColumns: %v", err) } @@ -445,45 +445,44 @@ func (e *ShowExec) fetchShowColumns(ctx context.Context) error { } } - for _, col := range tb.Cols() { - if e.Column != nil && e.Column.Name.String() != col.Name.String() { - continue - } + // Query columns directly from storage instead of using InfoSchema + db := e.ctx.GetSessionVars().Storage + var columns []storage.Column + query := db.Model(&storage.Column{}).Where(&storage.Column{ + Db: dbName, + TableName: tableName, + }) - tpStr, err := infoschema.FieldTypeString(col.FieldType) - if err != nil { - return fmt.Errorf("fail to convert mysql field type %v to scdb field type: %v", col.FieldType, err) - } + // If specific column is requested + if e.Column != nil { + query = query.Where("column_name = ?", e.Column.Name.String()) + } + // Order by ordinal_position + err := query.Order("ordinal_position").Find(&columns).Error + if err != nil { + return errors.Wrapf(err, "showExec.fetchShowColumns: failed to query columns") + } + + for _, col := range columns { // The FULL keyword causes the output to include the column collation and comments, // as well as the privileges you have for each column. if e.Full { e.appendRow([]interface{}{ - col.Name.String(), - tpStr, - col.Comment, + col.ColumnName, + col.Type, + col.Description, }) } else { e.appendRow([]interface{}{ - col.Name.String(), - tpStr, + col.ColumnName, + col.Type, }) } } return nil } -func (e *ShowExec) getTable() (table.Table, error) { - if e.Table == nil { - return nil, errors.New("table not found") - } - tb, err := e.is.TableByName(e.Table.Schema, e.Table.Name) - if err != nil { - return nil, err - } - return tb, nil -} - func (e *ShowExec) dbAccessDenied() error { user := e.ctx.GetSessionVars().User u := user.Username diff --git a/pkg/scdb/server/submit_handler.go b/pkg/scdb/server/submit_handler.go index d136a579..0274e2ec 100644 --- a/pkg/scdb/server/submit_handler.go +++ b/pkg/scdb/server/submit_handler.go @@ -158,9 +158,39 @@ func isQueryNeedInfoSchema(query string) (bool, error) { if err != nil { return false, err } - switch stmt.(type) { - case *ast.CreateUserStmt, *ast.DropTableStmt: + switch s := stmt.(type) { + case *ast.CreateUserStmt, *ast.DropUserStmt, *ast.AlterUserStmt: + // User management statements only operate on users table + // They use storage.CheckUserExist() directly, no need for InfoSchema return false, nil + case *ast.GrantStmt, *ast.RevokeStmt: + // GRANT and REVOKE statements have their own validation logic + // They directly query storage to check existence of db/table/columns + // No need to load complete InfoSchema + return false, nil + case *ast.CreateDatabaseStmt, *ast.DropDatabaseStmt: + // CREATE/DROP DATABASE only need to check database existence + // They use storage.CheckDatabaseExist() directly, no need for InfoSchema + return false, nil + case *ast.CreateTableStmt, *ast.DropTableStmt: + // CREATE/DROP TABLE only need to check existence + // They use CheckDatabaseExist() and CheckTableExist() directly, no need for InfoSchema + return false, nil + case *ast.ShowStmt: + // SHOW DATABASES, SHOW TABLES, SHOW GRANTS don't need InfoSchema + // SHOW COLUMNS can query storage.Column table directly + switch s.Tp { + case ast.ShowDatabases, ast.ShowTables, ast.ShowGrants, ast.ShowColumns: + return false, nil + default: + return true, nil + } + case *ast.ExplainStmt: + // DESCRIBE/DESC table is parsed as ExplainStmt with ShowStmt inside + if showStmt, ok := s.Stmt.(*ast.ShowStmt); ok && showStmt.Tp == ast.ShowColumns { + return false, nil + } + return true, nil default: return true, nil } diff --git a/pkg/scdb/server/submit_handler_test.go b/pkg/scdb/server/submit_handler_test.go new file mode 100644 index 00000000..ebc59600 --- /dev/null +++ b/pkg/scdb/server/submit_handler_test.go @@ -0,0 +1,228 @@ +// Copyright 2023 Ant Group Co., Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package server + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestIsQueryNeedInfoSchema(t *testing.T) { + r := require.New(t) + + testCases := []struct { + name string + query string + needSchema bool + shouldError bool + }{ + // GRANT statements - should NOT need InfoSchema + { + name: "GRANT on database level", + query: "GRANT CREATE ON db1.* TO alice", + needSchema: false, + }, + { + name: "GRANT on table level", + query: "GRANT CREATE ON db1.table1 TO alice", + needSchema: false, + }, + { + name: "GRANT with column level - PLAINTEXT", + query: "GRANT SELECT PLAINTEXT(col1, col2) ON db1.table1 TO alice", + needSchema: false, + }, + { + name: "GRANT with column level - PLAINTEXT_AFTER_JOIN", + query: "GRANT SELECT PLAINTEXT_AFTER_JOIN(device_number, month_id, day_id) ON scql_xxx.xxxx TO prov", + needSchema: false, + }, + { + name: "GRANT multiple privileges", + query: "GRANT CREATE, DROP, GRANT OPTION ON db1.* TO alice", + needSchema: false, + }, + + // REVOKE statements - should NOT need InfoSchema + { + name: "REVOKE on database level", + query: "REVOKE CREATE ON db1.* FROM alice", + needSchema: false, + }, + { + name: "REVOKE on table level", + query: "REVOKE DROP ON db1.table1 FROM alice", + needSchema: false, + }, + { + name: "REVOKE with column level", + query: "REVOKE SELECT PLAINTEXT_AFTER_AGGREGATE(col1) ON db1.table1 FROM alice", + needSchema: false, + }, + + // CREATE/DROP/ALTER USER - should NOT need InfoSchema + { + name: "CREATE USER", + query: `CREATE USER alice PARTY_CODE "party_A" IDENTIFIED BY "some_pwd"`, + needSchema: false, + }, + { + name: "CREATE USER with full options", + query: `CREATE USER alice PARTY_CODE 'party_alice' IDENTIFIED BY 'alice123' WITH '2023-08-23T15:46:16+08:00' 'signature' 'pubkey'`, + needSchema: false, + }, + { + name: "DROP USER", + query: "DROP USER alice", + needSchema: false, + }, + { + name: "DROP USER IF EXISTS", + query: "DROP USER IF EXISTS alice", + needSchema: false, + }, + { + name: "ALTER USER password", + query: `ALTER USER alice IDENTIFIED BY "new_password"`, + needSchema: false, + }, + { + name: "ALTER USER endpoint", + query: "ALTER USER alice WITH ENDPOINT 'engine-alice-host:port'", + needSchema: false, + }, + + // DROP TABLE - should NOT need InfoSchema + { + name: "DROP TABLE", + query: "DROP TABLE db1.table1", + needSchema: false, + }, + { + name: "DROP TABLE IF EXISTS", + query: "DROP TABLE IF EXISTS db1.table1", + needSchema: false, + }, + + // CREATE/DROP DATABASE - should NOT need InfoSchema + { + name: "CREATE DATABASE", + query: "CREATE DATABASE db1", + needSchema: false, + }, + { + name: "CREATE DATABASE IF NOT EXISTS", + query: "CREATE DATABASE IF NOT EXISTS test_db", + needSchema: false, + }, + { + name: "DROP DATABASE", + query: "DROP DATABASE db1", + needSchema: false, + }, + { + name: "DROP DATABASE IF EXISTS", + query: "DROP DATABASE IF EXISTS test_db", + needSchema: false, + }, + + // CREATE TABLE - should NOT need InfoSchema + { + name: "CREATE TABLE", + query: "CREATE TABLE db1.t1 (c1 int) REF_TABLE=d1.t1 DB_TYPE='mysql'", + needSchema: false, + }, + { + name: "CREATE TABLE IF NOT EXISTS", + query: "CREATE TABLE IF NOT EXISTS db1.t1 (c1 int, c2 string) REF_TABLE=d1.t1 DB_TYPE='mysql'", + needSchema: false, + }, + + // SHOW COLUMNS / DESCRIBE - should NOT need InfoSchema + { + name: "SHOW COLUMNS", + query: "SHOW COLUMNS FROM db1.table1", + needSchema: false, + }, + { + name: "SHOW FULL COLUMNS", + query: "SHOW FULL COLUMNS FROM db1.table1", + needSchema: false, + }, + { + name: "DESCRIBE table", + query: "DESCRIBE db1.table1", + needSchema: false, + }, + { + name: "DESC table", + query: "DESC db1.table1", + needSchema: false, + }, + + // Other statements - should NEED InfoSchema + { + name: "SELECT statement", + query: "SELECT * FROM db1.table1", + needSchema: true, + }, + + // Invalid SQL - should error + { + name: "Invalid SQL", + query: "INVALID SQL STATEMENT", + shouldError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + needSchema, err := isQueryNeedInfoSchema(tc.query) + + if tc.shouldError { + r.Error(err, "Expected error for query: %s", tc.query) + } else { + r.NoError(err, "Query: %s", tc.query) + r.Equal(tc.needSchema, needSchema, + "Query: %s\nExpected needSchema=%v, got %v", + tc.query, tc.needSchema, needSchema) + } + }) + } +} + +// TestIsQueryNeedInfoSchema_EdgeCases tests edge cases +func TestIsQueryNeedInfoSchema_EdgeCases(t *testing.T) { + r := require.New(t) + + // Empty query + _, err := isQueryNeedInfoSchema("") + r.Error(err, "Empty query should return error") + + // Query with comments + needSchema, err := isQueryNeedInfoSchema("/* comment */ GRANT CREATE ON db.* TO user") + r.NoError(err) + r.False(needSchema, "GRANT with comment should not need InfoSchema") + + // Case insensitive + needSchema, err = isQueryNeedInfoSchema("grant create on db.* to user") + r.NoError(err) + r.False(needSchema, "Lowercase GRANT should not need InfoSchema") + + needSchema, err = isQueryNeedInfoSchema("GRANT CREATE ON db.* TO user") + r.NoError(err) + r.False(needSchema, "Uppercase GRANT should not need InfoSchema") +}