cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
29.9k stars 3.78k forks source link

sql: parse roundtrip #30141

Closed maddyblue closed 6 years ago

maddyblue commented 6 years ago

Parse followed by Format is not idempotent: "WITH ident ( EXTRACT , INTERLEAVE , ident , ORDINALITY , ident ) AS ( IMPORT TABLE ident . FULL FROM INTERVAL ( PLACEHOLDER ) WITH ident = PLACEHOLDER ) , SCHEMA ( BOOL , BIGINT ) AS ( RESTORE ident FROM IS ) DELETE FROM ident . RETURNING . ILIKE * ORDER BY PRIMARY KEY TRIM . ident . GROUPING DESC RETURNING NOTHING" -> "WITH ident (\"extract\", interleave, ident, \"ordinality\", ident) AS (IMPORT TABLE ident.full FROM INTERVAL 'placeholder' WITH ident = 'placeholder') , schema (bool, \"bigint\") AS (RESTORE TABLE ident FROM 'is') DELETE FROM ident.returning.ilike ORDER BY PRIMARY KEY \"trim\".ident.grouping DESC RETURNING NOTHING" != "WITH ident (\"extract\", interleave, ident, \"ordinality\", ident) AS (IMPORT TABLE ident.full FROM INTERVAL 'placeholder' WITH ident = 'placeholder') , schema (bool, \"bigint\") AS (RESTORE TABLE ident FROM 'is' WITH ident = 'placeholder') DELETE FROM ident.returning.ilike ORDER BY PRIMARY KEY \"trim\".ident.grouping DESC RETURNING NOTHING"

maddyblue commented 6 years ago

Simplified to:

WITH
a AS (backup table t to $1 with a),
y  AS (RESTORE TABLE u FROM $1)
select 1;

Also:

WITH
a AS (RESTORE TABLE u FROM $1),
y  AS (IMPORT TABLE t FROM csv $1 WITH i)
select 1;

Problem: the WITH options are being copied from the import/backup to the restore. I can't get any other combination to produce the bug (two restores, backup+import).

knz commented 6 years ago

Wow nice find.

maddyblue commented 6 years ago

Yeah this is so weird I don't even know where to look but it feels like a pointer is being duplicated somewhere? Maybe WITH is doing something bad? I have no idea.

maddyblue commented 6 years ago

This just gets weirder and weirder.

Works fine:

WITH 
 a AS (backup table t to $1 with a = 'aoe', c = 'oaeu')
,b AS (backup table t to $1 with a)
,y AS (RESTORE TABLE u FROM $1 )
select 1;

Starting to add yacc to the suspect list.

knz commented 6 years ago

I think I found it: the empty rules in opt_with_options and opt_with_clause should not be {} but instead {$$.val = nil}.

Can you try and see what happens?

Maybe we should also audit the other empty clauses throughout the grammar.

jordanlewis commented 6 years ago

Ugh... that's my fault. Sorry :( I didn't know that you had to explicitly nil stuff out!

knz commented 6 years ago

a yacc-generated parser is a giant switch in a loop and the data structure is a stack implemented as indexes into an array. Each occurrence of $$.val is really thestack[curpos].val. Unless you nil things out the previous value remains.

(That's true of all yacc generators that don't provide destructors for values. Bison and Lemon provide destructors, for example, go-yacc and old Bison don't)

knz commented 6 years ago

I'll make the patch!