# Enhance afw table to support variable-length string data

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
6
• Sprint:
• Team:

#### Description

In order to persist lsst.afw.geom.SkyWcs in afw tables and FITS binary tables we want to expand afw table to support variable-length strings. It already supports variable-length numeric arrays, so Jim Bosch thinks adding this feature should be fairly straightforward.

#### Activity

Russell Owen created issue -
Field Original Value New Value
Epic Link DM-9679 [ 30784 ]
 Link This issue blocks DM-10765 [ DM-10765 ]
 Priority Undefined [ 10000 ] Major [ 3 ]
Hide
Russell Owen added a comment -

If you have time I would appreciate a full review. If not, I would still appreciate your weighing in on two things:

• How can I test my changes in FieldBase.h to FieldBase<std::string>::get[Const]Reference in Python? I think the changes are necessary if this code is reachable at all, but all of my attempts to access a string field using reference notation failed.
• Is my change to error handling in BaseRecord.cc commit 0c65e9f acceptable? I think the old test was inadequate because it ignored the case that the left-hand side was fixed and the right-hand side variable.
Show
Russell Owen added a comment - If you have time I would appreciate a full review. If not, I would still appreciate your weighing in on two things: How can I test my changes in FieldBase.h to FieldBase<std::string>::get [Const] Reference in Python? I think the changes are necessary if this code is reachable at all, but all of my attempts to access a string field using reference notation failed. Is my change to error handling in BaseRecord.cc commit 0c65e9f acceptable? I think the old test was inadequate because it ignored the case that the left-hand side was fixed and the right-hand side variable.
 Reviewers Jim Bosch [ jbosch ] Status To Do [ 10001 ] In Review [ 10004 ]
Hide
Jim Bosch added a comment -

I'll do a full review, but I'm afraid the answer to your first question is that it is not possible to test the reference changes from Python; that would require (at least) a mutable string type in Python.

Show
Jim Bosch added a comment - I'll do a full review, but I'm afraid the answer to your first question is that it is not possible to test the reference changes from Python; that would require (at least) a mutable string type in Python.
 Sprint Alert Production S17 - 6 [ 616 ] Alert Production S17 - 6, Alert Production F17 - 7 [ 616, 626 ]
 Rank Ranked higher
Hide
Jim Bosch added a comment -

Review complete. Comments on the GitHub PR.

The changes you mentioned in your second question are certainly an improvement. It's possible we could improve that function further by adding support for mixed fixed- and variable-length copies, but I'm not sure we need to and it's definitely good to throw if we're not supporting it.

Show
Jim Bosch added a comment - Review complete. Comments on the GitHub PR. The changes you mentioned in your second question are certainly an improvement. It's possible we could improve that function further by adding support for mixed fixed- and variable-length copies, but I'm not sure we need to and it's definitely good to throw if we're not supporting it.
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide
Russell Owen added a comment -

Show
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]
Hide
Russell Owen added a comment -

I have discovered a misfeature with variable-length strings in C++: one cannot specify a new variable-length field unless one also specifies a value for the doReplace flag. E.g.

 # this works schema.addField("foo", "docs for foo", 0, false)   # this fails: the compiler says it is ambiguous: schema.addField("foo", "docs for foo", 0) 

I'm not sure if there is anything reasonable that can be done about this. It is worrying because the compiler warning does not provide much help in figuring out the workaround.

Show
Russell Owen added a comment - I have discovered a misfeature with variable-length strings in C++: one cannot specify a new variable-length field unless one also specifies a value for the doReplace flag. E.g. # this works schema.addField<std::string>("foo", "docs for foo", 0, false)   # this fails: the compiler says it is ambiguous: schema.addField<std::string>("foo", "docs for foo", 0) I'm not sure if there is anything reasonable that can be done about this. It is worrying because the compiler warning does not provide much help in figuring out the workaround.

#### People

Assignee:
Russell Owen
Reporter:
Russell Owen
Reviewers:
Jim Bosch
Watchers:
Jim Bosch, Russell Owen