Revert "feat: Add rewrite for IN special form"#15649
Revert "feat: Add rewrite for IN special form"#15649kagamiori wants to merge 1 commit intofacebookincubator:mainfrom
Conversation
Summary: This diff reverts D87598240 This change introduced incorrect behavior as described in facebookincubator#15648. Depends on D87598240 Differential Revision: D87955860
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
|
@kagamiori has exported this pull request. If you are a Meta employee, you can view the originating Diff in D87955860. |
kagamiori
changed the title
| bool, | ||
| Generic<T1>, | ||
| Variadic<Generic<T1>>>({"in"}); | ||
| InRewrite::registerRewrite(); |
There was a problem hiding this comment.
@kagamiori, could you please remove the registration of the rewrite and retain InRewrite.h/.cpp? Reference: #15549 (comment) .
There was a problem hiding this comment.
Hi @pramodsatya, sorry that I just saw your message, but this PR is already merged. Could you create another PR to add InRewrite.h/.cpp back? (By the way, we found that IN after rewrite can produce incorrect result, so it's probably safer for your use case too to fix this bug before continue using it.)
There was a problem hiding this comment.
@kagamiori, no worries, I reintroduced the IN-rewrite here: #15663, avoiding the rewrite for expressions of form NULL IN [a, b, ...] in order to respect the kNullAsIndeterminate null compare flag and fix the result change. Could you please take a look when you get a chance?
pramodsatya
left a comment
pramodsatya
left a comment
There was a problem hiding this comment.
Thanks @kagamiori, could you please remove just the registration of the rewrite until #15648 is resolved? That way it can still be used outside of Velox expression eval, please see discussion in #15549 (comment).
|
This pull request has been merged in c045573. |
4 participants
Summary:
This diff reverts D87598240
This change introduced incorrect behavior as described in #15648.
Depends on D87598240
Differential Revision: D87955860