Skip to content

Commit

Permalink
Merge #96572 #96741
Browse files Browse the repository at this point in the history
96572: parser: no-op if QUOTE '"' is specified in COPY r=otan a=otan

Release note (sql change): `COPY ... FROM ... QUOTE '"'` will no longer error.

Epic: None

Informs: #85574

Release justification: fixes usability issues with third party tools that is really a no-op.

96741: sqlsmith: disable crdb_internal.hide_sql_constants r=rharding6373 a=rharding6373

Informs: #96555
Epic: None

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: rharding6373 <rharding6373@users.noreply.github.com>
  • Loading branch information
3 people committed Feb 7, 2023
3 parents 247a543 + 4fcad84 + 0529c92 commit 60c6293
Show file tree
Hide file tree
Showing 8 changed files with 47 additions and 3 deletions.
1 change: 1 addition & 0 deletions docs/generated/sql/bnf/stmt_block.bnf
Original file line number Diff line number Diff line change
Expand Up @@ -2002,6 +2002,7 @@ copy_options ::=
| 'DELIMITER' string_or_placeholder
| 'NULL' string_or_placeholder
| 'HEADER'
| 'QUOTE' 'SCONST'

db_object_name_component ::=
name
Expand Down
3 changes: 3 additions & 0 deletions pkg/internal/sqlsmith/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,9 @@ var functions = func() map[tree.FunctionClass]map[oid.Oid][]function {
"crdb_internal.revalidate_unique_constraint",
"crdb_internal.request_statement_bundle",
"crdb_internal.set_compaction_concurrency",
// TODO(96555): Temporarily disable crdb_internal.hide_sql_constants,
// which produces internal errors for some valid inputs.
"crdb_internal.hide_sql_constants",
} {
skip = skip || strings.Contains(def.Name, substr)
}
Expand Down
10 changes: 10 additions & 0 deletions pkg/sql/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util"
"github.com/cockroachdb/cockroach/pkg/util/buildutil"
"github.com/cockroachdb/cockroach/pkg/util/encoding/csv"
"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/mon"
"github.com/cockroachdb/cockroach/pkg/util/retry"
Expand Down Expand Up @@ -192,6 +193,15 @@ func newCopyMachine(
return nil, pgerror.Newf(pgcode.FeatureNotSupported, "HEADER only supported with CSV format")
}

if n.Options.Quote != nil {
if c.format != tree.CopyFormatCSV {
return nil, pgerror.Newf(pgcode.FeatureNotSupported, "QUOTE only supported with CSV format")
}
if n.Options.Quote.RawString() != `"` {
return nil, unimplemented.NewWithIssuef(85574, `QUOTE value %s unsupported`, n.Options.Quote.RawString())
}
}

exprEval := c.p.ExprEvaluator("COPY")
if n.Options.Delimiter != nil {
if c.format == tree.CopyFormatBinary {
Expand Down
13 changes: 12 additions & 1 deletion pkg/sql/copy/testdata/copyfrom
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,17 @@ exec-ddl
CREATE TABLE t (a int)
----

# copy quote without CSV not allowed.
copy-error
COPY t FROM STDIN QUOTE '"'
----
ERROR: QUOTE only supported with CSV format (SQLSTATE 0A000)

copy-error
COPY t FROM STDIN QUOTE 'x'
----
ERROR: QUOTE only supported with CSV format (SQLSTATE 0A000)

# builtins not allowed
copy-error
COPY t FROM STDIN
Expand All @@ -10,7 +21,7 @@ random()
ERROR: could not parse "random()" as type int: strconv.ParseInt: parsing "random()": invalid syntax (SQLSTATE 22P02)

copy
COPY t FROM STDIN
COPY t FROM STDIN QUOTE '"' CSV
----
0

Expand Down
1 change: 0 additions & 1 deletion pkg/sql/parser/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,6 @@ func TestUnimplementedSyntax(t *testing.T) {
{`COPY t FROM STDIN OIDS`, 41608, `oids`, ``},
{`COPY t FROM STDIN FREEZE`, 41608, `freeze`, ``},
{`COPY t FROM STDIN ENCODING 'utf-8'`, 41608, `encoding`, ``},
{`COPY t FROM STDIN QUOTE 'x'`, 41608, `quote`, ``},
{`COPY t FROM STDIN FORCE QUOTE *`, 41608, `quote`, ``},
{`COPY t FROM STDIN FORCE NULL *`, 41608, `force null`, ``},
{`COPY t FROM STDIN FORCE NOT NULL *`, 41608, `force not null`, ``},
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/parser/sql.y
Original file line number Diff line number Diff line change
Expand Up @@ -3977,7 +3977,7 @@ copy_options:
}
| QUOTE SCONST
{
return unimplementedWithIssueDetail(sqllex, 41608, "quote")
$$.val = &tree.CopyOptions{Quote: tree.NewStrVal($2)}
}
| ESCAPE SCONST error
{
Expand Down
8 changes: 8 additions & 0 deletions pkg/sql/parser/testdata/copy
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ COPY t (a, b, c) FROM STDIN -- fully parenthesized
COPY t (a, b, c) FROM STDIN -- literals removed
COPY _ (_, _, _) FROM STDIN -- identifiers removed

parse
COPY t FROM STDIN QUOTE '"'
----
COPY t FROM STDIN WITH QUOTE '"' -- normalized!
COPY t FROM STDIN WITH QUOTE ('"') -- fully parenthesized
COPY t FROM STDIN WITH QUOTE '_' -- literals removed
COPY _ FROM STDIN WITH QUOTE '"' -- identifiers removed

parse
COPY crdb_internal.file_upload FROM STDIN WITH destination = 'filename'
----
Expand Down
12 changes: 12 additions & 0 deletions pkg/sql/sem/tree/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type CopyOptions struct {
Null Expr
Escape *StrVal
Header bool
Quote *StrVal
}

var _ NodeFormatter = &CopyOptions{}
Expand Down Expand Up @@ -102,6 +103,11 @@ func (o *CopyOptions) Format(ctx *FmtCtx) {
maybeAddSep()
ctx.WriteString("HEADER")
}
if o.Quote != nil {
maybeAddSep()
ctx.WriteString("QUOTE ")
ctx.FormatNode(o.Quote)
}
}

// IsDefault returns true if this struct has default value.
Expand Down Expand Up @@ -145,6 +151,12 @@ func (o *CopyOptions) CombineWith(other *CopyOptions) error {
if other.Header {
o.Header = true
}
if other.Quote != nil {
if o.Quote != nil {
return pgerror.Newf(pgcode.Syntax, "quote option specified multiple times")
}
o.Quote = other.Quote
}
return nil
}

Expand Down

0 comments on commit 60c6293

Please sign in to comment.