On Mon, Jul 13, 2020 at 4:31 PM Micah Kornfield <emkornfi...@gmail.com> wrote: > > > > > > > That static cast is currently undefined behavior. > > Is ubsan reporting this? When looking into the feature enum I tried to > understand if that was valid. At the time I read the C++ spec* if the enum > has an explicitly declared type, all values in that types range are > supported.
We don't have any test cases that have a future metadata version. I made a branch where I added V6 and wrote an IPC message, then found that I was unable to determine that it was out of bounds (presumably UBSAN would error, though, but we need a runtime error outside of ASAN/UBSAN). > The generated enums provide a "max" [1] value that should be comparable > against. > <https://github.com/apache/arrow/blob/master/cpp/src/generated/Schema_generated.h#L109> > > > * I am not a C++ lawyer > > [1] > https://github.com/apache/arrow/blob/master/cpp/src/generated/Schema_generated.h#L109 > > On Mon, Jul 13, 2020 at 2:19 PM Wes McKinney <wesmck...@gmail.com> wrote: > > > I've discovered while working on ARROW-9399 that it is very difficult > > with the Flatbuffers API in C++ to detect a MetadataVersion [1] that > > is higher than the current version. > > > > For example, suppose that 3 or 4 years from now we move from version > > V5 to version V6. The generated Flatbuffers code looks like this > > > > org::apache::arrow::flatbuf::MetadataVersion version() const { > > return > > static_cast<org::apache::arrow::flatbuf::MetadataVersion>(GetField<int16_t>(VT_VERSION, > > 0)); > > } > > > > That static cast is currently undefined behavior. > > > > One way to deal with this would be to add placeholder future versions > > (e.g. V6 and V7). > > > > Another backward-and-forward-compatible option would be to return the > > version as a short (int16_t) rather than the enum value, which is > > subject to this brittleness. > > > > Either way unfortunately I think adding forward compatibility checks > > is out of scope for 1.0.0 and the risk is low since we don't > > anticipate bumping the version anytime soon. > > > > Thanks, > > Wes > > > > [1]: https://github.com/apache/arrow/blob/master/format/Schema.fbs#L22 > >
