Port tests in `csv_files.rs` to sqllogictest by Asura7969 · Pull Request #8251 · apache/datafusion

Asura7969 · 2023-11-17T14:06:59Z

Which issue does this PR close?

Closes #8197 .

Rationale for this change

What changes are included in this PR?

2 parts：

Add quote & escape for create external table in sql（The api method is already supported）
Port tests in csv_files.rs to sqllogictest

Are these changes tested?

Yes

Are there any user-facing changes?

# Conflicts: # datafusion/physical-plan/src/joins/hash_join_utils.rs

Asura7969 · 2023-11-17T14:08:45Z

datafusion/core/src/datasource/listing_table_factory.rs

                    .with_delimiter(cmd.delimiter as u8)
+                    .with_quote(cmd.options.get("quote").map_or(b'"', |x| x.as_bytes()[0]))
+                    .with_escape(cmd.options.get("escape").map_or(None, |x| Some(x.as_bytes()[0])))
                    .with_file_compression_type(file_compression_type),


I'm not sure if this is the best way, maybe we can open another issue to improve it

TBH I'd prefer to file a separate issue for integrating QUOTE & ESCAPE into CreateExternalTable pipeline. My concern here is that it probably will be confusing for end-users to pass read options into clause reserved for write options (and without any effect on writing CSV).

Asura7969 · 2023-11-17T14:13:00Z

datafusion/sqllogictest/test_files/csv_files.slt

+WITH HEADER ROW
+DELIMITER ','
+OPTIONS ('quote' '~')
+LOCATION '../../testing/data/csv/csv_custom_quote.csv';


This file is needed here, but I don't know how to create it.

The contents of the file(csv_custom_quote.csv) are as follows:

c1,c2
~id0~,~value0~
~id1~,~value1~
~id2~,~value2~
~id3~,~value3~
~id4~,~value4~
~id5~,~value5~
~id6~,~value6~
~id7~,~value7~
~id8~,~value8~
~id9~,~value9~

Asura7969 · 2023-11-17T14:14:33Z

datafusion/sqllogictest/test_files/csv_files.slt

+WITH HEADER ROW
+DELIMITER ','
+OPTIONS ('escape' '\')
+LOCATION '../../testing/data/csv/csv_custom_escape.csv';


This is the same

The contents of the file(csv_custom_escape.csv):

c1,c2
"id0","value\"0"
"id1","value\"1"
"id2","value\"2"
"id3","value\"3"
"id4","value\"4"
"id5","value\"5"
"id6","value\"6"
"id7","value\"7"
"id8","value\"8"
"id9","value\"9"

alamb · 2023-11-20T12:08:09Z

Thank you @Asura7969 . This PR appears to be failing CI, so marking it as draft until that is fixed.

Asura7969 · 2023-11-20T14:16:34Z

Thank you @Asura7969 . This PR appears to be failing CI, so marking it as draft until that is fixed.

The reason for this failure is that the test file is missing. Can you help me check test?

korowa · 2023-11-21T08:26:27Z

The reason for this failure is that the test file is missing. Can you help me check apache/arrow-testing#97?

I believe another way is to generate custom-delimited (or escaped) .csv while configuring test context -- reference example might be csv generation for joins.slt

Asura7969 · 2023-11-21T09:28:56Z

I believe another way is to generate custom-delimited (or escaped) .csv while configuring test context -- reference example might be csv generation for joins.slt

Thanks for your tip, I'll improve it

# Conflicts: # datafusion/core/tests/sql/mod.rs

Asura7969 · 2023-11-21T15:24:23Z

I believe another way is to generate custom-delimited (or escaped) .csv while configuring test context -- reference example might be csv generation for joins.slt

This PR also involves two attribute（escape and quote） configurations of the csv file,when creating a table using sql，For example：

CREATE EXTERNAL TABLE custom_escape (
    c1  VARCHAR DEFAULT NULL,
    c2  VARCHAR DEFAULT NULL
)
STORED AS CSV
WITH HEADER ROW
DELIMITER ','
OPTIONS ('escape' '\')  -- this
LOCATION '../core/tests/data/custom_escape.csv';

Using the API to create similar tables supports escape and quote, but SQL does not support it. Your tip may not cover the SQL method.

Thanks again for the tips.

Asura7969 · 2023-11-21T15:26:45Z

I created the test file under the path datafusion/core/tests/data/. I don’t know if it is correct.

korowa · 2023-11-21T18:39:20Z

I created the test file under the path datafusion/core/tests/data/. I don’t know if it is correct.

I suppose, it's more neat to use enable_testdir method of TextContext for all temporary test data.

Asura7969 · 2023-11-23T07:51:24Z

I suppose, it's more neat to use enable_testdir method of TextContext for all temporary test data.

I used enable_testdir method，thanks for your advice

Asura7969 · 2023-11-28T08:13:20Z

cc @alamb

alamb

Thanks @Asura7969 -- this code looks great to me. Also thank you very much for filing #8310

In order to avoid losing test coverage, I think we should fix #8310 before merging this one.

Is that something you are able to try @Asura7969 ? Conveniently, you already have written the test for the code in this PR :)

alamb · 2023-11-28T15:11:34Z

datafusion/sqllogictest/src/test_context.rs

                info!("Registering metadata table tables");
                register_metadata_tables(test_ctx.session_ctx()).await;
            }
