-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(small): Support <slt:ignore> marker in sqllogictest for non-deterministic expected parts
#18857
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
Conversation
| if let Some(idx) = actual_snapshot[pos..].find(frag) { | ||
| pos += idx + frag.len(); | ||
| } else { | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This early return would make it hard to debug. It would be nice to log a warning with the reason, as done below (line 116+).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I don't know what those log are used for. I think the error message is generated elsewhere, and I want to make sure they're developer-friendly.
It looks okay now, but with false positives
2. query result mismatch:
[SQL] select * from generate_series(3);
[Diff] (-expected|+actual)
- <slt:ignore>
+ 0
1
- 3<slt:ignore>
+ 2
3
at /Users/yongting/Code/datafusion/datafusion/sqllogictest/test_files/slt_features.slt:84
I have filed #18878 for improvement.
| continue; | ||
| } | ||
| if let Some(idx) = actual_snapshot[pos..].find(frag) { | ||
| pos += idx + frag.len(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic won't detect a prefix in the actual_snapshot.
Let's say the first expected fragment is "Hello" but the actual_snapshot starts with "Anything here before Hello". The find() will set the position to the correct index but Anything here before will be ignored and assumed as matching.
Similar issue with a suffix in actual_snapshot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good catch. I fixed it in a3154e7 and added more tests.
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Which issue does this PR close?
Part of #17612
Rationale for this change
sqllogictests are in general easier to maintain than rust tests, however it's not able to testEXPLAIN ANALYZEresults, because their results include changing part:(in datafusion-cli) The
elapsed_computemeasurement changes from run to run.We can add a special marker to
sqllogictestto skip those non-deterministic parts.What changes are included in this PR?
sqllogictestvalidator to recognize<slt:ignore>markerAre these changes tested?
Are there any user-facing changes?