[ArrayManager] Array version of fillna logic
State: closed

xref https://github.com/pandas-dev/pandas/issues/39146

I was looking into getting rid of the Block fallback (`apply_with_block`) for `fillna` in ArrayManager. For that, I started with putting the bits and pieces from the several BlockManager methods (the different `fillna` implementations, maybe_downcast) together in a function that would do the same for plain arrays as input, to see what this would give.

The draft result of that can be seen in this PR. This seems to pass all tests for the ArrayManager, so should have the same behaviour as BlockManager.fillna. I only hoped that we could then also use this in the BlockManager to have a shared implementation (as we did for eg `Block.astype` and others). However, that doesn't seem so easy in this case .. So not yet sure how to best proceed here.

Labels:
name: Refactor
description: Internal refactoring of code
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
> However, that doesn't seem so easy in this case .. So not yet sure how to best proceed here.

Some exposition on ideas I've been looking at for this problem (for Block methods in general, not specific to fillna)

- Instead of defining `fillna_array` (or more generally `foo_array`) to return a single array, we could define it to return a list of arrays.
  - For AM this would always return a length-1 list.
  - For BM it would return
    - a) a length-1 list if there was no splitting,
    - b) a length-N list of 1-column arrays of it fully split (equiv split_and_operate or maybe_split), or
    - c) a length-2 list for `where_array`.
  This where_array case is annoying, could be made nicer if we changed Block.where to fully split instead of split in two pieces.  This would entail slightly more aggressive downcasting (breaks 1 test, though tentatively looks like this is surfacing an unrelated bug).
  - Block.convert is low hanging fruit for this approach.

- 2/3 uses of `@maybe_split` are on ObjectBlock methods.  If we could get `_downcast_2d` to only split for object dtype (tried it; a few tests break bc they downcast floats less aggressively), it would become easier to share some of functions (though exactly how escapes me ATM, i just remember trying this a couple weeks ago with this being the motivation)

- #40988 might make things marginally easier (though if theres a deprecation cycle, it will take too long to really be useful for this)

- The complexity reduction of ArrayManager mostly comes from getting rid of BlockManager, not from getting rid of Block.  We could just learn to live with dispatching to blocks for some things.
  - One _disadvantage_ of the recent push to share methods across Block subclasses is that we end up repeating dtype checks.  I doubt it makes a big difference, but haven't measured.
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.
can you address @jbrockmendel comments
Started looking at this again, for now only simplified the `can_hold_element` usage. Still need to look into / respond to the other comments.
> * Instead of defining `fillna_array` (or more generally `foo_array`) to return a single array, we could define it to return a list of arrays.

I now shared the implementation for the ExtensionBlock (subclasses), which was essentially just the datetime64 special casing for allowing object fallback (but good to have this in one place). But for sharing the generic Block implementation something like the above would indeed be needed. This splitting into multiple arrays is very much block specific, though, and personally I would prefer to keep that logic on the Blocks itself.
I increasingly think that this type of change should be left until _right_ before BlockManager is ripped out.  Until then, this just duplicates code and makes it easy for behavior to accidentally drift apart (xref #40988, #45153)
@jorisvandenbossche if you can rebase
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.