Query: Consider having predicate on SqlEnumerableExpression so providers can integrate it accordingly

[smitpatel]

Current we put a case block inside SqlExpression itself. It generates null result when predicate is not matched this could be issue with some aggregate functions and translator needs to unwrap it.

[roji]

@smitpatel I'm probably missing something, but if we later add a Predicate on SqlEnumerableExpression, wouldn't the main underlying SqlExpression continue to have a CASE/WHEN, which providers would need to remove (since removing that would be a breaking change at that point)? If that's so, there's probably no point in adding Predicate...

One more thought about whether to introduce CASE/WHEN by default... If we don't introduce it, then I'm supposing translation will fail by default unless the provider specifically handles the separately-provided predicate (like for orderings). On the other hand, if we do CASE/WHEN by default, then translation will always succeed, but will return incorrect results where the function distinguishes nulls from non-nulls. It may be preferable to have "translation failure" as the default rather than "incorrect results" as the default behavior, for when a provider/plugin gets it wrong etc. But I agree it's not a super strong argument.

But to be clear, I'm fine with whatever decision is made here.

[smitpatel]

if we later add a Predicate on SqlEnumerableExpression, wouldn't the main underlying SqlExpression continue to have a CASE/WHEN, which providers would need to remove (since removing that would be a breaking change at that point)?

If we add a Predicate later then we will remove CASE/WHEN from SqlExpression. Removing it wouldn't be a breaking change because the Predicate on grouping element is optional. (you can write g.Select(..).Max() too). So providers which are unwrapping CASE/WHEN block needs to do it in fault-tolerant way. It wouldn't break the code where predicate is not used but it will break the code where providers needs to start using the Predicate field. i.e. That would need to react either way.

From default perspective, you argument is certainly true. I think another point (where predicate vs ordering differs), is orderings for many aggregate functions is irrelevant while predicate is not. So providers will always need specific processing for separate predicate. It goes into same bucket (albeit different direction), that when we could have generated case block anyway, why force everyone to create it. (the other direction is, when providers multiple times have to unwrap case block then why don't we provide it separate only).

This is one of the hard to figure out right default kinda decision because it is totally data driven based on translations added which is not even customer facing.

[roji]

[...] but it will break the code where providers needs to start using the Predicate field

Yeah, that's what I had in mind - if we remove CASE/WHEN later, any query that has a filter would stop working until the provider reacts (by handling the new Predicate).

I think another point (where predicate vs ordering differs), is orderings for many aggregate functions is irrelevant while predicate is not. So providers will always need specific processing for separate predicate.

Yeah, that's absolutely true, and I guess it's an argument in favor of doing CASE/WHEN, since filters would just work without any handling on the provider side.

It may also be worth keeping user function mappings in mind here - they would also possibly need to unwrap the case block if they want to map a function that cares about nulls.

In any case, whatever decision you make here, I'm not sure we'd ever want to reverse it (i.e. move from CASE/WHEN to separate predicate or vice versa).