ItemDisplayInfo.db2 wrong cardinality
Opened this issue · 14 comments
It appears that layout 089404D9's GeosetGroup and AttachmentGeosetGroup cardinality should be 6 not 4 but I'm not sure if its me or the tools that are incorrect - can someone confirm?
Yup, meta dumps also say [6]. Probably need to add support into merger for that. Did they change it without updating layout hash (hope not) or is this wrong for all versions with that layout?
Super quick investigation:
- 26287 has the first layout hash of 089404D9 but has a cardinality of 4
- 26287 is in the 99606089 layout hash group in the DBD
- 26433 is the first build with a cardinality of 6
Fun.
I think that cardinality is in DBs anyway? That might be why it doesn't really matter for them either.
Regardless of whether it can be implied from reading DBs or not, we should probably still think on how we can fix this. Are we going to allow multiple layouthashes but for different sets of builds? Hmm.
For this special case one can just not list layout hash but only versions. I would not break the assumption that there is a 1:1 relation.
Not sure if it is a unique case. :D
In regards to the build being under the wrong layouthash, not sure what happened there. Really need to start working on that raw DBD dump per version thing so we can get a handle on easily checking those and where the issue came from. Will make a new issue to re-dump 26287.
We've known for a long time what the meta blocks are for and why they're not part of the layouthash so I'm a little surprised it hasn't cropped up sooner - then again it may have.
I think everyone would agree that it is wrong to leave incorrect values in the definitions just because DBs are self-documented now. My first thoughts are to either:
- Switch to symbolic notation for arrays i.e.
GeoSetGroup[]for WDC (or even just these specific cases). We can still document the size and build ranges in the comments so no data is lost. The byproduct would be enforcing correct DB reader implementations that can parse the cardinalities from the DBs themselves which isn't a bad thing. - Split definitions by structure instead and in these scenarios it is down to the implementation to request/use a build number to filter when duplicate hashes are available.
- Have ranges/multiple values for cardinalities. (This feels messy).
Not against doing 1. Dislike 2 and 3.
Issue with 26287 being in the wrong layout should be fixed now. Will check some other stuff as I also noticed we forgot to redo signedness changes for older builds. Not a giant deal but not super smart either.
@barncastle Are you sure that cardinality can always be read from DBs themselves?
Not read but calculated - yes. People need to use the field_structures if the compression type doesn't have cardinality info. My code is shit and doesn't do this but I can whip together an example to test this? FYI this should be applicable to WDB5+.
Simca did some wiki clarifications on how to calculate sizes and stuff that should help implementations. And your code isn't alone, most implementations currently don't do this correctly. We can probably move forwards with option 1 for the next format update (I want to batch a few things together instead of changing often).
Going off something I just added to DBDefsTest for WDC3, in 8.1.0.28048 (but probably much earlier) this seems to affect 3 DBs/4 fields. The holidays one is pretty crazy but seems to be correct.
[8.1.0.28048][itemdisplayinfo] Array length for field GeosetGroup wrong! DBC: 6, DBD: 4
[8.1.0.28048][itemdisplayinfo] Array length for field AttachmentGeosetGroup wrong! DBC: 6, DBD: 4
[8.1.0.28048][holidays] Array length for field Date wrong! DBC: 26, DBD: 16
[8.1.0.28048][itemsearchname] Array length for field Flags wrong! DBC: 4, DBD: 3