From c2ff663856555bdd0ffbcd5908ec0a42fb819d99 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Sat, 10 Dec 2022 07:25:51 -0500
Subject: [PATCH 1/4] Add impl Display for Record (refactor Record::unparse)
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
---
sqllogictest/src/parser.rs | 60 +++++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 27 deletions(-)
diff --git a/sqllogictest/src/parser.rs b/sqllogictest/src/parser.rs
index 6a25f26..f293b93 100644
--- a/sqllogictest/src/parser.rs
+++ b/sqllogictest/src/parser.rs
@@ -142,9 +142,15 @@ impl Record {
/// # Panics
/// If the record is an internally injected record which should not occur in the test file.
pub fn unparse(&self, w: &mut impl std::io::Write) -> std::io::Result<()> {
+ write!(w, "{}", self)
+ }
+}
+
+impl std::fmt::Display for Record {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Record::Include { loc: _, filename } => {
- write!(w, "include {}", filename)
+ write!(f, "include {}", filename)
}
Record::Statement {
loc: _,
@@ -153,21 +159,21 @@ impl Record {
sql,
expected_count,
} => {
- write!(w, "statement ")?;
+ write!(f, "statement ")?;
match (expected_count, expected_error) {
- (None, None) => write!(w, "ok")?,
+ (None, None) => write!(f, "ok")?,
(None, Some(err)) => {
if err.as_str().is_empty() {
- write!(w, "error")?;
+ write!(f, "error")?;
} else {
- write!(w, "error {}", err)?;
+ write!(f, "error {}", err)?;
}
}
- (Some(cnt), None) => write!(w, "count {}", cnt)?,
+ (Some(cnt), None) => write!(f, "count {}", cnt)?,
(Some(_), Some(_)) => unreachable!(),
}
- writeln!(w)?;
- write!(w, "{}", sql)
+ writeln!(f)?;
+ write!(f, "{}", sql)
}
Record::Query {
loc: _,
@@ -179,60 +185,60 @@ impl Record {
sql,
expected_results,
} => {
- write!(w, "query")?;
+ write!(f, "query")?;
if let Some(err) = expected_error {
- writeln!(w, " error {}", err)?;
- return write!(w, "{}", sql);
+ writeln!(f, " error {}", err)?;
+ return write!(f, "{}", sql);
}
write!(
- w,
+ f,
" {}",
type_string.iter().map(|c| format!("{c}")).join("")
)?;
if let Some(sort_mode) = sort_mode {
- write!(w, " {}", sort_mode.as_str())?;
+ write!(f, " {}", sort_mode.as_str())?;
}
if let Some(label) = label {
- write!(w, " {}", label)?;
+ write!(f, " {}", label)?;
}
- writeln!(w)?;
- writeln!(w, "{}", sql)?;
+ writeln!(f)?;
+ writeln!(f, "{}", sql)?;
- write!(w, "----")?;
+ write!(f, "----")?;
for result in expected_results {
- write!(w, "\n{}", result)?;
+ write!(f, "\n{}", result)?;
}
Ok(())
}
Record::Sleep { loc: _, duration } => {
- write!(w, "sleep {}", humantime::format_duration(*duration))
+ write!(f, "sleep {}", humantime::format_duration(*duration))
}
Record::Subtest { loc: _, name } => {
- write!(w, "subtest {}", name)
+ write!(f, "subtest {}", name)
}
Record::Halt { loc: _ } => {
- write!(w, "halt")
+ write!(f, "halt")
}
Record::Control(c) => match c {
- Control::SortMode(m) => write!(w, "control sortmode {}", m.as_str()),
+ Control::SortMode(m) => write!(f, "control sortmode {}", m.as_str()),
},
Record::Condition(cond) => match cond {
Condition::OnlyIf { engine_name } => {
- write!(w, "onlyif {}", engine_name)
+ write!(f, "onlyif {}", engine_name)
}
Condition::SkipIf { engine_name } => {
- write!(w, "skipif {}", engine_name)
+ write!(f, "skipif {}", engine_name)
}
},
Record::HashThreshold { loc: _, threshold } => {
- write!(w, "hash-threshold {}", threshold)
+ write!(f, "hash-threshold {}", threshold)
}
Record::Comment(comment) => {
let mut iter = comment.iter();
- write!(w, "#{}", iter.next().unwrap().trim_end())?;
+ write!(f, "#{}", iter.next().unwrap().trim_end())?;
for line in iter {
- write!(w, "\n#{}", line.trim_end())?;
+ write!(f, "\n#{}", line.trim_end())?;
}
Ok(())
}
From 9924a96b96dcac07ea398975f3f05a04a4462c8e Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Sat, 10 Dec 2022 07:37:43 -0500
Subject: [PATCH 2/4] Add test for reparsing
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
---
sqllogictest/src/parser.rs | 83 +++++++++++++++++++++++++++++++++++++-
sqllogictest/src/runner.rs | 1 +
2 files changed, 82 insertions(+), 2 deletions(-)
diff --git a/sqllogictest/src/parser.rs b/sqllogictest/src/parser.rs
index f293b93..6910023 100644
--- a/sqllogictest/src/parser.rs
+++ b/sqllogictest/src/parser.rs
@@ -132,6 +132,7 @@ pub enum Record {
},
Condition(Condition),
Comment(Vec<String>),
+ Whitespace(String),
/// Internally injected record which should not occur in the test file.
Injected(Injected),
}
@@ -242,6 +243,9 @@ impl std::fmt::Display for Record {
}
Ok(())
}
+ Record::Whitespace(w) => {
+ write!(f, "{}", w)
+ }
Record::Injected(p) => panic!("unexpected injected record: {:?}", p),
}
}
@@ -397,6 +401,7 @@ fn parse_inner(loc: &Location, script: &str) -> Result<Vec<Record>, ParseError>
}
if line.is_empty() {
+ records.push(Record::Whitespace(line.to_string()));
continue;
}
@@ -609,11 +614,85 @@ fn parse_file_inner(loc: Location) -> Result<Vec<Record>, ParseError> {
#[cfg(test)]
mod tests {
- use crate::parse_file;
+ use difference::{Changeset, Difference};
+
+ use super::*;
#[test]
fn test_include_glob() {
let records = parse_file("../examples/include/include_1.slt").unwrap();
- assert_eq!(14, records.len());
+ assert_eq!(16, records.len());
+ }
+
+ #[test]
+ fn test_basic() {
+ parse_roundtrip("../examples/basic/basic.slt")
+ }
+
+ /// Parses the specified file into Records, and ensures the
+ /// results of unparsing them are the same
+ ///
+ /// Prints a hopefully useful message on failure
+ fn parse_roundtrip(filename: impl AsRef<Path>) {
+ let filename = filename.as_ref();
+ let input_contents = std::fs::read_to_string(filename).expect("reading file");
+
+ let records = parse_file(filename).expect("parsing to complete");
+
+ let unparsed = records
+ .into_iter()
+ .map(record_to_string)
+ .collect::<Vec<_>>();
+
+ let output_contents = unparsed.join("\n");
+
+ let changeset = Changeset::new(&input_contents, &output_contents, "\n");
+
+ assert!(
+ no_diffs(&changeset),
+ "Mismatch for {:?}\n\
+ *********\n\
+ diff:\n\
+ *********\n\
+ {}\n\n\
+ *********\n\
+ output:\n\
+ *********\n\
+ {}\n\n",
+ filename,
+ UsefulDiffDisplay(&changeset),
+ output_contents,
+ );
+ }
+
+ fn record_to_string(record: Record) -> String {
+ if matches!(&record, Record::Statement { .. } | Record::Query { .. }) {
+ // the statement parser includes a newline between the items but the display
+ // output does not, so add it here
+ // Not sure about this one
+ format!("{}\n", record)
+ } else {
+ record.to_string()
+ }
+ }
+
+ /// returns true if there are no differences in the changeset
+ fn no_diffs(changeset: &Changeset) -> bool {
+ changeset
+ .diffs
+ .iter()
+ .all(|diff| matches!(diff, Difference::Same(_)))
+ }
+
+ struct UsefulDiffDisplay<'a>(&'a Changeset);
+
+ impl<'a> std::fmt::Display for UsefulDiffDisplay<'a> {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ self.0.diffs.iter().try_for_each(|diff| match diff {
+ Difference::Same(x) => writeln!(f, "{x}"),
+ Difference::Add(x) => writeln!(f, "+ {x}"),
+ Difference::Rem(x) => writeln!(f, "- {x}"),
+ })
+ }
}
}
diff --git a/sqllogictest/src/runner.rs b/sqllogictest/src/runner.rs
index b064b3c..3bb4f35 100644
--- a/sqllogictest/src/runner.rs
+++ b/sqllogictest/src/runner.rs
@@ -546,6 +546,7 @@ impl<D: AsyncDB> Runner<D> {
}
Record::Include { .. }
| Record::Comment(_)
+ | Record::Whitespace(_)
| Record::Subtest { .. }
| Record::Halt { .. }
| Record::Injected(_)
From 6f49f290ddb8a7ffdf0ef66bbc4a893f2024dac3 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Sat, 10 Dec 2022 13:24:30 -0500
Subject: [PATCH 3/4] Add clarifying comments
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
---
sqllogictest/src/parser.rs | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/sqllogictest/src/parser.rs b/sqllogictest/src/parser.rs
index 6910023..5fea40c 100644
--- a/sqllogictest/src/parser.rs
+++ b/sqllogictest/src/parser.rs
@@ -640,10 +640,23 @@ mod tests {
let records = parse_file(filename).expect("parsing to complete");
let unparsed = records
- .into_iter()
+ .iter()
.map(record_to_string)
.collect::<Vec<_>>();
+ // Technically this will not always be the same due to some whitespace normalization
+ //
+ // query III
+ // select * from foo;
+ // ----
+ // 1 2
+ //
+ // Will print out collaposting the spaces between `query`
+ //
+ // query III
+ // select * from foo;
+ // ----
+ // 1 2
let output_contents = unparsed.join("\n");
let changeset = Changeset::new(&input_contents, &output_contents, "\n");
From 44ee05a7a109857d76d09793c0b70470ac1df8a4 Mon Sep 17 00:00:00 2001
From: Andrew Lamb <andrew@nerdnetworks.org>
Date: Sat, 10 Dec 2022 13:29:39 -0500
Subject: [PATCH 4/4] Review feedback
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
---
sqllogictest/src/parser.rs | 32 ++++++++++++--------------------
sqllogictest/src/runner.rs | 2 +-
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/sqllogictest/src/parser.rs b/sqllogictest/src/parser.rs
index 5fea40c..2e2f43f 100644
--- a/sqllogictest/src/parser.rs
+++ b/sqllogictest/src/parser.rs
@@ -132,7 +132,7 @@ pub enum Record {
},
Condition(Condition),
Comment(Vec<String>),
- Whitespace(String),
+ Newline,
/// Internally injected record which should not occur in the test file.
Injected(Injected),
}
@@ -147,6 +147,9 @@ impl Record {
}
}
+/// As is the standard for Display, does not print any trailing
+/// newline except for records that always end with a blank line such
+/// as Query and Statement.
impl std::fmt::Display for Record {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
@@ -174,7 +177,8 @@ impl std::fmt::Display for Record {
(Some(_), Some(_)) => unreachable!(),
}
writeln!(f)?;
- write!(f, "{}", sql)
+ // statement always end with a blank line
+ writeln!(f, "{}", sql)
}
Record::Query {
loc: _,
@@ -189,7 +193,7 @@ impl std::fmt::Display for Record {
write!(f, "query")?;
if let Some(err) = expected_error {
writeln!(f, " error {}", err)?;
- return write!(f, "{}", sql);
+ return writeln!(f, "{}", sql);
}
write!(
@@ -210,7 +214,8 @@ impl std::fmt::Display for Record {
for result in expected_results {
write!(f, "\n{}", result)?;
}
- Ok(())
+ // query always ends with a blank line
+ writeln!(f)
}
Record::Sleep { loc: _, duration } => {
write!(f, "sleep {}", humantime::format_duration(*duration))
@@ -243,9 +248,7 @@ impl std::fmt::Display for Record {
}
Ok(())
}
- Record::Whitespace(w) => {
- write!(f, "{}", w)
- }
+ Record::Newline => Ok(()), // Display doesn't end with newline
Record::Injected(p) => panic!("unexpected injected record: {:?}", p),
}
}
@@ -401,7 +404,7 @@ fn parse_inner(loc: &Location, script: &str) -> Result<Vec<Record>, ParseError>
}
if line.is_empty() {
- records.push(Record::Whitespace(line.to_string()));
+ records.push(Record::Newline);
continue;
}
@@ -641,7 +644,7 @@ mod tests {
let unparsed = records
.iter()
- .map(record_to_string)
+ .map(|record| record.to_string())
.collect::<Vec<_>>();
// Technically this will not always be the same due to some whitespace normalization
@@ -678,17 +681,6 @@ mod tests {
);
}
- fn record_to_string(record: Record) -> String {
- if matches!(&record, Record::Statement { .. } | Record::Query { .. }) {
- // the statement parser includes a newline between the items but the display
- // output does not, so add it here
- // Not sure about this one
- format!("{}\n", record)
- } else {
- record.to_string()
- }
- }
-
/// returns true if there are no differences in the changeset
fn no_diffs(changeset: &Changeset) -> bool {
changeset
diff --git a/sqllogictest/src/runner.rs b/sqllogictest/src/runner.rs
index 3bb4f35..67366ab 100644
--- a/sqllogictest/src/runner.rs
+++ b/sqllogictest/src/runner.rs
@@ -546,7 +546,7 @@ impl<D: AsyncDB> Runner<D> {
}
Record::Include { .. }
| Record::Comment(_)
- | Record::Whitespace(_)
+ | Record::Newline
| Record::Subtest { .. }
| Record::Halt { .. }
| Record::Injected(_)