Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Tracked by #4460
alamb opened this issue Dec 9, 2022 · 6 comments · Fixed by #4665
Closed
Tracked by #4460
Assignees

Comments

@alamb
Copy link
Contributor

alamb commented Dec 9, 2022

Usecase

I am writing a new sqllogictest case (I love this tool) and I want to quickly update the expected outputs

For example, given this type of sqllogictest script, I would like to easily update the script (fill in the values of XX and YY)

# Reproducer for https://github.com/apache/arrow-datafusion/issues/3944
statement ok
CREATE TABLE test(
  i_item_desc VARCHAR,
  d1_date DATE,
  d2_date DATE,
  d3_date DATE
) as VALUES
  ('h','2022-12-12','2022-12-5','2022-12-12')
;


# Check greater than 
query C
select i_item_desc
from test
where d3_date > d2_date + INTERVAL '1 days';
----
XX

# Check less than 
query C
select i_item_desc
from test
where d3_date < d2_date + INTERVAL '1 days';
----
YY

Prior to #4547 (thanks @xxchan and @xudong963 ) I would copy/paste the output from my console into the test file

After #4547 I need to copy/paste a diff and then remove all instances of +

For example, I do

cargo test -p datafusion --test sqllogictests -- dates

Which would print out

[Diff] (-excepted|+actual)
-   DD
+   c
+   d
+   e
+   f
+   g
+   h

Which while I can do is quite annoying

Desire

  1. A way to run sqllogictest that given the input file would produce the expected output file so I can verify the contents and update the script

Implementation Ideas

This is called "test script completion mode" in sqlllogictest https://www.sqlite.org/sqllogictest/doc/trunk/about.wiki

I think the sqllogictest runner binary has some version of this that looks super awesome (and relevant to updating results), here:

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

@alamb alamb mentioned this issue Dec 9, 2022
28 tasks
@alamb alamb changed the title Some way to update expected sqltestlogic output more easily Make it easier to update sqltestlogic test expected output ("test script completion mode") Dec 9, 2022
@alamb
Copy link
Contributor Author

alamb commented Dec 9, 2022

I am curious if you have any thoughts @mvanschellebeeck @xudong963 and @xxchan ? I may try to code this up over the weekend if you think it is reasonable

@xxchan
Copy link
Member

xxchan commented Dec 9, 2022

Yes I think it makes total sense. This feature is already tested risingwavelabs/risingwave#6776. It's currently only supported in sqllogictest-bin which only supports postgres protocol, but we do want to make it more accessible risinglightdb/sqllogictest-rs#114. I haven't considered it carefully yet, but maybe simply moving related code into runner is enough.

@alamb
Copy link
Contributor Author

alamb commented Dec 9, 2022

Thanks @xxchan -- if I get a chance this weekend I'll see if I can make a proposal upstream.

@alamb alamb self-assigned this Dec 10, 2022
@alamb
Copy link
Contributor Author

alamb commented Dec 10, 2022

Her is my solution sketch:

  1. Add impl Display for Record to sqlogictests (that is the opposite of parse)
  2. Create an update_record based on the code in https://github.com/risinglightdb/sqllogictest-rs/blob/d753e4c77fbf00cdaab69477df2e9c3dba3f5fcc/sqllogictest-bin/src/lib.rs#L620 that updates a Record with the output of RecordOutput
/// Update record using output. If this record is then
///
fn update_output(record: output: RecordOutput) -> Result<Record> {
...
}

Then it will be possible to write code like

for record in input_records {
    let record_output =  runner.apply_record(record.clone()).await?;
    let new_record = update_record(record, record_output)
    write!(output_file, "{}", new_record)
}

To write out the new file

cc @xxchan and @xudong963

@alamb
Copy link
Contributor Author

alamb commented Dec 10, 2022

First sqllogictest PR proposal: risinglightdb/sqllogictest-rs#127

@alamb
Copy link
Contributor Author

alamb commented Dec 11, 2022

I think I need 2 more PRs in sqllogictest before I can implement this feature in DataFusion

risinglightdb/sqllogictest-rs#129
risinglightdb/sqllogictest-rs#130

Will keep following things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants