Support the custom terminator for the CSV file format #12263

goldmedal · 2024-08-30T14:37:40Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Now, we can customize the record delimiter (terminator) when creating a dataframe like

let df = session_ctx
    .read_csv("memory:///", CsvReadOptions::new().terminator(Some(b'\r')))
    .await
    .unwrap();

The default value is None which means Arrow will use CRLF as the terminator.
https://docs.rs/csv-core/latest/csv_core/struct.ReaderBuilder.html#method.terminator

Are these changes tested?

yes

Are there any user-facing changes?

New API for the CSV Options.

korowa

Thank you @goldmedal

I've left some comments, and did I get it right that writing support is not impleented as it's not supported by arrow-csv (yet)?

korowa · 2024-09-01T14:24:34Z

datafusion/common/src/config.rs

@@ -1696,6 +1697,13 @@ impl CsvOptions {
        self
    }

+    /// The character that terminates a row.
+    /// - default to '\n'


If it's None as stated above -- won't it be CRLF by default (which is slightly more than \n according to csv_core docs used internally by arrow-csv)?

You're right. I missed modifying this from my first version design. Thanks.

korowa · 2024-09-01T14:25:28Z

datafusion/core/src/datasource/physical_plan/csv.rs

@@ -493,6 +517,7 @@ impl CsvConfig {

 impl CsvConfig {
    fn open<R: Read>(&self, reader: R) -> Result<csv::Reader<R>> {
+        dbg!(&self.terminator);


Should this line be cleaned up?

korowa · 2024-09-01T14:26:09Z

datafusion/core/src/datasource/physical_plan/csv.rs

@@ -529,6 +556,7 @@ impl CsvOpener {
        config: Arc<CsvConfig>,
        file_compression_type: FileCompressionType,
    ) -> Self {
+        dbg!(&config);


This one, probably, should be cleaned up too

Thanks, I missed to remove them 😢

korowa · 2024-09-01T14:29:55Z

datafusion/core/src/datasource/physical_plan/csv.rs

@@ -112,6 +114,7 @@ impl CsvExecBuilder {
            has_header: false,
            delimiter: b',',
            quote: b'"',
+            terminator: None,


I wonder if this option should have any interactions with newlines_in_values, or somehow affect execution of CSVExec (if newlines_in_values used anywhere to actually check input data -- I wasn't able to find such a usages for it, but still not sure).

I did some tests. I think that newlines_in_values won't be impacted by terminator. No matter how I change the terminator, the number of rows is the same.

let session_ctx = SessionContext::new(); let store = object_store::memory::InMemory::new(); let data = bytes::Bytes::from("a,b\r1,\"2\na\"\r3,\"4\nb\""); let path = object_store::path::Path::from("a.csv"); store.put(&path, data.into()).await.unwrap(); let url = Url::parse("memory://").unwrap(); session_ctx.register_object_store(&url, Arc::new(store)); let df = session_ctx .read_csv("memory:///", CsvReadOptions::new().newlines_in_values(true).terminator(Some(b'\r'))) .await .unwrap(); let result = df.collect().await.unwrap(); assert_eq!(result[0].num_rows(), 2);

korowa · 2024-09-01T14:31:24Z

datafusion/core/src/datasource/physical_plan/csv.rs

@@ -1210,6 +1244,44 @@ mod tests {
        crate::assert_batches_eq!(expected, &result);
    }

+    #[tokio::test]
+    async fn test_terminator() {


Perhaps there also should be added sqllogictests for reading files with customized line terminator (also checking that nothing is broken when terminator used along with newlines_in_values).

I got two issues when creating the sqllogictests case:

I prepared a test file breaking the line by CR. However, it seems that object store doesn't support this by default.

External error: query failed: DataFusion error: Object Store error: Generic LocalFileSystem error: Requested range was invalid

I tried to create an external table with the terminator config like

statement ok CREATE EXTERNAL TABLE stored_table_with_cf_terminator ( col1 TEXT, col2 TEXT ) STORED AS CSV LOCATION '../core/tests/data/cr_terminator.csv' OPTIONS ('format.terminator' '\r', 'format.has_header' 'true');

However, there are some issues for parsing the value.

External error: statement failed: DataFusion error: Invalid or Unsupported Configuration: Error parsing \r as u8. Non-ASCII string provided

It seems that \r is parsed to two different ASCII but it is one. 🤔
I tried to select \r directly

> select '\t'; +-----------+ | Utf8(" ") | +-----------+ | | +-----------+ 1 row(s) fetched. Elapsed 0.003 seconds.

It works fine but it's not valid for the config value. Is there any way to correctly create \r character for the config value?

Yes, it works only for single character -- probably ConfigField implementation for u8 should be enhanced, as right now it doesn't accept escape sequences like \t \n etc.

(same works for the delitmiter -- I wasn't able to read tab-separated file via SQL 🤔 )

To solve the ConfigField issue, I support EscapedStringLiteral in 8a1c9c7. Now, we can use E'\r' to create the terminator. I think it also works for E'\t' or E'\n'.

goldmedal · 2024-09-01T15:57:59Z

I've left some comments, and did I get it right that writing support is not impleented as it's not supported by arrow-csv (yet)?

Thanks @korowa for reviewing. I think the custom terminator for writing isn't supported by arrow-csv yet. I checked the doc of WriterBuilder and the source code in master branch. I can't find it.

goldmedal · 2024-09-03T17:14:41Z

datafusion/sqllogictest/test_files/csv_files.slt

+# TODO: It should be passed but got the error: External error: query failed: DataFusion error: Object Store error: Generic LocalFileSystem error: Requested range was invalid
+# query TT
+# select * from stored_table_with_cr_terminator;


I still got this error in the sqllogictests but I don't know why. The same case in datafusion/core/src/datasource/physical_plan/csv.rs works fine. I tried to find if there are any special configs for object_store in the sqllogictests but I found nothing. 🤔

I bet it is a bug and not related to this PR

What I think we should do is file a ticket tracking the issue and then merge this PR and work on figuring it out as a follow on.

Thanks for reminding this bug. However, I'm not sure what the root cause of this issue is, but I can help by drafting an issue and adding a reference to this test. You can edit it when you're available. WDYT?

I filed #12328 to trace this issue. Feel free to edit it or add more information.

alamb · 2024-09-03T20:33:32Z

FYI @Blizzara

alamb

Thank you @goldmedal and @korowa -- I think this PR is looking very nice now.

To get this merged, I suggest we file the ticket about sql level failure and add a reference to the test. I can do this over the next day or two if no one beats me to it

alamb · 2024-09-04T12:07:25Z

datafusion/core/src/datasource/physical_plan/csv.rs

+            .read_csv("memory:///", CsvReadOptions::new().terminator(Some(b'\n')))
+            .await.unwrap().collect().await {
+            Ok(_) => panic!("Expected error"),
+            Err(e) => assert_eq!(e.strip_backtrace(), "Arrow error: Csv error: incorrect number of fields for line 1, expected 2 got more than 2"),


FYI you can also use unwrap_err here as well

Something like

let e = session_ctx .read_csv("memory:///", CsvReadOptions::new().terminator(Some(b'\n'))) .await.unwrap().collect().await.unwrap_err(); assert_eq!(e.strip_backtrace(), "Arrow error: Csv error: incorrect number of fields for line 1, expected 2 got more than 2"),

Thanks! It looks better.

alamb · 2024-09-04T12:08:58Z

datafusion/sqllogictest/test_files/csv_files.slt

+# TODO: It should be passed but got the error: External error: query failed: DataFusion error: Object Store error: Generic LocalFileSystem error: Requested range was invalid
+# query TT
+# select * from stored_table_with_cr_terminator;


I bet it is a bug and not related to this PR

What I think we should do is file a ticket tracking the issue and then merge this PR and work on figuring it out as a follow on.

alamb · 2024-09-05T11:20:27Z

Thank you @goldmedal for filing #12328 👍

goldmedal · 2024-09-05T12:28:01Z

Thanks @korowa and @alamb for reviewing

goldmedal added 2 commits August 29, 2024 23:52

add terminator config to CsvConfig

dd1110b

add test and fix missing builder

b98980b

github-actions bot added core Core DataFusion crate common Related to common crate proto Related to proto crate labels Aug 30, 2024

goldmedal marked this pull request as ready for review August 30, 2024 15:04

korowa reviewed Sep 1, 2024

View reviewed changes

remove the debug message and fix the doc

f2ff11f

goldmedal added 2 commits September 4, 2024 01:07

support EscapedStringLiteral

8a1c9c7

add create external table tests

6116f9e

github-actions bot added sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Sep 3, 2024

goldmedal commented Sep 3, 2024

View reviewed changes

alamb approved these changes Sep 4, 2024

View reviewed changes

refactor the error assertion

7b60e54

goldmedal mentioned this pull request Sep 4, 2024

The file with non-standard newline character can't be read when sqllogictests testing #12328

Closed

add issue reference

4597f04

alamb merged commit 008c942 into apache:main Sep 5, 2024
24 checks passed

goldmedal deleted the feature/12215-csv-terminator branch September 5, 2024 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support the custom terminator for the CSV file format #12263

Support the custom terminator for the CSV file format #12263

goldmedal commented Aug 30, 2024 •

edited

Loading

korowa left a comment

korowa Sep 1, 2024

goldmedal Sep 1, 2024

korowa Sep 1, 2024

korowa Sep 1, 2024

goldmedal Sep 1, 2024

korowa Sep 1, 2024

goldmedal Sep 2, 2024

korowa Sep 1, 2024

goldmedal Sep 2, 2024

korowa Sep 2, 2024 •

edited

Loading

goldmedal Sep 3, 2024

goldmedal commented Sep 1, 2024

goldmedal Sep 3, 2024

alamb Sep 4, 2024

goldmedal Sep 4, 2024

goldmedal Sep 4, 2024

alamb commented Sep 3, 2024

alamb left a comment

alamb Sep 4, 2024

goldmedal Sep 4, 2024

alamb Sep 4, 2024

alamb commented Sep 5, 2024

goldmedal commented Sep 5, 2024

Support the custom terminator for the CSV file format #12263

Support the custom terminator for the CSV file format #12263

Conversation

goldmedal commented Aug 30, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

korowa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korowa Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldmedal commented Sep 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Sep 3, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Sep 5, 2024

goldmedal commented Sep 5, 2024

goldmedal commented Aug 30, 2024 •

edited

Loading

korowa Sep 2, 2024 •

edited

Loading