apache / apache/datafusion-sqlparser-rs
COPY TO/FROM is overly restrictive on options
- Lenguaje dominante
- Rust
- Estrellas
- 3.5k
- Forks
- 772
- Merge medio
- 4 d 9 h
- PR fusionados (30 d)
- 17
Descripción
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)
Guía de contribución
No hay ninguna guía de contribución indexada para este repositorio
Línea de trabajo
Comienza con el análisis de las opciones de COPY en src/parser/mod.rs, en las líneas referenciadas, y con las definiciones de CopyOption y Statement::Copy en src/ast/mod.rs. Compara la representación actual con los ejemplos de opciones de PostgreSQL, DuckDB y DataFusion del issue, incluidas las claves entre comillas y los valores permisivos. Se considera terminado cuando las opciones de COPY son lo bastante genéricas para esos dialectos sin restringir innecesariamente las claves o los valores, y las opciones heredadas siguen contempladas.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- rust
- Área
- compilers
- Tipo de issue
- Error
- Dificultad
- 5/5
- Tiempo estimado
- Más de una semana
- Estado de actividad
- Estancado
- Claridad
- Bastante claro
- Aptitud para principiantes
- 30/100