-
Notifications
You must be signed in to change notification settings - Fork 205
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
Panic on query x -> y
#4280
Comments
Thanks, I can take a look at this.
Interesting — do you have any context on this? We've been considering fuzzing ourselves. |
ClickHouse's CI has a fuzzer that generates SQL queries by using the tests as corpus and iterating pseudorandomly. One test introduced recently changes the dialect to PRQL ( |
OK that's great! If you happen to know — how painful is a panic from PRQL for ClickHouse? Does it kill the ClickHouse process? If it does, we should be much more careful about panics. We treat them all as bugs, but don't fuzz the compiler, and there will be ways of getting them, as this example shows... (FWIW this is something I asked a while ago on the ClickHouse issue tracker in the early days, since I think it used to be bad, but there was some discussion of isolating rust extensions) |
We have a similar practice - some exceptions (of type "logical error") turn into assertions in debug builds. So they terminate the process in CI. But in release builds, they are just exceptions - thrown, then caught, and reported to the user. |
OK great. I'll work on this regardless |
Right now panics crash CH. We tried to avoid it (see https://github.com/ClickHouse/ClickHouse/blob/65cfbaaa4b6194937478e98c773ed6ed56a6d70f/rust/prql/src/lib.rs#L60-L62) but apparently it does not work. If you have any suggestion about how to translate panics into errors it'd be really appreciated, as it would lower the impact of PRQL bugs a lot. |
That looks reasonable. There are some corner cases where a panic could happen in string conversions, but that doesn't seem to be the issue. I'm looking through your list at the top of the issue to try and find the traceback in CH for the |
if you go the CH issue, click in the report linked there and then fatal.log has the backtrace. The index has other files like the server or the fuzzer logs. |
What happened?
Rust panic on SQL query.
Source from ClickHouse fuzzer (the query might not even be a valid SQL query, let alone valid PRQL)
Tested 0.9.5 (CH version), 0.11.3 and the playground and both fail with the query:
Seems like the lambda makes panic.
PRQL input
SQL output
Expected SQL output
MVCE confirmation
Anything else?
No response
The text was updated successfully, but these errors were encountered: