importccl: add support for SQLFns in pgdump #50850
Conversation
|
This change is |
|
RFAL @pbardea @Anzoteh96 |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Anzoteh96, @dt, @mjibson, @otan, and @pbardea)
pkg/ccl/importccl/read_import_pgdump.go, line 400 at r2 (raw file):
case *tree.Select: switch sel := stmt.Select.(type) { case *tree.SelectClause:
should this have a default counterpart for unsupported?
pkg/ccl/importccl/read_import_pgdump.go, line 403 at r2 (raw file):
for _, selExpr := range sel.Exprs { switch expr := selExpr.Expr.(type) { case *tree.FuncExpr:
ditto, what if it is something else?
pkg/ccl/importccl/read_import_pgdump.go, line 414 at r2 (raw file):
fn := ov.SQLFn if fn == nil { continue
huh, in what cases do we get here / why are they okay to ignore?
There was a problem hiding this comment.
I've added some comments regarding what to do about non schema-transforming SELECTs, and how we just ignore them. This was previously the implicit policy, and I'm making it more explicit with these comments. Since (AFAIK) no users have complained about IMPORT PGDUMP not executing their SELECTs (except in this geospatial case), I think it's ok to ignore them. Thoughts?
Reviewable status:
pkg/ccl/importccl/read_import_pgdump.go, line 400 at r2 (raw file):
Previously, dt (David Taylor) wrote…
should this have a
defaultcounterpart for unsupported?
I don't know? We were already ignoring any SELECTs, so I don't see why we would disallow them in this commit.
pkg/ccl/importccl/read_import_pgdump.go, line 403 at r2 (raw file):
Previously, dt (David Taylor) wrote…
ditto, what if it is something else?
Same as above: we already were ignoring these. If it is decided to care about erroring on SELECTs on other unsupported things, those should be added and tested in another PR.
pkg/ccl/importccl/read_import_pgdump.go, line 414 at r2 (raw file):
Previously, dt (David Taylor) wrote…
huh, in what cases do we get here / why are they okay to ignore?
A non-sqlfn is ignored just like they were before this pr.
| semaCtx := tree.MakeSemaContext() | ||
| _, err := expr.TypeCheck(ctx, &semaCtx, nil /* desired */) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Personal preference (take with a grain of salt :) ), I would condense this to
if _, err := expr.TypeCheck(ctx, &semaCtx, nil); err != nil {
return err
}
|
bors r+ |
Build failed |
SQLFns are functions users can call from SQL that create and execute SQL. AddGeometryColumn, for example, creates an ADD COLUMN statement and executes it. Add support for these in IMPORT PGDUMP by detecting them, running them, and feeding them back in to the table statement reader. Release note (sql change): add support for AddGeometryColumn and other functions that mutate schema in IMPORT PGDUMP.
|
bors r+ |
Build succeeded |
SQLFns are functions users can call from SQL that create and execute
SQL. AddGeometryColumn, for example, creates an ADD COLUMN statement and
executes it. Add support for these in IMPORT PGDUMP by detecting them,
running them, and feeding them back in to the table statement reader.
Release note (sql change): add support for AddGeometryColumn and other
functions that mutate schema in IMPORT PGDUMP.