Looks good! Though while I like the generality of generating possibly more types of classes on demand, I wonder if for code clarity it wouldn't be simpler to just define the two cases we need explicitly. I guess one issue is that the names would clash, correct? ... ... ... Hmm, thinking more, your approach also has the advantage that any subclasses automatically do the right thing. So, would seem all fine.

Right, it had to be done this way to work for any BaseColumn subclasses that come along as well (for example, it works transparently for the Column class too).

One disadvantage is that it could affect subclasses that for whatever reason do not need this workaround, or implement a different __getitem__ entirely. It's a corner case but it could happen. I think I'll add one tweak to enable that...

Actually, nevermind, doesn't need any tweak. Since _get_nd_proxy_class is a classmethod, any subclass can still override it. They could even do:

@classmethod
def _get_nd_proxy_class(cls):
    return cls

to make it effectively a no-op, if they don't need this work-around.

This looks cool, I learned something new! Just one question and one request.

First, is this MRO at all a problem (that it includes Column twice)?

In [7]: c.__class__.__mro__
Out[7]: 
(astropy.table.column.Column,
 astropy.table.column._NDColumnMixin,
 astropy.table.column.Column,
 astropy.table.column.BaseColumn,
 numpy.ndarray,
 object)

Second, can you just name this_NDColumn instead of _NDColumnMixin? Within Table we have co-opted the Mixin suffix to mean something specific and different (in particular that it doesn't behave entirely like a Column).

First, is this MRO at all a problem (that it includes Column twice)?

No, both of those "Columns" are actually different classes. The first is the new class that gets generated with the bases (_NDColumnMixin, Column), where the second "Column" is the original Column class. I ping-ponged on whether or not to make it take the same name, because it is obviously confusing, but for this to work entirely transparently it has to (otherwise you'll get funky results on any code that uses self.__class__.__name__ for example). It's just something to be aware of I guess.

Second, can you just name this_NDColumn instead of _NDColumnMixin?

It really is a mixin class though. It's composed with other, more concrete classes through multiple-inheritance to add/override one piece of functionality. Does it matter so long as it's an internal implementation detail most people will never see?

Seems like a nice solution.

Don't hold the PR up for this, but I wonder if it would benefit from more generality if _get_nd_proxy_class took the data as an argument and could act accordingly (moving the if self_data.ndim > 1: into _get_nd_proxy_class). That way subclasses (or future necessary workarounds like this) could act on more features of the data than just the fact that it's n-dimensional.

It really is a mixin class though. It's composed with other, more concrete classes through multiple-inheritance to add/override one piece of functionality. Does it matter so long as it's an internal implementation detail most people will never see?

I agree that this is a technically accurate name in the broader context, but for anyone that starts looking at Table code it will be confusing because of the ubiquitous and conflicting usage of mixin.

@taldcroft Okay, this PR is confusing enough as it is I don't want to make it worse. What about _NDColumnShim? :) (I just don't want to give the impression that it's a column type itself).

Don't hold the PR up for this, but I wonder if it would benefit from more generality if _get_nd_proxy_class took the data as an argument

I thought of that, and then didn't do it. Not sure why though--now that I think about it again I like it.

Looks good to me @embray, thanks!

👍 from me!