DM-32138: Remove exposureId from VisitInfo #697
Conversation
parejkoj
force-pushed
the
tickets/DM-32138
branch
2 times, most recently
from
507d310
to
63e7640
Compare
| // exposureId was removed in version 5, but we need to reserve space in the schema for old files. | ||
| // We don't save the field record, as we don't need to read into it. | ||
| schema.addField<table::RecordId>("exposureid", "exposure ID", ""); | ||
| } |
There was a problem hiding this comment.
This seems... not very portable if there are more schema changes in the future; the logic for exactly which fields belong in which version could get quite twisty. You also have to sacrifice the benefits of one-time initialization to do it this way. And then there's the potential confusion from VisitInfoSchema acting like a singleton but not actually being one (a dualton?).
Why not just have separate classes for versions 0-4 and 5+, and make VisitInfoFactory responsible for initializing the correct schema? Then you could extend to future incompatibilities without worrying about accidentally changing the old stuff.
There was a problem hiding this comment.
I'd tried your latter approach, but ran into C++ compiler problems that I couldn't sort out. This approach was @TallJimbo 's suggest (on top of my initial attempt that forgot about the static members and had the "get which version" in the factory).
Line number no longer accurate, but this was the error (I had made a VisitInfoSchemaWithExposureId subclass):
src/image/VisitInfo.cc:322:39: error: member initializer 'schema' does not name a non-static data member or base class
VisitInfoSchemaWithExposureId() : schema() {
There was a problem hiding this comment.
🤦 Right, I've been working in Python for too long. But this version's also potentially unsafe re: its API (you're fine this time because you don't care about the expId key in either case), so maybe file a ticket to clean up the implementation later?
There was a problem hiding this comment.
I'm game to try another approach here, if you have a suggestion (or a way around the error above, which I think is due to static-ness of schema shared between the parent and child classes). I think CRTP would be one way around it.
There was a problem hiding this comment.
I don't understand why schema would be static (it's not so in the current or previous versions?), but I don't think this is a problem we can solve in <1 hour. The current code at least gives the desired behavior for now.
There was a problem hiding this comment.
We have to preserve the schema order for older versions that don't have exposureId.
parejkoj
force-pushed
the
tickets/DM-32138
branch
from
63e7640
to
3891491
Compare
No description provided.