Skip to content

Add COPY for import and export #139

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

Merged
merged 11 commits into from
Jan 8, 2020
Merged

Add COPY for import and export #139

merged 11 commits into from
Jan 8, 2020

Conversation

dey4ss
Copy link
Member

@dey4ss dey4ss commented Jan 6, 2020

@@ -215,6 +218,7 @@ int yyerror(YYLTYPE* llocp, SQLParserResult* result, yyscan_t scanner, const cha
%type <group_t> opt_group
%type <alias_t> opt_table_alias table_alias opt_alias alias
%type <with_description_t> with_description
%type <import_type_t> opt_file_type import_file_type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you harmonize the mix of file_type, import_file_type, and import_type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I myself would prefer using FileType instead of ImportType in general for both Import and Export statement (so it would sense for both). I just thought renaming it could be a thing regarding downwards compatibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in that case, please add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I get it right and renaming ImportType to FileType and altering its values to kFileCSV, ... with just adding a comment there that this has been done is okay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was to leave it as it, but add comments where appropriate that this is done for compat reasons

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, are those two comments enough?

* COPY students TO 'test/students.tbl' (WITH FORMAT TBL)
******************************/
export_statement:
COPY table_name TO file_path opt_file_type {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this uses opt_file_type, too, it is weird that opt_file_type has a import_file_type in an export context

@@ -76,10 +76,11 @@ VALUES
DIRECT
SORTED

COPY
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this file serve a purpose?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, I just thought it is somehow common to update it

namespace hsql {
// Represents SQL Export statements.
struct ExportStatement : SQLStatement {
ExportStatement(ImportType type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImportType?

@mrks mrks merged commit e3cfc80 into hyrise:master Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants