Skip to content

Commit

Permalink
Engine empty array (#1388)
Browse files Browse the repository at this point in the history
* engine: fix empty array handling

* uncomment tests

* fix unit tests
  • Loading branch information
brennanjl authored Feb 18, 2025
1 parent 9069b70 commit 0d6a57e
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 21 deletions.
19 changes: 18 additions & 1 deletion app/node/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,24 @@ func StartCmd() *cobra.Command {

// we don't need to worry about order of priority with applying the extension
// flag configs because flags are always highest priority
cfg.Extensions = extConfs

// we merge the flags here because we don't want to totally delete all
// other extension flags. For example, if we have the extension
// "my_ext" configured with key "foo" and value "bar" in the config file,
// and we pass the flag "--extension.erc20.rpc=http://localhost:8545",
// we want to keep the "foo" key in the "my_ext" extension.
for extName, extConf := range extConfs {
existing, ok := cfg.Extensions[extName]
if !ok {
existing = make(map[string]string)
}

for k, v := range extConf {
existing[k] = v
}

cfg.Extensions[extName] = existing
}

bind.Debugf("effective node config (toml):\n%s", bind.LazyPrinter(func() string {
rawToml, err := cfg.ToTOML()
Expand Down
52 changes: 40 additions & 12 deletions node/engine/interpreter/interpreter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,19 +53,16 @@ func Test_SQL(t *testing.T) {
name string // name of the test
// array of sql statements, first element is the namespace, second is the sql statement
// they can begin with {namespace}sql, or just sql
sql []string
execSQL string // sql to return the results. Either this or execAction must be set
execVars map[string]any // variables to pass to the execSQL
results [][]any // table of results
err error // expected error, can be nil. Errors _MUST_ occur on the exec. This is a direct match
errContains string // expected error message, can be empty. Errors _MUST_ occur on the exec. This is a substring match
caller string // caller to use for the action, will default to defaultCaller
sql []string
execSQL string // sql to return the results. Either this or execAction must be set
execVars map[string]any // variables to pass to the execSQL
results [][]any // table of results
err error // expected error, can be nil. Errors _MUST_ occur on the exec. This is a direct match
errContains string // expected error message, can be empty. Errors _MUST_ occur on the exec. This is a substring match
caller string // caller to use for the action, will default to defaultCaller
skipInitTables bool // skip the creation of the users and posts tables
}

// this is for debugging.
// It helps me skip the users and posts table creation
skipInitTables := false

tests := []testcase{
{
name: "insert and select",
Expand Down Expand Up @@ -714,6 +711,18 @@ func Test_SQL(t *testing.T) {
execSQL: `ALTER TABLE tbl DROP column id;`,
err: engine.ErrCannotAlterPrimaryKey,
},
{
name: "select empty array (untyped)",
execSQL: `SELECT ARRAY[];`,
err: engine.ErrQueryPlanner,
skipInitTables: true,
},
{
name: "select empty array (type casted)",
execSQL: `SELECT ARRAY[]::TEXT[];`,
results: [][]any{{[]*string{}}},
skipInitTables: true,
},
}

db := newTestDB(t, nil, nil)
Expand All @@ -725,7 +734,7 @@ func Test_SQL(t *testing.T) {
require.NoError(t, err)
defer tx.Rollback(ctx) // always rollback

interp := newTestInterp(t, tx, test.sql, !skipInitTables)
interp := newTestInterp(t, tx, test.sql, !test.skipInitTables)

var values [][]any
err = interp.Execute(newEngineCtx(test.caller), tx, test.execSQL, test.execVars, func(v *common.Row) error {
Expand Down Expand Up @@ -1965,6 +1974,25 @@ func Test_Actions(t *testing.T) {
caller: "some_user",
err: engine.ErrDoesNotHavePrivilege,
},
{
name: "empty array",
stmt: []string{
`CREATE ACTION empty_array() public view {
$arr := array[];
}`,
},
action: "empty_array",
err: engine.ErrArrayDimensionality,
},
{
name: "empty array (typecasted)",
stmt: []string{
`CREATE ACTION empty_array() public view {
$arr := array[]::text[];
}`,
},
action: "empty_array",
},
}

db := newTestDB(t, nil, nil)
Expand Down
12 changes: 5 additions & 7 deletions node/engine/parse/antlr.go
Original file line number Diff line number Diff line change
Expand Up @@ -1786,8 +1786,9 @@ func (s *schemaVisitor) VisitUnary_sql_expr(ctx *gen.Unary_sql_exprContext) any
}

func (s *schemaVisitor) VisitMake_array_sql_expr(ctx *gen.Make_array_sql_exprContext) any {
e := &ExpressionMakeArray{
Values: ctx.Sql_expr_list().Accept(s).([]Expression),
e := &ExpressionMakeArray{}
if ctx.Sql_expr_list() != nil {
e.Values = ctx.Sql_expr_list().Accept(s).([]Expression)
}

if ctx.Type_cast() != nil {
Expand Down Expand Up @@ -2116,12 +2117,9 @@ func (s *schemaVisitor) VisitMake_array_action_expr(ctx *gen.Make_array_action_e
// directly below.
}

// we could enforce this in the parser, but it is not super intuitive,
// so we want to control the error message
if ctx.Action_expr_list() == nil {
s.errs.RuleErr(ctx, ErrSyntax, "cannot assign empty arrays. declare using `$arr type[];` instead`")
if ctx.Action_expr_list() != nil {
e.Values = ctx.Action_expr_list().Accept(s).([]Expression)
}
e.Values = ctx.Action_expr_list().Accept(s).([]Expression)

if ctx.Type_cast() != nil {
e.TypeCast = ctx.Type_cast().Accept(s).(*types.DataType)
Expand Down
1 change: 1 addition & 0 deletions node/engine/planner/logical/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ var (
ErrWindowNotDefined = errors.New("window not defined")
ErrFunctionDoesNotExist = errors.New("function does not exist")
ErrActionInSQLStmt = errors.New("actions cannot be used in SQL statements")
ErrUntypedEmptyArray = errors.New("cannot detect type for empty array")
)
8 changes: 7 additions & 1 deletion node/engine/planner/logical/planner.go
Original file line number Diff line number Diff line change
Expand Up @@ -1334,7 +1334,13 @@ func (s *scopeContext) exprWithAggRewrite(node parse.Expression, currentRel *Rel
}, field2)
case *parse.ExpressionMakeArray:
if len(node.Values) == 0 {
return nil, nil, false, fmt.Errorf("array constructor must have at least one element")
// if there are no values, we cannot determine the type unless
// it is type casted
if node.TypeCast == nil {
return nil, nil, false, fmt.Errorf("%w: Try type casting to the desired type, for example ARRAY[]::TEXT[]", ErrUntypedEmptyArray)
}

return cast(&ArrayConstructor{}, anonField(node.TypeCast))
}

var exprs []Expression
Expand Down
12 changes: 12 additions & 0 deletions node/engine/planner/logical/planner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,18 @@ func Test_Planner(t *testing.T) {
},
err: logical.ErrActionInSQLStmt,
},
{
name: "select empty array",
sql: "select array[]",
err: logical.ErrUntypedEmptyArray,
},
{
name: "select empty array with type",
sql: "select array[]::int[]",
wt: "Return: ?column? [int8[]]\n" +
"└─Project: []::int8\n" +
" └─Empty Scan\n",
},
}

for _, test := range tests {
Expand Down
24 changes: 24 additions & 0 deletions node/pg/db_live_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1847,3 +1847,27 @@ func Test_CancelListen(t *testing.T) {

require.Len(t, received, 10)
}

func Test_DeleteMe(t *testing.T) {
ctx := context.Background()

db, err := NewDB(ctx, cfg)
require.NoError(t, err)
defer db.Close()

tx, err := db.BeginTx(ctx)
require.NoError(t, err)
defer tx.Rollback(ctx) // always rollback

_, err = tx.Execute(ctx, "CREATE TABLE users (id serial primary key, name text)", QueryModeExec)
require.NoError(t, err)

ids := []any{}
res := []any{
&ids,
}
err = QueryRowFunc(ctx, tx, "SELECT array_agg(id) from users", res, func() error {
return nil
})
require.NoError(t, err)
}

0 comments on commit 0d6a57e

Please sign in to comment.