Add update_record_with_output function and tests
#130
Conversation
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
alamb
force-pushed
the
alamb/record_update
branch
from
6eb4711
to
dc4b704
Compare
| }) | ||
| } | ||
|
|
||
| // No update possible, return the original record |
There was a problem hiding this comment.
Did you look at update_record? https://github.com/risinglightdb/sqllogictest-rs/blob/46752d1b536f186e3f976bdfce4968ce761ab230/sqllogictest-bin/src/lib.rs
I think they are highly alike, but it contains more updatable edge cases. How do you think the differences?
There was a problem hiding this comment.
🤔 I forgot out update_record
I think it is cleaner to update the Record and then use the Display logic to print it back out, rather than write to a new file directly. However, there is nothing wrong with generating the text directly (but there may be more chance of inconsistencies)
I am out of time for coding today, sadly, but I will think on this question and see if I can come to a conclusion
There was a problem hiding this comment.
@xxchan if I want to refactor update_record and avoid breaking your tests, what is the best way to do so? I have been running cargo test but I guess that is not the only thing 🤔
There was a problem hiding this comment.
@alamb I did the refactor. You can take a look whether it looks good to you.
There was a problem hiding this comment.
I took a look - and it looks good to me @xxchan -- thank you 🙏
The only thing I would normally do is to write some more tests to cover these cases (and also illustrate how the updates work)-- perhaps I can do that as a follow on PR?
Signed-off-by: xxchan <xxchan22f@gmail.com>
| /// By default ([`default_validator`]), we will use compare normalized results. | ||
| pub type Validator = fn(actual: &[Vec<String>], expected: &[String]) -> bool; | ||
|
|
||
| pub fn default_validator(actual: &[Vec<String>], expected: &[String]) -> bool { |
| let record_output = runner.apply_record(record.clone()).await; | ||
| match update_record_with_output(&record, &record_output, "\t", default_validator) { | ||
| Some(new_record) => { | ||
| writeln!(outfile, "{new_record}")?; |
| expected_results, | ||
| }, | ||
| RecordOutput::Query { | ||
| // FIXME: maybe we should use output's types instead of orignal query's types |
There was a problem hiding this comment.
yes, I agree it should use the output query types
| }) | ||
| } | ||
|
|
||
| // No update possible, return the original record |
There was a problem hiding this comment.
I took a look - and it looks good to me @xxchan -- thank you 🙏
The only thing I would normally do is to write some more tests to cover these cases (and also illustrate how the updates work)-- perhaps I can do that as a follow on PR?
|
@xxchan would you like me to add tests to this PR or do you plan to merge this one and have me add more tests as a follow on PR? |
|
Let's merge it first and improve it later, since it generally looks good and works well in my manual testing. |
Here is a follow on PR #131 |
Draft as it builds on #129Rationale
I want to be able to update Records given the output of a particular run (see apache/datafusion#4570)
Changes
update_record_with_outputfunctionI think this is my last planned PR to
sqllogictestfor a bit (I want to go try and integrated this into DataFusion upstream first)