apache / apache/datafusion-sqlparser-rs

COPY TO/FROM is overly restrictive on options

Offen
#1,085 4 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
Vorherrschende Sprache
Rust
Sterne
3.5k
Forks
772
Ø Merge
4 T. 9 Std.
Gemergte PRs (30 T.)
17

Beschreibung

Current parsing of `COPY TO/FROM` options is too restrictive on the how the keys & values can be provided.

https://github.com/sqlparser-rs/sqlparser-rs/blob/ce498864dc705f72e9f85d4dc5f7eba3d17b9ef6/src/parser/mod.rs#L5157-L5180

Given this parsing code, the expected format is:

```sql
COPY (SELECT 1 AS a, 2 AS b) TO 'file'
WITH (
FORMAT 'text',
FREEZE true,
DELIMITER 'd',
NULL 'null',
HEADER true,
QUOTE 'q',
ESCAPE 'e',
FORCE_QUOTE (a, b),
FORCE_NOT_NULL (a, b),
FORCE_NULL (a, b),
ENCODING 'encoding'
)
```

The problems:

- Key must be a keyword and cannot be a quoted string, even though postgres allows this, i.e. `WITH ("format" 'text', ...)`
- It strictly limits the key set at parsing time, leading to further problems:
- Need to update this parser whenever postgres supports a new key, e.g. DEFAULT (supported in postgres 16: https://www.postgresql.org/docs/16/sql-copy.html)
- We can't use this code for other databases/engines with a similar copy statement as they may have different supported keys (e.g. [duckdb](https://duckdb.org/docs/sql/statements/copy.html) and datafusion, see #1080)
- Some values are too strict in their format:
- FORMAT expects an identifier, which disallows escaped strings (`format e'csv'`), but this is allowed by postgres
- FREEZE expects keywords `true` or `false` but postgres allows these to be passed as string literals (single quoted, double quoted, escaped), e.g. `freeze 'true'`
- Same for HEADER

**Solutions**

We could fix these problems for Postgres specifically to make it more permissive, but I was hoping to also fix it to allow usage by other engine dialects (e.g. duckdb and datafusion). This might involve removing the `CopyOption` enum entirely:

https://github.com/sqlparser-rs/sqlparser-rs/blob/ce498864dc705f72e9f85d4dc5f7eba3d17b9ef6/src/ast/mod.rs#L4707-L4736

And replacing its usage in `Statement::Copy` with a more generic version that allows any string key with a new value enum to represent different values (e.g. string value, parenthesized column list value, number value):

https://github.com/sqlparser-rs/sqlparser-rs/blob/ce498864dc705f72e9f85d4dc5f7eba3d17b9ef6/src/ast/mod.rs#L1466-L1478

I'm not sure how the legacy options will fit into this, it could just be left as is and probably tackled as part of another ticket (as the original reason here was for interop with datafusion copy statement)

Beitragsleitfaden

Für dieses Repository ist kein Beitragsleitfaden indexiert

Rechercherichtung

Beginne mit dem Parsen der COPY-Optionen in src/parser/mod.rs an den referenzierten Zeilen sowie mit den Definitionen von CopyOption und Statement::Copy in src/ast/mod.rs. Vergleiche die aktuelle Darstellung mit den Optionsbeispielen für PostgreSQL, DuckDB und DataFusion im Issue, einschließlich in Anführungszeichen gesetzter Schlüssel und freizügiger Werte. Als erledigt gilt die Aufgabe, wenn COPY-Optionen generisch genug für diese Dialekte sind, ohne Schlüssel oder Werte unnötig einzuschränken, und die Legacy-Optionen weiterhin berücksichtigt werden.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
rust
Bereich
compilers
Issue-Typ
Bug
Schwierigkeit
5/5
Geschätzter Aufwand
Über eine Woche
Aktivitätsstatus
Veraltet
Klarheit
Größtenteils klar
Anfängerfreundlichkeit
30/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.