fix(binder): fix explain with parameters; simplify IN expr without consts

[chenzl25]

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

• Previously, select * from t where id in ($1) will be converted into select * from t where id in() or id = $1 which is unfriendly to the const folding and it looks like a bug as well.

Checklist

• I have written necessary rustdoc comments.
• I have added necessary unit tests and integration tests.
• I have added test labels as necessary.
• I have added fuzzing tests or opened an issue to track them.
• My PR contains breaking changes.
• My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
• I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

• My PR needs documentation updates.

Release note

[xiangjinwu]

is unfriendly to the const folding and it looks like a bug as well

Could you elaborate on this?

• I agree it is redundant and better removed.
• But is there any correctness issue that we overlooked? If yes how can we reproduce it?
• The two fixes in this PR seems unrelated, one about non-const IN and another about $1 in ConstEvalRewriter. In which case is the latter code path reachable?

[xiangjinwu]

nit: if-let or let-else to avoid this unwrap()

[chenzl25]

In which case is the latter code path reachable?

We can simply run explain select $1 to reproduce it.

[chenzl25]

Adding 2 test cases here.

[xiangjinwu]

We can simply run explain select $1 to reproduce it.

I see; so it is indeed unrelated to IN and just blocks the usage of explain when parameter is not provided. select $1 without explain is still rejected (via captured panic) at a later stage: Parameter should not be serialized to ExprNode

[xiangjinwu]

I agree it is redundant and better removed.

With the redundant part removed, a single equality condition can be pushed down for index selection. I guess this is the real fix (for performance), right?

[chenzl25]

True, but we shouldn't panic. And explain is useful to debug this prepared statement.

[chenzl25]

With the redundant part removed, a single equality condition can be pushed down for index selection. I guess this is the real fix (for performance), right?

Yes. BTW, an in expression without any arguments seems an invalid expression.

[xiangjinwu]

Agree. Just trying to understand how each part of the PR contribute to fix for panic and performance.

[xiangjinwu]

an in expression without any arguments seems an invalid expression

It is syntactically invalid, but semantically well defined - just testing against an empty set, like IN (subquery) where the subquery returns zero rows. Or did you find any problem in this definition or our existing implementation?

[chenzl25]

Indeed.It is syntactically invalid, but semantically well defined .