apache / apache/datafusion-sqlparser-rs

COPY TO/FROM is overly restrictive on options

オープン
#1,085 コメント 4 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
Rust
スター
3.5k
フォーク
772
平均マージ
4日 9時間
マージ済み PR(30日)
17

説明

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)

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

調査の方向性

参照されている行の src/parser/mod.rs にある COPY オプションのパースと、src/ast/mod.rs にある CopyOption および Statement::Copy の定義から着手してください。Issue にある PostgreSQL、DuckDB、DataFusion のオプション例と現在の表現を比較し、引用符で囲まれたキーや制限の緩い値も含めて確認してください。COPY オプションがこれらの方言に対して十分に汎用的であり、キーや値を不必要に制限せず、従来のオプションも引き続き考慮されていれば完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
rust
領域
compilers
issue の種類
バグ
難易度
5/5
見積もり時間
1週間以上
活発さ
停滞
明瞭さ
おおむね明確
初心者へのやさしさ
30/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。