risingwavelabs / risingwave

SQL stream processing, analytics, and management. We decouple storage and compute to offer efficient joins, instant failover, dynamic scaling, speedy bootstrapping, and concurrent query serving.
https://www.risingwave.com/slack
Apache License 2.0
6.6k stars 541 forks source link

bug(optimizer): panic due to missing visit/rewrite of lambda arg #13724

Open xiangjinwu opened 7 months ago

xiangjinwu commented 7 months ago

Describe the bug

We have supported higher-order function array_transform with a lambda argument, for example:

select array_transform(array['a', 'b'], |x| x || 3);

We also have expression visitors and rewriters in optimizer, and their example usage includes extracting common sub-expression, inlining now(), and inlining implicit session timezone.

The default implementation provided by visitor / rewriter trait skips the lambda arg, and this helps prevent common sub-expression extraction from inside. https://github.com/risingwavelabs/risingwave/pull/11937#issuecomment-1697180328

But by skipping the lambda arg for inlining now() or implicit timezone, the plan would be un-evaluable in the backend. (The timezone support is being migrated from rewriter to expr context. Shall it happen to now() as well?)

It is possible to fix each visitor / rewriter case by case. The real problem is: what should be the default?

Error message/log

thread 'risingwave-main' panicked at src/expr/core/src/expr/build.rs:126:32:
internal error: entered unreachable code: now should not be built at backend

ERROR:  Failed to run the query
Caused by these errors (recent errors listed first):
  1: Expr error
  2: Unsupported function: date_trunc

To Reproduce

select array_transform(array['a', 'b'], |x| x || now());

select array_transform(array['a', 'b'], |x| x || date_trunc('hour', to_timestamp(0)));

Expected behavior

No response

How did you deploy RisingWave?

No response

The version of RisingWave

No response

Additional context

Maybe there should be no default, or we require each implementation to provide a fn include_lambda_arg() -> bool

cc @TennyZhuang @chenzl25

st1page commented 7 months ago

The real problem is: what should be the default?

How about just not implement the default behavior :thinking: c.c. @chenzl25