Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translate IF and CASE to ConnectorExpression #11699

Open
assaf2 opened this issue Mar 29, 2022 · 6 comments
Open

Translate IF and CASE to ConnectorExpression #11699

assaf2 opened this issue Mar 29, 2022 · 6 comments

Comments

@assaf2
Copy link
Member

assaf2 commented Mar 29, 2022

Open question - Should both be translated to a "$case" function or should we use separated functions?

@findepi
Copy link
Member

findepi commented Apr 5, 2022

Martin and I discussed possible options to represent CASE.

An important property of CASE expression is that is has conditional evaluation for the values.
eg expression like CASE WHEN x > 0 THEN 10 / x ELSE NULL END must not fail when x=0.

One option is to model CASE as a function call. This would mean that either this function call is "special", as it defines conditional evaluation of it some of its parameters, of we model the laziness using lambda expressions. There is a conclusion that we want ConnectorExpressions to support parametric lambdas (eg array reduction, or pushdown filter through UNNEST), but we don't want to add this complexity now.

Thus, the CASE should be modeled as a new class of ConnectorExpression.

Now, actually there are these forms of case

  • CASE WHEN ... THEN ... - "searched case" -- let's represent it as SearchedCase class
  • CASE X WHEN const THEN ... - "simple case" - let's represent it as SimpleCase Switch class
  • CASE x WHEN LESS THAN c THEN ... - not supported in Trino
  • ....

So, let's introduce new classes

  • SearchedCase
  • SimpleCase Switch

@findepi
Copy link
Member

findepi commented Apr 5, 2022

Now, regarding IF:
IF should be treated as a syntactic sugar and represented as CASE.
Whether the translation back from ConnectorExpression should recreate the IF does not matter from planner perspective, but MAY matter from bytecode generation perspective. We should check with @dain @sopel39 @lukasz-stec whether one form -- IF or CASE -- is preferred by the expression execution engine.

@martint
Copy link
Member

martint commented Apr 5, 2022

So, let's introduce new classes

  • SearchedCase
  • SimpleCase

I would name it Switch instead of SimpleCase. It's more descriptive of what the operation does.

Whether the translation back from ConnectorExpression should recreate the IF does not matter from planner perspective, but MAY matter from bytecode generation perspective.

IF has a different code generator (IfCodeGenerator vs SwitchCodeGenerator), but if I recall correctly, it produces the same (or very similar) bytecode for an equivalent expression. It's been too long since I wrote it :)

@findepi
Copy link
Member

findepi commented Apr 6, 2022

I would name it Switch instead of SimpleCase. It's more descriptive of what the operation does.

good idea, updated above

Whether the translation back from ConnectorExpression should recreate the IF does not matter from planner perspective, but MAY matter from bytecode generation perspective.

IF has a different code generator (IfCodeGenerator vs SwitchCodeGenerator), but if I recall correctly, it produces the same (or very similar) bytecode for an equivalent expression

@sopel39 @lukasz-stec please chime in on this

@lukasz-stec
Copy link
Member

I'm not 100% sure that the current generated code for IF and CASE representing IF have the same performance (though it looks like it), but since those two are logically equivalent, there is nothing stopping us from generating the same bytecode in both cases if necessary.

@findepi
Copy link
Member

findepi commented Apr 6, 2022

Thanks @lukasz-stec for chiming in. So for this issue, let's not try to preserve IF troundtip from Expression to ConnectorExpression and back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants