VO table no longer access char field as bytes - #9505

This PR has been updated to ensure that it handles both fixed-length and variable length strings. The upshot is that for FIELDs with datatype="char", the values will always be stored in the astropy Table as str instead of bytes (e.g., 'myvalue' instead of b'myvalue').

That behavior was already the case for FIELDs with datatype="unicodeChar", so the general idea was to make the char handling more like unicodeChar, but with some differences depending on the serialization.

For TABLEDATA serialization:

• char is read in just like unicodeChar except that if non-ASCII characters are present for char, a warning is issued. This behavior is similar to many existing VOTable parsers which also allow non-ASCII characters, often without a warning.
• On write, an exception is raised if attempting to write non-ASCII characters to a char FIELD. The stricter enforcement here is to prevent astropy from writing a VOTable that is not compliant with the spec.

For BINARY and BINARY2:

• The FIELD datatype determines the number of bytes per character, so char values are ASCII-encoded into strs, and unicodeChar values are UTF-16-BE -encoded into strs.

For all serializations, the astropy Table column dtype is O for variable length strings and U<length> for fixed length strings. This is consistent with the prior behavior except that fixed length char strings were S<length>.

Without having looked at the patch as such – someone will have to poke me again for that –, I heartily support the intent of the PR. VOTable char[] most definitely is supposed to be an ASCII string (as opposed to unsignedByte), and so it shouldn't have been turned into bytes in the first place.

I kind-of also support interpreting content as utf-8; I've spoken for this ages ago (http://mail.ivoa.net/pipermail/apps/2014-October/001010.html is a thread from 2014), and if astropy establishes a quasi-standard here, that may actually push this along. It should be made clear somewhere that it's possible that behaviour with non-ASCII in char is actually undefined by the IVOA.

Of course, this will break some code that did something with strings from VOTable. But bytes coming out of VOTable are so painful that to me that seems worth it.

Hence: informally, +1.

Reading the ascii datatype as unicode char is fine, as long there is a warning emitted. This is necessary for writing a service compliance checker with astropy/pyvo (This idea came up somewhere).

I'm missing a vo_raise(E24) when trying to write out non ascii values in the output method of the char datatype.

@tomdonaldson , do you think this is ready for another round of review, or are you still waiting to add more code or hear back from VO stakeholders?

I'm missing a vo_raise(E24) when trying to write out non ascii values in the output method of the char datatype.

@funbaker Thanks for noticing that! I've added code to the Char converter that warns if the value contains non-ASCII characters, and added corresponding test cases. Perhaps that check can be a little cleaner with isascii() once we require Python 3.7+.

@pllim With this last commit, this is ready for another round of review. Perhaps @funbaker and/or @hugobuddel could also have another look? It would be great to get this in for the upcoming 4.0.1 release.

To summarize, I believe we've changed the behavior to parse all char and unicodeChar VOTable FIELDs into str instead of bytes objects (allowing but warning on non-ASCII values for the char case). We've also added checks on outputting non-ASCII values to char FIELDs (warnings for TABLEDATA serialization and errors for BINARY and BINARY2 serializations). Outside of those changes, the previous behaviors, assumptions and test coverage should have been preserved.

Perhaps that check can be a little cleaner with isascii() once we require Python 3.7+

Note to self: Open a follow-up issue for this after this PR is merged.

Now I gotta clean up astroquery.vo_conesearch... 😅

@bsipocz and @tomdonaldson , as discussed on Slack, since this change is causing some downstream breakage (e.g., astroquery), I am remilestoning to 4.1 instead of 4.0.1. It is my understanding that there is no action needed on my part in terms of consolidating the change log. This PR needs to be backed out of the 4.0.x branch too. I apologize for any inconvenience caused.