-
Notifications
You must be signed in to change notification settings - Fork 243
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
Make COPY
statement options compliant to Postgres
#247
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.
Only tiny things.
src/sql/ImportExportOptions.h
Outdated
|
||
namespace hsql { | ||
|
||
// Name unchanged for compatibility. |
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.
I don't get this comment.
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.
Dates back to when we added the COPY
statement and, with it, the ability to export tables. Even though we use these file types for import and export, we kept the name so people with forks don't have to rename stuff in their code.
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.
This thinking is also the reson why I added the encoding as another member of the statement rather than just passing the options object with both file type and encoding.
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.
What name would you have chosen?
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.
For the enum? In the grammar, we simply call it file type (so FileType
as enum name):
sql-parser/src/parser/bison_parser.y
Line 457 in 9a67d24
file_type : IDENTIFIER { |
I remember that it was a big discussion back then and some insisted on compatibility. Anyway, I think it's kinda late for that now.
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.
Can you briefly mention that we call it FileyType in the grammar, but won't adapt it here?
Not stating what the alternative is (nobody is aware of the probably old discussion) does not give away why the current name might not be perfect. :)
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.
Feel free to zünd afterwards.
While fiddling around with a Postgres instance, I noticed that our syntax for
COPY
statements is not 100% compliant to Postgres. In detail, we usedCOPY table_a FROM 'path/to/tbl' WITH FORMAT CSV;
, whereas Postgres usesCOPY table_a FROM 'path/to/tbl' WITH (FORMAT CSV);
– there are brackets around the options. Furthermore, theWITH
keyword is optional [1].The incorrect syntax was introduced in #139 (my bad 😕) and it slipped through the review back then. This PR makes the
COPY
statement compliant to Postgres again. Furthermore, I used the opportunity to add the encoding as another option. This allows us to apply any specified encoding for imported and exported tables in Hyrise.[1] https://www.postgresql.org/docs/16/sql-copy.html