Fix handling of bounded variable-length char arrays in BINARY2 format by stvoutsin · Pull Request #18105 · astropy/astropy

stvoutsin · 2025-05-08T03:10:04Z

Description

This pull request is to address issue #18099 with VOTable BINARY2 serialization where fields with bounded variable length char arrays (e.g., arraysize="128*") were being handled as fixed length.

Specifically, the parser was removing the asterisk from the arraysize attribute and treating these fields as fixed-length arrays, which caused them to be incorrectly encoded/decoded in BINARY2 format.

The issue resulted in either empty tables or exceptions when parsing VOTable results containing these field types.

Changes

Modified the Char converter class's __init__ method to detect bounded variable length arrays (e.g. arraysize="128*") and use the appropriate var parsing methods
Preserved the original arraysize format with the asterisk to ensure it's correctly written back during serialization
Added bounds checking in variable length array handlers to properly validate against the max lengths
Added unit tests to verify correct handling of bounded variable length char arrays
Changed existing unit tests to ensure these field type metadata is properly propagated through the roundtrips.

Technical Details

The issue was caused by this code in the Char converter's __init__ method:
field.arraysize = field.arraysize.removesuffix("*")

This was removing the asterisk and therefore losing the information that this should be handled as a variable length field with an upper bound, which later in the __init__ method lead to the fixed length binary parsing methods being used. The changes should address this by recognizing that the field is a variable length field with an upper bound and assigning the converted the appropriate binary parsing methods. The changes also address the round-tripping of this information between astropy/votable tables.

Testing

Added following new tests:

test_binary2_single_bounded_char_array: Test that variable length char arrays with a max length arraysize="128*") are read correctly in BINARY2 format.
test_binary2_bounded_variable_length_char: Tests round-trip serialization of bounded variable length char arrays across all formats
test_binary2_bounded_variable_length_char_edge_cases: Tests edge cases
test_binary2_char_fields_vizier_data: Test parsing a VOTable from a Vizier query which includes bounded variable-length char arrays (e.g., arraysize="7*"). Related issue: votable binary parsing appears to fail on null columns #8737

This fix is particularly important for interoperability with IVOA services that commonly use this field type in their response formats.

Related Issues

Fixes #18099
Fixes #8737

Notes:

Let me know if I've misinterpreted the issue or if there are better ways to address it and I'd be happy to make any changes accordingly.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2025-05-08T03:10:29Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions

Welcome to Astropy 👋 and congratulations on your first pull request! 🎉

A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.

If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail [email protected].

stvoutsin · 2025-05-08T17:51:49Z

Looks like the test data files I added are not properly being resolved in the GH workflow. I've set this PR as a draft while I sort out what went wrong.

Update: Was missing a test data file from the commit, that has now been added

Co-authored-by: P. L. Lim <[email protected]>

pllim · 2025-05-08T18:59:46Z

You can ignore the "dev version" allowed failure job. I restarted RTD.

ManonMarchand

Thanks for the PR. I checked in the branch, and it works.

My only comment is that the tests could have an unicodeChar field instead of one of the char ones.

stvoutsin · 2025-05-13T00:25:08Z

Thanks for the PR. I checked in the branch, and it works.

My only comment is that the tests could have an unicodeChar field instead of one of the char ones.

Thanks for the comment and for checking the branch @ManonMarchand .I've added a couple more unit tests for checking UnicodeChars & for validating handling of bounds.

…hars

ManonMarchand · 2025-05-13T07:55:33Z

Looks good to me, thanks!
Maybe @bsipocz or @tomdonaldson would like to review too?

pllim · 2025-05-20T13:50:43Z

Alas this missed v7.1. Another approval would be nice. Thanks, all.

bsipocz

I only have minor comments, take or leave them as you wish.

bsipocz · 2025-05-20T21:42:40Z

+        if self.arraysize != "*" and len(value) > self.arraysize:
+            vo_warn(W46, ("char", self.arraysize), None, None)
+            value = value[: self.arraysize]


is this a reasonable behaviour when len(value) > self.arraysize, should this maybe raise an exception rather than just a warning?

That's an interesting point, are you suggesting we raise an exception in the output methods or the parse methods as well? I've gone with raising warning to keep consistent with the default Char parse method and generally in favour of being more forgiving for users, but I'm happy to also raise an exception if you think this is more appropriate here. If so do you want me to do the same for the parsing method?

Yes, but neither the Char parser no here in e.g. L385 has any value loss, while in this method you chop the return at arraysize.
Maybe keep it a warning but don't chop?

Thinking about this I think for consistency it may be best to truncate in the parser and the output method.
Providing a warning without truncating has potential to cause unexpected compatibility issues later on for users, for example if a user pre-allocates fixed buffers based on declared length. Also it feels wrong to generate a Table that will not conform with the schema.

If we didn't want to truncate, I'd also consider increasing the arraysize along with a warning, but that may also lead to unexpected issues.

Any thoughts on truncating both at parse and output? Otherwise I can also just not chop, the potential for the error occurring through some round-trip where on the other end they assume data that conforms with the bound is probably rare.

I would lean towards not truncating in here, so it will be consistent (even if it's wrong) with the rest of the methods, and thus can be easily be backported. Then this PR can go in now without further discussion.

But then open a bug report issue asking for input/discussion, and possibly changing the behaviour, either by truncating or changing the schema. The former feels more correct to me, but I would like to see the input from others, too. Then potentially a fix/refactor; that is consistent with the Char methods, too, can go in the next feature release as a breaking change.

Ok sounds good, I've removed the truncation for the output method. I've also added a few more test methods since I noticed codecov was complaining about the coverage. Let me know if you think anything else needs to be changed.

Thanks!

Not sure why codecov is still complaining, I would suggest to ignore it for now and go ahead with the merge once CI is all green.

Would you mind opening an issue outlining the concerns you mentioned in #18105 (comment) ?

Sure, new issue created here: #18167

By the way I noticed the CI failed during the link check here:

(api/astropy.io.votable.validator.make_validation_report: line 23) broken http://www.starlink.ac.uk/stilts - HTTPConnectionPool(host='www.starlink.ac.uk', port=80): Max retries exceeded with url: /stilts (Caused by ConnectTimeoutError(<urllib3.connection.HTTPConnection object at 0x7f33be2d7350>, 'Connection to www.starlink.ac.uk timed out. (connect timeout=180)'))

Is this a known issue? I noticed that it is using an older URL for the starlink docs:

astropy/docs/conf.py

Line 612 in 16a94b4

"STIL": "http://www.starlink.ac.uk/stil",

This should be the new URL I think:
https://www.star.bristol.ac.uk/mbt/stilts/

Unrelated, so ignore it or open a separate PR.

(actually I'm not sure why we require the extra CIs for this PR, but at this point I'm just waiting for all the jobs on the exotic arc to finish)

bsipocz · 2025-05-20T21:46:57Z

-            field.arraysize = field.arraysize.removesuffix("*")
+            is_variable = field.arraysize.endswith("*")
+
+            # Get numeric part without modifying field.arraysize
+            numeric_part = field.arraysize.removesuffix("*")


We could uniformize the comments here with between L326:L330 as currently they are complimenter for the same code content.

Yes good point, should be done in new commit

bsipocz · 2025-05-20T21:49:28Z

+        if self.arraysize != "*" and len(value) > self.arraysize:
+            vo_warn(W46, ("unicodeChar", self.arraysize), None, None)
+            value = value[: self.arraysize]
+            encoded = value.encode("utf_16_be")


remove the same line from L497 and deindent this one

…e in b2 output method

… arraysize bound

bsipocz

Thanks!

…ar arrays in BINARY2 format

bsipocz · 2025-05-22T01:03:26Z

+
+        if self.arraysize != "*" and len(value) > self.arraysize:
+            vo_warn(W46, ("unicodeChar", self.arraysize), None, None)
+            value = value[: self.arraysize]


Ahh, review mistake, I didn't spot that this line still truncate and is now inconsistent with the same method in Char

Is a follow-up PR planned? Thanks!

I think we should do one, whatever the output of the conversation in #18167 is, we don't want a different behavior between the two field types.

Oh I didn't notice that inconsistency, my bad!

…105-on-v7.1.x Backport PR #18105 on branch v7.1.x (Fix handling of bounded variable-length char arrays in BINARY2 format)

…archarfix" This reverts commit 00f164d, reversing changes made to 16a94b4.

pllim · 2025-05-27T21:55:00Z

Alas, I think this broke astroquery, see #18179

Original PR #18105 by stvoutsin Original: astropy/astropy#18105

…rrays in BINARY2 format Merged from original PR #18105 Original: astropy/astropy#18105

Original PR #18105 by stvoutsin Original: astropy/astropy#18105

…rrays in BINARY2 format Merged from original PR #18105 Original: astropy/astropy#18105

Fix handling of bounded variable-length char arrays in BINARY2 format

7c632a3

github-actions Bot added the io.votable label May 8, 2025

github-actions Bot reviewed May 8, 2025

View reviewed changes

pllim added Bug backport-v7.0.x labels May 8, 2025

pllim added this to the v7.0.2 milestone May 8, 2025

pllim requested review from ManonMarchand and bsipocz May 8, 2025 15:25

pllim reviewed May 8, 2025

View reviewed changes

Comment thread docs/changes/io.votable/18105.bugfix.rst Resolved

stvoutsin marked this pull request as draft May 8, 2025 17:50

pllim reviewed May 8, 2025

View reviewed changes

Comment thread astropy/io/votable/tests/test_table.py Resolved

Add period to the end of the change rst file

04ce208

Co-authored-by: P. L. Lim <[email protected]>

stvoutsin force-pushed the u/stvoutsin/varcharfix branch from 1916022 to 04ce208 Compare May 8, 2025 18:29

stvoutsin marked this pull request as ready for review May 8, 2025 19:08

ManonMarchand reviewed May 12, 2025

View reviewed changes

saimn modified the milestones: v7.0.2, v7.1.1 May 12, 2025

stvoutsin marked this pull request as draft May 13, 2025 01:01

Added some additional unit tests for out-of-bound fields and UnicodeC…

9f90029

…hars

stvoutsin force-pushed the u/stvoutsin/varcharfix branch from 55b9690 to 9f90029 Compare May 13, 2025 01:05

stvoutsin marked this pull request as ready for review May 13, 2025 01:50

ManonMarchand approved these changes May 13, 2025

View reviewed changes

pllim removed the backport-v7.0.x label May 13, 2025

pllim removed this from the v7.1.1 milestone May 13, 2025

pllim added this to the v7.1.0 milestone May 13, 2025

pllim requested a review from tomdonaldson May 13, 2025 12:38

pllim added the Extra CI Run cron CI as part of PR label May 13, 2025

pllim modified the milestones: v7.1.0, v7.1.1 May 20, 2025

bsipocz approved these changes May 20, 2025

View reviewed changes

Uniformize the comments for common behaviour and remove redundant lin…

162c7c9

…e in b2 output method

stvoutsin marked this pull request as draft May 21, 2025 21:25

Remove truncation in char b2 output and add tests for cases exceeding…

dd40eec

… arraysize bound

stvoutsin marked this pull request as ready for review May 21, 2025 21:38

bsipocz approved these changes May 21, 2025

View reviewed changes

bsipocz merged commit 00f164d into astropy:main May 22, 2025

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request May 22, 2025

Backport PR astropy#18105: Fix handling of bounded variable-length ch…

e0f4d1f

…ar arrays in BINARY2 format

meeseeksmachine mentioned this pull request May 22, 2025

Backport PR #18105 on branch v7.1.x (Fix handling of bounded variable-length char arrays in BINARY2 format) #18170

Merged

bsipocz reviewed May 22, 2025

View reviewed changes

bsipocz added a commit that referenced this pull request May 22, 2025

Merge pull request #18170 from meeseeksmachine/auto-backport-of-pr-18…

10a4d0a

…105-on-v7.1.x Backport PR #18105 on branch v7.1.x (Fix handling of bounded variable-length char arrays in BINARY2 format)

pllim added a commit to pllim/astropy that referenced this pull request May 27, 2025

Revert "Merge pull request astropy#18105 from stvoutsin/u/stvoutsin/v…

5cc66f3

…archarfix" This reverts commit 00f164d, reversing changes made to 16a94b4.

pllim mentioned this pull request May 27, 2025

Revert "Merge pull request #18105 from stvoutsin/u/stvoutsin/varcharfix" #18179

Closed

3 tasks

This was referenced May 28, 2025

MAINT: astropy 7.1.1 compatibility in tests astropy/astroquery#3325

Merged

MAINT: making code self-consistent, remove truncation #18181

Merged

ryantqiu mentioned this pull request Oct 1, 2025

Fix handling of bounded variable-length char arrays in BINARY2 format snorkel-marlin-repos/astropy_astropy_pr_18105_e4d6461e-88f7-4b5d-adbc-aae837d8b8fa#1

Merged

ryantqiu mentioned this pull request Oct 2, 2025

Fix handling of bounded variable-length char arrays in BINARY2 format snorkel-marlin-repos/astropy_astropy_pr_18105_b9935d72-7f86-4ad0-b9c1-4974e723a0cc#1

Merged

Conversation

stvoutsin commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Technical Details

Testing

Related Issues

Notes:

Uh oh!

github-actions Bot commented May 8, 2025

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stvoutsin commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pllim commented May 8, 2025

Uh oh!

ManonMarchand left a comment

Choose a reason for hiding this comment

Uh oh!

stvoutsin commented May 13, 2025

Uh oh!

ManonMarchand commented May 13, 2025

Uh oh!

pllim commented May 20, 2025

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented May 27, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

stvoutsin commented May 8, 2025 •

edited

Loading

stvoutsin commented May 8, 2025 •

edited

Loading

bsipocz May 21, 2025 •

edited

Loading