+            "csv_files.slt" => {


Asura7969 · 2023-11-29T00:46:04Z

Is that something you are able to try @Asura7969 ? Conveniently, you already have written the test for the code in this PR :)

I'll try it this week

alamb · 2023-11-29T16:11:39Z

Is that something you are able to try @Asura7969 ? Conveniently, you already have written the test for the code in this PR :)

I'll try it this week

For anyone following along this is done in #8351

simicd · 2024-01-17T11:48:45Z

Thanks for implementing #8351 @Asura7969! Since all tests for #8197 have been ported & with #8885 being merged this draft PR here can be closed?

alamb · 2024-01-17T20:41:19Z

Thanks for implementing #8351 @Asura7969! Since all tests for #8197 have been ported & with #8885 being merged this draft PR here can be closed?

I agree -- thank you @simicd

Asura7969 added 15 commits November 8, 2023 17:04

Minor: Improve the document format of JoinHashMap

7afeb8b

Merge remote-tracking branch 'origin/main'

6332bec

# Conflicts: # datafusion/physical-plan/src/joins/hash_join_utils.rs

Merge remote-tracking branch 'origin/main'

cc5e0c7

# Conflicts: # datafusion/physical-plan/src/joins/hash_join_utils.rs

Merge remote-tracking branch 'origin/main'

a114310

Merge remote-tracking branch 'origin/main'

928c811

Merge remote-tracking branch 'origin/main'

839093e

Merge remote-tracking branch 'origin/main'

a836cde

Merge branch 'apache:main' into main

5648dc7

Merge branch 'apache:main' into main

a670409

Merge branch 'apache:main' into main

22894a3

Merge branch 'apache:main' into main

73a59d2

Merge branch 'apache:main' into main

46409c2

Merge branch 'apache:main' into main

8a86a4c

Merge branch 'apache:main' into main

cf5c584

Port tests in csv_files.rs to sqllogictest

737d73c

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Nov 17, 2023

Asura7969 commented Nov 17, 2023

View reviewed changes

Asura7969 added 3 commits November 17, 2023 23:09

Merge branch 'main' into csv_files

447eed2

fix: fmt

3492266

Merge branch 'apache:main' into main

62ae9b9

alamb marked this pull request as draft November 20, 2023 12:08

Asura7969 mentioned this pull request Nov 20, 2023

add csv custom quote(and escape) file apache/arrow-testing#97

Open

alamb mentioned this pull request Nov 20, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 20, 2023 #8274

Closed

7 tasks

Asura7969 added 2 commits November 20, 2023 23:43

Merge branch 'apache:main' into main

da02fa2

Merge branch 'apache:main' into main

d98eb2e

Asura7969 added 3 commits November 21, 2023 22:34

Merge branch 'apache:main' into main

79e7216

Merge branch 'main' into csv_files

fec6eea

# Conflicts: # datafusion/core/tests/sql/mod.rs

fix: test file

3a6e362

Asura7969 mentioned this pull request Nov 23, 2023

Add quote & escape for create external table #8310

Closed

fix: use enable_testdir

126b3c2

alamb marked this pull request as ready for review November 28, 2023 15:09

alamb mentioned this pull request Nov 28, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 27, 2023 #8329

Closed

8 tasks

alamb marked this pull request as draft November 28, 2023 15:11

alamb reviewed Nov 28, 2023

View reviewed changes

alamb closed this Jan 17, 2024

Conversation

Asura7969 commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Asura7969 Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

korowa Nov 21, 2023 • 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!

Asura7969 Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

Asura7969 Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 20, 2023

Uh oh!

Asura7969 commented Nov 20, 2023

Uh oh!

korowa commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Asura7969 commented Nov 21, 2023

Uh oh!

Asura7969 commented Nov 21, 2023

Uh oh!

Asura7969 commented Nov 21, 2023

Uh oh!

korowa commented Nov 21, 2023

Uh oh!

Asura7969 commented Nov 23, 2023

Uh oh!

Asura7969 commented Nov 28, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

Asura7969 commented Nov 29, 2023

Uh oh!

alamb commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simicd commented Jan 17, 2024

Uh oh!

alamb commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Asura7969 commented Nov 17, 2023 •

edited

Loading

korowa Nov 21, 2023 •

edited

Loading

korowa commented Nov 21, 2023 •

edited

Loading

alamb commented Nov 29, 2023 •

edited

Loading