Fix swapped values when reading BINARY votable columns out of file order - #20513

What is the user really asking for when they give columns=["a", "a"]?

A mistake? This is a very corner case that sorta falls out with the fix. They get precisely one column back, column a. We don't need the unit test if it is bothersome - i.e. we don't want to have a unit test that codifies unlikely API requests. Before it raised an IndexError, which was not intuitive either.

Probably the more likely case is that they give pass 15 of the 96 columns names to the parameter, and accidentally repeat one of them. Now it gracefully gives them that column back once instead of IndexError.

I would argue that if I give columns=["a", "a"], I would expect duplicate columns or an error telling me that I cannot do it (if duplicate columns is against the standard). But I also don't use VOTable much.

That could be done. Currently columns don't come back in order that that they are listed in the columns=[...] param, and they never have. So the list is not absolute in the sense of, "produce a table of exactly these columns in this order". We've always returned them in the order they are in the table. So if I passed:

columns = ["flux", "error", "mag_a", "mag_a_error", "flag1", "flux"]

I still get back flux once, and I get them back in the column order they are in the input table.

If we want something different, that's probably an interface change and outside the scope of the this bugfix. Though we could certainly warn on duplicates, but it would be more code, and I like deleting code, not adding more. 😅

I am just a tiny tiny bit worried because the behavior is changing from explicit (ugly traceback) to implicit (silently dropping duplicate). I cannot decide if that is better or not. I'll defer to other votable maintainers. Thanks for the clarification!

@stvoutsin any thoughts on whether we should raise an exception with an informative message if someone lists the same column more than once in columns=[...]? Or silently just give them the column once?

I do see the worry, but all things considered my inclination (at least for this bugfix) is to return it once, since that's what TABLEDATA already does and we're just matching it for BINARY/BINARY2.

Worth noting the IndexError before wasn't really a check against duplicates, it was the binary reader going past the end of the row because colnumbers didn't line up with the output columns. But It never mentions duplicates and only happens with certain serializations.

Since columns= doesn't control output order my reading of it is as "which columns do I want" rather than an exact spec so you still get everything you asked for.
Returning it twice would mean making up a second name since the array can't have duplicate field names, which feels wrong to me

Whether to warn is a good question, I'd lean towards not warning. TABLEDATA returns the column once and it doesn't sound like this is causing problems.
It's very possible the duplicate is a typo for another column, but they'd hit a KeyError the first time they try to use it, which I don't think is much worse than if we had warned them.

Either way if we did want to change the behaviour, warning or raising an error, I'd propose we do it as a follow-up rather than in this fix. Happy to go the other way if others feel strongly though.

So after some poking around, trying to understand what's going on, I see that since astropy 7.0.0, when the format is tabledata_format="tabledata" and you list the same column twice, you get it back once, and it is silent on the duplication. So this PR is just aligning binary/binary2 formats with existing tabledata format behavior.

Before astropy 7, all formats (tabledata/binary/binary2) returned a masked array table with just the single column having data, or (nans), and this goes way back to when columns=[...] was introduced. So it has been this way for years. It was only the astropy 7+ fix of returning a table without the columns instead of as masked that introduced the regression fixed here.

On main and tag 7.2.2:

from astropy.table import Table
from astropy.io.votable import parse_single_table

Table({"a": [1.0, 2.0], "b": [3.0, 4.0]}).write(
    "test.vot", format="votable", tabledata_format="tabledata"
)

parsed = parse_single_table("test.vot", columns=["a", "a"])
print(parsed)
#  a 
# ---
# 1.0
# 2.0

With astropy 6, tabledata format returned a masked array table when you asked for "a" twice:

 a   b 
--- ---
1.0  --
2.0  --

and binary/binary2 format returned nans:

 a   b 
--- ---
1.0 nan
2.0 nan

So it has never raised except for binary/binary2 format in astropy 7+, which we are now fixing in this PR.

I think if we wanted it to raise an exception intentionally, then we should change that for all 3 formats, and it would be an API change. And probably separate from this PR. Thoughts?