fix: Unregister special forms SWITCH rewrite

[peterenescu]

Summary:
SWITCH rewrite has correctness issues identified by expression fuzzer and causes behavior mismatch.

More information and investigation can be found here: #15486

Differential Revision: D87355432

[meta-codesync]

@peterenescu has exported this pull request. If you are a Meta employee, you can view the originating Diff in D87355432.

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	37ec069
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/691f639451c8c3000893f19b

[pramodsatya]

Thanks @peterenescu. Could you also please remove SwitchRewriteTest.cpp from velox/expression/tests/CMakeLists.txt ?

[pramodsatya]

@peterenescu, could you please retain SwitchRewrite.h/.cpp? We are dependent on this rewrite in Presto C++ for optimizing Presto RowExpressions with the native engine (ref: prestodb/presto#22927). The rewritten expression will be converted to a Presto RowExpression and it will not be compiled/evaluated in Velox.
cc: @czentgr @aditi-pandit @majetideepak

[czentgr]

@pramodsatya If there is a correctness problem it won't work for PrestoC++ either. We have to investigate what the correctness issues are. I see you opened the issue. Looks to be a type issue where the fuzzer generates two large integer literals but on constant folding the resulting value would overflow the type. I would say this is expected and a technically a user error that the fuzzer should skip, no?

@peterenescu Instead of fully reverting it, can we just not register the rewrite when registering the special form? That way the changes are minimal and we still have an option to register it separately in PrestoC++?

[peterenescu]

@czentgr The issue is not with the fuzzer but a behavior change that was introduced by the switch rewrite. More information can be found here: #15486 (comment).

@pramodsatya As the investigation above identifies, there is a meaningful behavioral change that will affect expression evaluation in Presto C++ as it uses Velox and any ongoing efforts in prestodb/presto#22927 to use Velox constant folding and expression rewrite in coordinator. Therefore, it would be prudent to test and certify this behavior change before we introduce it to Presto.

To clarify, we were hoping to revert only SWITCH while we test and verify internally and add it back hopefully by next week.

[pramodsatya]

Thanks @czentgr, @peterenescu. I agree the SWITCH rewrite introduces behavior change that will affect any expression evaluated in Presto C++.
Clarifying a bit more about why we need this rewrite in Presto C++:

1. We intend to apply the SWITCH rewrite only via the Velox ExprOptimizer. The intent is to optimize a Presto expression using the existing Velox ExprOptimizer (constant folding + rewrites) and return back an optimized Presto expression to the coordinator. In the linked Presto PR, we are only adding the requisite Velox -> Presto expression conversion.
2. The Velox TypedExpr optimized via the ExprOptimizer will not be compiled and it will not be evaluated on any input data, mitigating the risk due to correctness issue that has been identified. Instead, the optimized TypedExpr will be converted back to a Presto expression and returned to the coordinator. Presto java already performs this optimization for SWITCH, so this rewrite will give us functional parity with Presto for expression optimization in C++.
3. All these changes will be made in the Presto C++ sidecar, which is an opt-in via config (native-sidecar=false by default). We are also adding a new endpoint v1/expressions for this change, so this will not affect any existing Presto C++ functionality.

We should definitely avoid applying SWITCH rewrite in Velox until the issue is resolved. But retaining SwitchRewrite.h/.cpp would give us the flexibility to test/add this in Presto C++ for our usecase without duplicating effort. Please let me know what you think.

[bikramSingh91]

Update on next steps: #15486 (comment)

TLDR: we are closing the PR and will work on enhancing the fuzzer to handle this case