Skip to content

refactor(parser): more strict parser for format_options. #9635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 23 commits into from
Jan 17, 2023
Merged

refactor(parser): more strict parser for format_options. #9635

merged 23 commits into from
Jan 17, 2023
+152 −42

Conversation

youngsofun
Copy link
Member

@youngsofun youngsofun commented Jan 16, 2023 •
edited
Loading

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

the problem is described in #9626

add format_options, connection_options, credentails_options which are more strict.

  1. use keyword for option keys
  2. use keyword for option values of type and compression, but allow string literal for compatibility, forbidden ident
  3. check string/int/bool for literal values

Closes #issue

@vercel
Copy link

vercel bot commented Jan 16, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
databend ⬜️ Ignored (Inspect) Jan 17, 2023 at 5:56AM (UTC)

@mergify mergify bot added the pr-refactor this PR changes the code base without new features or bugfix label Jan 16, 2023
@youngsofun youngsofun mentioned this pull request Jan 16, 2023
Open
@youngsofun youngsofun marked this pull request as draft January 16, 2023 13:30
@youngsofun
Copy link
Member Author

youngsofun commented Jan 17, 2023 •
edited
Loading 

        // TODO: We will use `CONNECTION` to replace `CREDENTIALS` and `ENCRYPTION`.
            let mut conns = connection_opt.map(|v| v.2).unwrap_or_default();
            conns.extend(credentials_opt.map(|v| v.2).unwrap_or_default());
            conns.extend(encryption_opt.map(|v| v.2).unwrap_or_default());

cc @Xuanwo @sundy-li @BohuTANG

is it time to do it?

CREDENTIALS is used in doc

https://databend.rs/doc/sql-commands/dml/dml-copy-into-location#externallocation-for-amazon-s3

ENCRYPTION not seen in doc, only in unit test, we can rm it first?

      ENCRYPTION = (
                    MASTER_KEY = 'master_key'
                )

@youngsofun youngsofun marked this pull request as ready for review January 17, 2023 04:55
@youngsofun youngsofun requested a review from Xuanwo January 17, 2023 04:56
@Xuanwo
Copy link
Member

Xuanwo commented Jan 17, 2023

Let's do this!

@BohuTANG
Copy link
Member 


Differences (-left|+right):
 error: 
   --> SQL:1:38
   |
 1 | COPY INTO mytable FROM 's3://bucket' CREDENTIAL = ();
-  |                                      ^^^^^^^^^^ expected `CONNECTION`, `CREDENTIALS`, `LOCATION_PREFIX`, `FILES`, `PATTERN`,`FILE_FORMAT`, or 10 more ...
+  |                                      ^^^^^^^^^^ expected `CONNECTION`, `CREDENTIALS`, `LOCATION_PREFIX`, `FILES`, `PATTERN`, `FILE_FORMAT`, or 10 more ...

@BohuTANG BohuTANG merged commit 11b6a4f into databendlabs:main Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sundy-li sundy-li

@BohuTANG BohuTANG

@Xuanwo Xuanwo

Assignees
No one assigned
Labels
pr-refactor this PR changes the code base without new features or bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@youngsofun @Xuanwo @BohuTANG @sundy-li