bump sqllogictest to 0.9.0 by xxchan · Pull Request #4547 · apache/datafusion

xxchan · 2022-12-07T20:54:33Z

bump sqllogictest & fix breaking changes: https://github.com/risinglightdb/sqllogictest-rs/blob/HEAD/CHANGELOG.md
workaround NULL normalization, close Implement null / empty string handling for sqllogictest #4500

xxchan · 2022-12-07T21:02:19Z

datafusion/core/tests/sqllogictests/src/main.rs

-        let builder = WriterBuilder::new().has_headers(false).with_delimiter(b' ');
+        let builder = WriterBuilder::new()
+            .has_headers(false)
+            .with_delimiter(b'\t');
        let mut writer = builder.build(&mut bytes);
        for batch in batches {
            writer.write(&normalize_batch(batch)).unwrap();
        }
    }
-    Ok(String::from_utf8(bytes).unwrap())
+    let res = String::from_utf8(bytes).unwrap();
+    let rows = res
+        .lines()
+        .map(|s| {
+            s.split("\t")
+                .map(|s| {
+                    if s.is_empty() {
+                        "NULL".to_string()
+                    } else {
+                        s.to_string()
+                    }
+                })
+                .collect()
+        })
+        .collect();


Using tab-separated CSV to get a row as Vec<String> is a little bit hacky, but it works... 😄

BTW here's some information about separator in test files risinglightdb/sqllogictest-rs#109

Thank you @xxchan -- this is great! The new interface makes a lot of sense. Thank you for the PR!

Given what I see here, I think there may be some better ways to format the output into Vec<Vec<String>> than re-parsing CSV, by using arrow directly (for example how it is done in https://docs.rs/arrow/28.0.0/arrow/util/pretty/index.html)

I'll give it a think and see if I can find some way to update it

cc @mvanschellebeeck

Here is a PR that avoids reparsing CSV: #4578

alamb · 2022-12-07T21:22:32Z

datafusion/core/tests/sqllogictests/src/main.rs

+    if batches.is_empty() {
+        return Ok(DBOutput::StatementComplete(0));
+    }
+    // TODO: use the actual types


👍 thanks -- we can fill this in

alamb · 2022-12-07T21:26:58Z

datafusion/core/tests/sqllogictests/src/main.rs

-        let builder = WriterBuilder::new().has_headers(false).with_delimiter(b' ');
+        let builder = WriterBuilder::new()
+            .has_headers(false)
+            .with_delimiter(b'\t');
        let mut writer = builder.build(&mut bytes);
        for batch in batches {
            writer.write(&normalize_batch(batch)).unwrap();
        }
    }
-    Ok(String::from_utf8(bytes).unwrap())
+    let res = String::from_utf8(bytes).unwrap();
+    let rows = res
+        .lines()
+        .map(|s| {
+            s.split("\t")
+                .map(|s| {
+                    if s.is_empty() {
+                        "NULL".to_string()
+                    } else {
+                        s.to_string()
+                    }
+                })
+                .collect()
+        })
+        .collect();


Thank you @xxchan -- this is great! The new interface makes a lot of sense. Thank you for the PR!

Given what I see here, I think there may be some better ways to format the output into Vec<Vec<String>> than re-parsing CSV, by using arrow directly (for example how it is done in https://docs.rs/arrow/28.0.0/arrow/util/pretty/index.html)

I'll give it a think and see if I can find some way to update it

cc @mvanschellebeeck

xudong963 · 2022-12-08T03:15:06Z

datafusion/core/tests/sqllogictests/test_files/misc.slt

 select 'foo', '', NULL
 ----
-foo (empty)
+foo (empty) NULL


@xxchan You are so quickly!

I noticed your new release and prepared to remove the issue about NULL. Then I found you done!

Thank you

xudong963

Thanks @xxchan

xudong963 · 2022-12-08T12:24:06Z

datafusion/core/tests/sqllogictests/test_files/ddl.slt

 select * from users;
 ----
 1 2
+11 20


Why this change?

For rowsort, runner will only sort actual results now, which means the result in the test cases should be sorted.

alamb · 2022-12-09T11:17:08Z

FWIW if no one beats me to it I plan to try and polish the conversion to Vec<Vec<String>> this weekend, but I would be happy to have this one merged in first. I suspect it has been accumulating both actual conflicts (as github says) as well as logical conflicts with newly added SLT files.

I poked around in the sqllogictest runner, and it looks like you already have some code that will run / update the test files with new results that looks super awesome (and relevant to updating results)

https://github.com/risinglightdb/sqllogictest-rs/blob/d753e4c77fbf00cdaab69477df2e9c3dba3f5fcc/sqllogictest-bin/src/lib.rs#L505

xudong963 · 2022-12-09T11:25:39Z

I poked around in the sqllogictest runner, and it looks like you already have some code that will run / update the test files with new results that looks super awesome (and relevant to updating results)

https://github.com/risinglightdb/sqllogictest-rs/blob/d753e4c77fbf00cdaab69477df2e9c3dba3f5fcc/sqllogictest-bin/src/lib.rs#L505

Yes, it's a cool feature.

but I would be happy to have this one merged in first

Sounds great!

xxchan · 2022-12-09T11:39:17Z

rebased & resolved conflicts.

xxchan · 2022-12-09T12:29:23Z

weird 🤨

xudong963 · 2022-12-09T12:33:21Z

#4565 @xxchan

Need to rebase master

xxchan · 2022-12-09T13:19:34Z

Hope this is the last fix...

xudong963 · 2022-12-09T14:41:28Z

Let's merge!

alamb · 2022-12-09T14:54:09Z

Thanks @xxchan and @xudong963

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Dec 7, 2022

xxchan commented Dec 7, 2022

View reviewed changes

alamb reviewed Dec 7, 2022

View reviewed changes

xudong963 reviewed Dec 8, 2022

View reviewed changes

xudong963 approved these changes Dec 8, 2022

View reviewed changes

xxchan added 5 commits December 9, 2022 13:34

bump sqllogictest to 0.9.0

c76a136

cargo update datafusion-cli

62c3865

fix clippy

22e816a

fix Cargo.lock

0c9283e

fix test case

b8e18ac

xudong963 merged commit 8365d96 into apache:master Dec 9, 2022

xxchan deleted the xxchan/naval-donkey branch December 9, 2022 14:45

This was referenced Dec 9, 2022

Make it easier to update sqltestlogic test expected output ("test script completion mode") #4570

Closed

Output sqllogictests with arrow display rather than CSV writer #4578

Merged

Conversation

xxchan commented Dec 7, 2022 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xudong963 left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 9, 2022

Uh oh!

xudong963 commented Dec 9, 2022

Uh oh!

xxchan commented Dec 9, 2022

Uh oh!

xxchan commented Dec 9, 2022

Uh oh!

xudong963 commented Dec 9, 2022

Uh oh!

xxchan commented Dec 9, 2022

Uh oh!

xudong963 commented Dec 9, 2022

Uh oh!

alamb commented Dec 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xxchan commented Dec 7, 2022 •

edited by alamb

Loading

xudong963 left a comment •

edited

Loading