why is this an entirely different funtion? pls integrate to existing infra

This is a different function because it's specific for MaskedArrays. Having this as a separate function is consistent with how it's currently implemented IMO (with a similar separate function for generic EAs).

It could also be another elif check in _ea_wrap_cython_operation, but it's not that it would result in less code or so, and since _ea_wrap_cython_operation already gets quite complicated, I think this separate function is good.

In an initial version I tried to fold this into _ea_wrap_cython_operation, but thought this smaller function was a cleaner solution since the conditionals here can remain much simpler. While adding a function for just supporting masked cummax, cummin seems wasteful, this infrastructure should extend to more masked groupby algos.

see my comments. This MUST integrate with the existing infrastructure (or refactor that). Duplicating is -1

1. The whole point of this PR is to add a special handling for masked arrays, to ensure to pass through the mask to the groupby cython algo. This will always add some code (and the code in _masked_ea_wrap_cython_operation is specific to masked arrays).
2. IMO this is integrated with the existing infrastructure: it integrates nicely into the existing _cython_operation and follows the same pattern as we already have for _ea_wrap_cython_operation

If you don't like how the added code is structured right now, please do a concrete suggestion of how you would do it differently.

FWIW im planning to kill off this function; for EAs this is always just arr.to_numpy(dtype="float64", na_value=np.nan)

Thanks for bringing up - realized this whole condition can be simplified since we actually have an ndarray at this point

i really don't understand all of this code duplication. this is adding huge complexity. pls reduce it.

Jeff, did you actually read the previous responses to your similar comment? (https://github.com/pandas-dev/pandas/pull/40651/files#r603319910) Can you then please answer there to the concrete reasons given.

yes and its a terrible pattern.

this duplication of code is ridiculous. We have a VERY large codebase. Having this kind of separate logic is amazingling confusing & is humungous tech debt. This is heavily used code and needs to be carefully modified.

I understand the concern about adding code complexity - my thinking was that if the goal is for nullable types to become the default in pandas, then direct support makes sense. And in that case, nullable types would need to be special-cased somewhere, and I think the separate function is cleaner than interleaving in _ea_wrap_cython_operation.

If direct support for nullable dtypes is not desired, we can just close this. If it is, I'll keep trying to think of ways to achieve this without adding more code, but any suggestions there would be welcome!

Proper support for nullable dtypes is certainly desired (how to add it exactly can of course be discussed), so thanks a lot @mzeitlin11 for your efforts here.

AFAIK, it's correct we need some special casing for it somewhere (that's the whole point of this PR is to add special handling for it).
Where exactly to put this special casing can of course be discussed, but to me the separate helper method instead of interleaving it in _ea_wrap_cython_operation seems good (I don't think that interleaving it into the existing _ea_wrap_cython_operation would result in fewer added lines of code (and would be harder to read)).

@jreback please try to stay constructive (eg answer to our arguments or provide concrete suggestions on where you would put it / how you would do it differently) and please mind your language (there is no need to call the approach taken by a contributor "terrible").

1. I agree with @jorisvandenbossche on phrasing concerns. Even the best of us slip up here from time to time.

2. if the goal is for nullable types to become the default in pandas

This decision has not been made.

3. I think the separate function is cleaner than interleaving in _ea_wrap_cython_operation.

Agreed.

4. My preferred dispatch logic would look something like:

def _cython_operation(...)
    if is_ea_dtype(...):
       return self. _ea_wrap_cython_operation(...)
    [status quo]

def _ea_wrap_cython_operation(...):
    if should_use_mask(...):
        return self._masked_ea_wrap_cython_operation(...)
    [status quo]

as Joris correctly pointed out, that is not viable ATM. I think a lot of this dispatch logic eventually belongs in WrappedCythonOp (which i've been vaguely planning on doing next time there aren't any open PRs touching this code), at which point we can reconsider flattening this

5. My other preferred dispatch logic would not be in this file at all, but be implemented as a method on the EA subclass. I'm really uncomfortable with this code depending on MaskedArray implementation details, seeing as how there has been discussion of swapping them out for something arrow-based.

@jbrockmendel if you plan further refactoring of this code, I'm happy to just mothball this pr for now. The real benefit won't come in until more groupby algos allow a mask on this path anyway, so not worth adding now if it's just going to cause more pain in future refactoring.

I also like the idea of approach 5 instead of this - could start looking into that if you think it's a promising direction.

if you plan further refactoring of this code, I'm happy to just mothball this pr for now.

From today's call, I think the plan is to move forward with this first.

I also like the idea of approach 5 instead of this - could start looking into that if you think it's a promising direction.

Long-term I think this is the right way to go to get the general case right, so I'd encourage you if you're interested in trying to implement this on the EA- separate PR(s).