-
Notifications
You must be signed in to change notification settings - Fork 454
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.
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
storage/copy-to-s3: Require format option #27158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not sure how I feel about this from a SQL perspective because it differs from the postgres default. Maybe instead of defaulting to CSV for that variant, require that it be set to something (i.e., no default)? SQL council should weigh in.
+1 making it a required option when the |
+1 from me too. |
74f0750
to
d34db04
Compare
Updated to make required instead of defaulted for |
@maddyblue mind a quick re-review? |
d34db04
to
194c164
Compare
Motivation
FORMAT
option should be required or default to supported value #27101Tips for reviewer
This leaves the default as TEXT for all other COPY statements, but requires the FORMAT option is provided for the copy-to-expr statement.
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.