Panic on query x -> y

[Algunenano]

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:

SELECT id FROM distributed_test_table GROUP BY x -> concat(concat(materialize(toNullable(NULL)))) LIMIT 3

Seems like the lambda makes panic.

PRQL input

x -> y

SQL output

A compiler bug was encountered

Expected SQL output

An error

MVCE confirmation

• Minimal example
• New issue

Anything else?

No response

[max-sixty]

Thanks, I can take a look at this.

Source from ClickHouse fuzzer

Interesting — do you have any context on this? We've been considering fuzzing ourselves.

[Algunenano]

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 (SET dialect = 'prql';), so when the fuzzer executes that query and then continues executing other random queries it ends up reaching PRQL.

[max-sixty]

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)

[alexey-milovidov]

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.

[max-sixty]

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

[Algunenano]

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.

[max-sixty]

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.

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 x -> y issue, as a case for panic::catch_unwind failing to work. I can't find after a few min of looking. I'll look more but if it's obvious please lmk...

[Algunenano]

It’s here: https://s3.amazonaws.com/clickhouse-test-reports/0/d93d5c2a0028944e8f17f8fa5257bb1e0de44e1f/ast_fuzzer__asan_/fatal.log

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.