CSV import options for DELIMITER, NULL, and QUOTE #256

Greenscreen23 · 2025-07-28T15:21:28Z

This branch implements more csv parsing options. The goal is that the following sql string is correctly parsed:

COPY "{}" FROM '{}' WITH (FORMAT CSV, DELIMITER '|', NULL '', QUOTE '"');

where delimiter is the delimiter of the csv file, null is the value of null values in the csv and quote is the string used to quote values.

… following sql string is correctly parsed: ``` COPY "{}" FROM '{}' WITH (FORMAT CSV, DELIMITER '|', NULL '', QUOTE '"'); ``` where delimiter is the delimiter of the csv file, null is the value of null values in the csv and quote is the string used to quote values.

Greenscreen23 · 2025-07-28T15:24:00Z

Should bison check that the values of delimiter, null, and quote are only set if the format is CSV? This check would have to be done when the options are set (if they are set after the format) and when the format is set (if the values are set before the format). Additionally, this check may have to be done in the import/export statement (if no format is set at all), but we could leave that out (if we don't want to infer the data type from the file string).

dey4ss

Some initial comments. I guess it makes sense to prohibit CSV options for binary files accordung to Postgres (https://www.postgresql.org/docs/current/sql-copy.html) at some point (either in the parser or on the DBMS side), whatever is easier.

src/sql/ImportExportOptions.h

src/parser/bison_parser.y

…sv (or auto)

Greenscreen23 · 2025-08-02T16:32:57Z

Some initial comments. I guess it makes sense to prohibit CSV options for binary files accordung to Postgres (https://www.postgresql.org/docs/current/sql-copy.html) at some point (either in the parser or on the DBMS side), whatever is easier.

Sure, I've tried to implement something like that. Does this fit your needs?

dey4ss

Some more comments, mostly regarding readability

src/sql/CsvImportExportOptions.h

src/parser/bison_parser.y

dey4ss · 2025-08-04T14:11:55Z

test/queries/queries-bad.sql

 !SELECT * FROM t WHERE a = DATE '2000-01-01' + x DAYS;
 !SELECT * FROM t WHERE a = DATE '2000-01-01' + INTERVAL 'x' DAY;
 !SELECT * FROM t WHERE a = DATE '2000-01-01' + INTERVAL '3.3 DAYS';
+!COPY students FROM 'file_path' WITH (FORMAT TBL, DELIMITER '|', NULL '', QUOTE '"'); # Cannot have CSV options with non-CSV format


Can you please add further invalid cases (e.g., options passed multiple times, order of format and CSV options, ...)?

This is really necessary to ensure we clean up everything and do not leak when we call YYERROR. Please have a look at every path where you do that and add a case fot that, either in a full cpp test or here, so it will be triggered when we do leak checking.

Sure, I think I got every path (that is not identical to another path)

src/parser/bison_parser.y

dey4ss

Some more comments, still. IMO, the macro clutters the code too much

src/parser/bison_parser.y

test/queries/queries-bad.sql

…d testcases

test/queries/queries-good.sql

src/parser/bison_parser.y

…s/better-csv-import

dey4ss

Just some comments on style. Good to go apart from that.

dey4ss · 2025-09-15T12:12:54Z

src/parser/bison_parser.y

  }
  delete ($$);
 } <table_vec> <table_element_vec> <update_vec> <expr_vec> <order_vec> <stmt_vec>
+%destructor { free($$->second); delete ($$); } <csv_option_t>


Suggested change

%destructor { free($$->second); delete ($$); } <csv_option_t>

%destructor {

free($$->second);

delete ($$);

} <csv_option_t>

src/parser/bison_parser.y

dey4ss · 2025-09-15T12:23:58Z

src/sql/statements.cpp

+  switch (option->first) {
+    case CsvOptionType::Delimiter:
+      if (delimiter != nullptr) return false;
+      delimiter = option->second;
+      break;
+    case CsvOptionType::Null:
+      if (null != nullptr) return false;
+      null = option->second;
+      break;
+    case CsvOptionType::Quote:
+      if (quote != nullptr) return false;
+      quote = option->second;
+      break;
+  }


Suggested change

switch (option->first) {

case CsvOptionType::Delimiter:

if (delimiter != nullptr) return false;

delimiter = option->second;

break;

case CsvOptionType::Null:

if (null != nullptr) return false;

null = option->second;

break;

case CsvOptionType::Quote:

if (quote != nullptr) return false;

quote = option->second;

break;

}

switch (option->first) {

case CsvOptionType::Delimiter:

if (delimiter != nullptr) {

return false;

}

delimiter = option->second;

break;

case CsvOptionType::Null:

if (null != nullptr) {

return false;

}

null = option->second;

break;

case CsvOptionType::Quote:

if (quote != nullptr) {

return false;

}

quote = option->second;

break;

}

Here (and elsewhere): To compare != nullptr or not, that is the question. Hyrise's guidelines are quite clear about that:

Prefer if (object) over if (object != nullptr) or if (object.has_value()).

On the other hand, this is parser land. What dou you think, @Bouncner?

Whatever happens in the parser land, I'd stick to it (I actually like very explicit checks for pointers and optionals, but I lost the fight back in the days).

It's really mixed here (even within this PR, that's why it caught my attention). I actually like the more implicit form, but I don't want to start a long discussion here. Braces after if are a must, though.

Okay. Then use your recommended snipped? Looks good to me.

Bouncner

Zündable. But maybe still wait for @dey4ss to confirm. :)

dey4ss

🧨

Greenscreen23 changed the title ~~Better CSV import~~ CSV import options for DELIMITER, NULL, and QUOTE Jul 28, 2025

dey4ss reviewed Jul 29, 2025

View reviewed changes

src/sql/ImportExportOptions.h Outdated Show resolved Hide resolved

src/parser/bison_parser.y Outdated Show resolved Hide resolved

move csv options to separate struct and check whether the format is c…

062a6ec

…sv (or auto)

Greenscreen23 requested a review from dey4ss August 2, 2025 16:33

dey4ss reviewed Aug 4, 2025

View reviewed changes

src/parser/bison_parser.y Outdated Show resolved Hide resolved

Use single CSV option, simplify parsing and add more test cases

f7d7b9f

Greenscreen23 requested a review from dey4ss August 5, 2025 10:02

Add type specifier to std::pair

0110e59

dey4ss reviewed Aug 5, 2025

View reviewed changes

Implement destructor, switch, remove namespace prefix and add more ba…

14069d8

…d testcases

Greenscreen23 requested review from Bouncner and dey4ss August 25, 2025 18:24

Add addtitional anti testcase

9b4f431

Bouncner reviewed Aug 29, 2025

View reviewed changes

test/queries/queries-good.sql Show resolved Hide resolved

src/parser/bison_parser.y Show resolved Hide resolved

Add more good query examples

95f98b8

Bouncner reviewed Aug 29, 2025

View reviewed changes

src/parser/bison_parser.y Outdated Show resolved Hide resolved

Greenscreen23 added 3 commits August 30, 2025 17:34

Shorten csv option handling

9c638a7

Merge branch 'main' of https://github.com/hyrise/sql-parser into luka…

7d8171f

…s/better-csv-import

Move accept_csv_option to struct CsvOptions

31b8916

dey4ss reviewed Sep 15, 2025

View reviewed changes

Last changes

5166a80

Greenscreen23 requested review from Bouncner and dey4ss September 16, 2025 16:56

Bouncner approved these changes Sep 16, 2025

View reviewed changes

dey4ss approved these changes Sep 17, 2025

View reviewed changes

Greenscreen23 merged commit 3456f97 into hyrise:main Sep 17, 2025
4 checks passed

Greenscreen23 deleted the lukas/better-csv-import branch September 17, 2025 08:58

CSV import options for DELIMITER, NULL, and QUOTE #256

CSV import options for DELIMITER, NULL, and QUOTE #256

Uh oh!

Conversation

Greenscreen23 commented Jul 28, 2025

Uh oh!

Greenscreen23 commented Jul 28, 2025

Uh oh!

dey4ss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Greenscreen23 commented Aug 2, 2025

Uh oh!

dey4ss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dey4ss Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dey4ss Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Greenscreen23 Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dey4ss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dey4ss left a comment

Choose a reason for hiding this comment

Uh oh!

dey4ss Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dey4ss Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Bouncner Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

dey4ss Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Bouncner Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Bouncner left a comment

Choose a reason for hiding this comment

Uh oh!

dey4ss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

dey4ss Aug 4, 2025 •

edited

Loading

dey4ss Aug 4, 2025 •

edited

Loading