apache / apache/datafusion-sqlparser-rs
COPY TO/FROM is overly restrictive on options
- Linguagem predominante
- Rust
- Estrelas
- 3.5k
- Forks
- 772
- Merge médio
- 4d 9h
- PRs com merge (30d)
- 17
Descrição
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)
Guia de contribuição
Nenhum guia de contribuição indexado para este repositório
Direção de pesquisa
Comece pelo parsing das opções de COPY em src/parser/mod.rs nas linhas referenciadas e pelas definições de CopyOption e Statement::Copy em src/ast/mod.rs. Compare a representação atual com os exemplos de opções de PostgreSQL, DuckDB e DataFusion na issue, incluindo chaves entre aspas e valores permissivos. Considera-se concluído quando as opções de COPY forem genéricas o suficiente para esses dialetos sem restringir desnecessariamente as chaves ou os valores, e as opções legadas continuarem sendo contempladas.
Escrita pelo modelo de indexação a partir do texto da issue.
Avaliação
- Stack de tecnologia
- rust
- Domínio
- compilers
- Tipo de issue
- Bug
- Dificuldade
- 5/5
- Tempo estimado
- Mais de uma semana
- Status de atividade
- Estagnada
- Clareza
- Razoavelmente clara
- Facilidade para iniciantes
- 30/100