-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: display the failed sqllogictest file and query that failed in case of a panic #18785
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
fix: display the failed sqllogictest file and query that failed in case of a panic #18785
Conversation
| .fetch_add(1, std::sync::atomic::Ordering::Relaxed); | ||
| let mut lock = self.currently_executed_sqls.lock().unwrap(); | ||
| lock.insert(index, sql.into()); | ||
| drop(lock); |
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.
Is this explicit drop() needed ? I think it is not really needed.
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.
nope, removed
|
In the example test in the PR body, are we meant to be able to see which query panicked in which SLT file? I can't seem to find those details in the panic trace unless I'm overlooking it 🤔 |
|
updated |
Jefffrey
left a comment
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 nice improvement 👍
(Is it possible to get the line number of the SQL as well?)
Not that I know of, but I don't have much knowledge of this part of the codebase |
Which issue does this PR close?
Rationale for this change
To be able to the failed tests
What changes are included in this PR?
Are these changes tested?
Manually, by adding panic in the join like in the issue linked PR and saw that it working for datafusion
Added panic in
sort_merge_joinand run:Logs for running join slt tests (7 files)
Are there any user-facing changes?
Yes, added new struct for tracking running sql