-
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
base: main
Are you sure you want to change the base?
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+).
|
|
||
| ## Cookbook: Ignoring volatile output | ||
|
|
||
| Sometimes parts of a result change every run (timestamps, counters, etc). To keep the rest of the snapshot checked in, replace those fragments with the `<slt:ignore>` marker inside the expected block. During validation the marker acts like a wildcard, so only the surrounding text must match. |
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.
| Sometimes parts of a result change every run (timestamps, counters, etc). To keep the rest of the snapshot checked in, replace those fragments with the `<slt:ignore>` marker inside the expected block. During validation the marker acts like a wildcard, so only the surrounding text must match. | |
| Sometimes parts of a result change every run (timestamps, counters, etc.). To keep the rest of the snapshot checked in, replace those fragments with the `<slt:ignore>` marker inside the expected block. During validation the marker acts like a wildcard, so only the surrounding text must match. |
| 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.
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?