Backporting BJData features to UBJSON - ND array, u/m/M/h markers and handling of NaN

[fangq]

@Steve132, Re your comments at Steve132/ubj#8 and #108

I think the right step forward for the project would be to agree on an
array spec, agree on other changes, and move forward to a 1.0 spec, along
with reference implementations in as many languages as possible and a test
suite.

Again, I am happy to assist on backporting the new syntax to UBJSON so we do not have to maintain two separate specs.

The main additions made in BJData Draft 1 include

1. an optimized ND array container (via # followed by a 1D array object)

https://github.com/OpenJData/bjdata/blob/Draft_1/Binary_JData_Specification.md#optimized-n-dimensional-array-of-uniform-type

2. new dedicated markers for unsigned integers and half-precision floating-point numbers
   NeuroJSON/bjdata@95544dc

3. Instead of converting NaN/+-infinity to null, BJData keeps their respective IEEE 754 binary forms
   NeuroJSON/bjdata@f3d338f#diff-d94a316d99f805938f1320db036a84a5L174

I will explain the rationales for each of my additions, and we can discuss/approve/disapprove in this thread.

[Steve132]

Some reasons why I don't like your ND array spec is that 1) The way you use # is ambiguous with the count specifier. 2) the way you use [ seems to add an extra character where it's not needed. 3) It explicitly disallows multidimensional arrays of non-fixed or structured type (which is an unusual but reasonable use case that I think UBJ should support) 4) it seems to violate the TLV 'convention' we use somewhat. But yes, I think we should work together on merging the specs if possible.

…

On Wed, May 13, 2020 at 10:20 AM Qianqian Fang ***@***.***> wrote: @Steve132 <https://github.com/Steve132>, Re your comments at Steve132/ubj#8 <Steve132/ubj#8> and #108 <#108> I think the right step forward for the project would be to agree on an array spec, agree on other changes, and move forward to a 1.0 spec, along with reference implementations in as many languages as possible and a test suite. Again, I am happy to assist on backporting the new syntax to UBJSON so we do not have to maintain two separate specs. The main additions made in BJData Draft 1 include 1 an optimized ND array container (via # followed by a 1D array object) https://github.com/OpenJData/bjdata/blob/Draft_1/Binary_JData_Specification.md#optimized-n-dimensional-array-of-uniform-type 2 new dedicated markers for unsigned integers and half-precision floating-point numbers ***@***.*** <NeuroJSON/bjdata@95544dc> 1. Instead of converting NaN/+-infinity to null, BJData keeps their respective IEEE 754 binary forms ***@***.***#diff-d94a316d99f805938f1320db036a84a5L174 <NeuroJSON/bjdata@f3d338f#diff-d94a316d99f805938f1320db036a84a5L174> I will explain the rationales for each of my additions, and we can discuss/approve/disapprove in this thread. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#109>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGWIMAE6CT5637MMPTSJGLRRKUBHANCNFSM4M7ZM7PQ> .

[fangq]

For the ND array construct, I am aware that @Steve132 has a similar proposal using the below construct

[[] [$] [type] [@] [1-byte Ndim] [Nx type] [Nx] [Ny type] [Ny] ... [serialized ND array in column-major order]

the ND array optimized container used in BJData was originally proposed in this mailing list post back in 2013, it is done via #+a UBJSON 1D integer array object, followed by row-major serialized array elements, for example

[[] [$] [type] [#] [[] [$] [Nx type] [#] [Ndim type] [Ndim] [Nx Ny Nz ...]  [Nx*Ny*Nz*...*sizeof(type)]
  or
[[] [$] [type] [#] [[] [Nx type] [nx] [Ny type] [Ny] [Nz type] [Nz] ... []] [Nx*Ny*Nz*...*sizeof(type)]

The main reasons that had led to my prefer to the #+[ construct are

1. it does not need to add handling to a new marker in an existing parser, instead, one can just call the array parsing function to read out the dimensions when #+[ is encountered. You can see this in below examples:

• for JSONLab: https://github.com/fangq/jsonlab/blob/master/loadubjson.m#L135-L138
• for py-ubjson: NeuroJSON/pybj@b90e140#diff-fae7cbc0e5df9b9d3d69d5cc4453b6feR226-R229
• for ubj: fangq/ubj@cca51f6#diff-572570f413b5480c5985c6a5c9ede5b2R359

in my opinion, this requires the least amount of work for parser updates

2. it does not limit the maximum dimension to 255 as the @ construct

3. the row-major byte order aligns with the conventions for the majority of programming languages, such as C, C++, python, Javascript Typed array etc, therefore, the data do not require transpose when saving/reading.

4. it feels intuitive because for an ND array, its count is not a single number but a 1D vector.

5. This construct also align with the general template construct I am about to propose - by allowing $+{ or #+{ to represent payload less object template, similar to what I proposed for messagepack: Add support for typed N-D array to simplify large array storage msgpack/msgpack#267 (comment)

6. last, but not the least, the #+[ construct has been used in JSONLab since 2013, and currently has accumulated about 45,000 downloads from matlab file exchange, and about 1000 clones from github per week. Changing this syntax will render all previously generated files incompatible.

[fangq]

1. The way you use # is ambiguous with the count specifier.

I don't think so - in draft 12, # is supposed to be followed by one of the integer markers U,i,I,l,L and will be in no way to be confused with [.

3. It explicitly disallows multidimensional arrays of non-fixed or structured type (which is an unusual but reasonable use case that I think UBJ should support)

For extending to template objects, I reserved $+{ or #+{ for this purpose, which will also capitalize on the existing object parsing functions to handle the following structure without needing to write a specialized parsing routine. Similar to the ND array extension, I expect this to be much easier to be implemented from a parser's standing point.

[Steve132]

it does not need to add handling to a new marker in an existing parser,

instead, one can just call the array parsing function to read out the dimensions when #+[ is encountered. You can see this in below examples: That's actually a good argument *against* doing this. Parsing the array as an array like you've done here in code is a bug, because it actually allows people to use nd-arrays (and arrays of strings or objects or floats) as the dimension specifier. We don't *want* parsers to be encouraged to re-use the existing array parsing code, because if they do that's incorrect and leads to bugs. Really what your spec says is that you need to either implement most of the array parser twice (one for the general version, and again for the constrained version) OR you need to add a 'constrain to 1d array' flag to the code.

it does not limit the maximum dimension to 255 as the @ construct

I'd say that, in practice, this limitation isn't an actual problem. I've never seen a 256-dimensional array anywhere in actual usage for any problem domain. But sure.

the row-major byte order aligns with the conventions for the majority of

programming languages, such as C, C++, python, Javascript Typed array etc, therefore, the data do not require transpose when saving/reading. I actually completely agree with this.

it feels intuitive because for an ND array, its count is not a single

number but a 1D vector. I understand your argument here. One concern that I have: in UBJ I'd want to relax the restriction that an ND-array must have a type. An array can have a count but not a type in UBJSON. To me it seems like it's reasonable to do an ND array of unknown things.

This construct also align with the general template construct I am about

to propose - by allowing $+{ or #+{ to represent payload less object template, s $+{ already has a meaning in UBJSON. It's a fixed array where each item is an object. The only available place for extension is #{ I think we can come together to find a middle ground spect here that doesn't break backwards compatibility for you in 99% of cases but fixes some of these problems.

…

On Wed, May 13, 2020 at 10:54 AM Qianqian Fang ***@***.***> wrote: For the ND array construct, I am aware that @Steve132 <https://github.com/Steve132> has a similar proposal using the below construct [[] [$] [type] [@] [1-byte Ndim] [Nx type] [Nx] [Ny type] [Ny] ... [serialized ND array in column-major order] the ND array optimized container used in BJData was originally proposed in this mailing list post back in 2013, it is done via #+a UBJSON 1D integer array object, followed by row-major serialized array elements, for example [[] [$] [type] [#] [[] [$] [Nx type] [#] [Ndim type] [Ndim] [Nx Ny Nz ...] [Nx*Ny*Nz*...*sizeof(type)] or [[] [$] [type] [#] [[] [Nx type] [nx] [Ny type] [Ny] [Nz type] [Nz] ... []] [Nx*Ny*Nz*...*sizeof(type)] The main reasons that had led to my prefer to the #+[ construct are 1. it does not need to add handling to a new marker in an existing parser, instead, one can just call the array parsing function to read out the dimensions when #+[ is encountered. You can see this in below examples: - for JSONLab: https://github.com/fangq/jsonlab/blob/master/loadubjson.m#L135-L138 - for py-ubjson: ***@***.*** #diff-fae7cbc0e5df9b9d3d69d5cc4453b6feR226-R229 <NeuroJSON/pybj@b90e140#diff-fae7cbc0e5df9b9d3d69d5cc4453b6feR226-R229> - for ubj: ***@***.***#diff-572570f413b5480c5985c6a5c9ede5b2R359 <fangq/ubj@cca51f6#diff-572570f413b5480c5985c6a5c9ede5b2R359> in my opinion, this requires the least amount of work for parser updates 1. it does not limit the maximum dimension to 255 as the @ construct 2. the row-major byte order aligns with the conventions for the majority of programming languages, such as C, C++, python, Javascript Typed array etc, therefore, the data do not require transpose when saving/reading. 3. it feels intuitive because for an ND array, its count is not a single number but a 1D vector. 4. This construct also align with the general template construct I am about to propose - by allowing $+{ or #+{ to represent payload less object template, similar to what I proposed for messagepack: msgpack/msgpack#267 (comment) <msgpack/msgpack#267 (comment)> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#109 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGWIMDEOLLRDPMFZZCV6XTRRKYD5ANCNFSM4M7ZM7PQ> .

[Steve132]

"For extending to template objects, I reserved $+{ or #+{ for this purpose, " ${ already has a meaning in UBJSON. (It's an array of objects). Keep in mind that an array of objects is NOT always going to be a template. An ND-array of dynamic type does not always have to be typed, and it doesn't always have to be a template. The template use case is a separate case.

…

On Wed, May 13, 2020 at 11:20 AM Steven Braeger ***@***.***> wrote: >it does not need to add handling to a new marker in an existing parser, instead, one can just call the array parsing function to read out the dimensions when #+[ is encountered. You can see this in below examples: That's actually a good argument *against* doing this. Parsing the array as an array like you've done here in code is a bug, because it actually allows people to use nd-arrays (and arrays of strings or objects or floats) as the dimension specifier. We don't *want* parsers to be encouraged to re-use the existing array parsing code, because if they do that's incorrect and leads to bugs. Really what your spec says is that you need to either implement most of the array parser twice (one for the general version, and again for the constrained version) OR you need to add a 'constrain to 1d array' flag to the code. > it does not limit the maximum dimension to 255 as the @ construct I'd say that, in practice, this limitation isn't an actual problem. I've never seen a 256-dimensional array anywhere in actual usage for any problem domain. But sure. > the row-major byte order aligns with the conventions for the majority of programming languages, such as C, C++, python, Javascript Typed array etc, therefore, the data do not require transpose when saving/reading. I actually completely agree with this. > it feels intuitive because for an ND array, its count is not a single number but a 1D vector. I understand your argument here. One concern that I have: in UBJ I'd want to relax the restriction that an ND-array must have a type. An array can have a count but not a type in UBJSON. To me it seems like it's reasonable to do an ND array of unknown things. > This construct also align with the general template construct I am about to propose - by allowing $+{ or #+{ to represent payload less object template, s $+{ already has a meaning in UBJSON. It's a fixed array where each item is an object. The only available place for extension is #{ I think we can come together to find a middle ground spect here that doesn't break backwards compatibility for you in 99% of cases but fixes some of these problems. On Wed, May 13, 2020 at 10:54 AM Qianqian Fang ***@***.***> wrote: > For the ND array construct, I am aware that @Steve132 > <https://github.com/Steve132> has a similar proposal using the below > construct > > [[] [$] [type] [@] [1-byte Ndim] [Nx type] [Nx] [Ny type] [Ny] ... [serialized ND array in column-major order] > > the ND array optimized container used in BJData was originally proposed > in this mailing list post back in 2013, it is done via #+a UBJSON 1D > integer array object, followed by row-major serialized array elements, for > example > > [[] [$] [type] [#] [[] [$] [Nx type] [#] [Ndim type] [Ndim] [Nx Ny Nz ...] [Nx*Ny*Nz*...*sizeof(type)] > or > [[] [$] [type] [#] [[] [Nx type] [nx] [Ny type] [Ny] [Nz type] [Nz] ... []] [Nx*Ny*Nz*...*sizeof(type)] > > The main reasons that had led to my prefer to the #+[ construct are > > 1. it does not need to add handling to a new marker in an existing > parser, instead, one can just call the array parsing function to read out > the dimensions when #+[ is encountered. You can see this in below > examples: > > > - for JSONLab: > https://github.com/fangq/jsonlab/blob/master/loadubjson.m#L135-L138 > - for py-ubjson: ***@***.*** > #diff-fae7cbc0e5df9b9d3d69d5cc4453b6feR226-R229 > <NeuroJSON/pybj@b90e140#diff-fae7cbc0e5df9b9d3d69d5cc4453b6feR226-R229> > - for ubj: ***@***.***#diff-572570f413b5480c5985c6a5c9ede5b2R359 > <fangq/ubj@cca51f6#diff-572570f413b5480c5985c6a5c9ede5b2R359> > > in my opinion, this requires the least amount of work for parser updates > > 1. > > it does not limit the maximum dimension to 255 as the @ construct > 2. > > the row-major byte order aligns with the conventions for the majority > of programming languages, such as C, C++, python, Javascript Typed array > etc, therefore, the data do not require transpose when saving/reading. > 3. > > it feels intuitive because for an ND array, its count is not a single > number but a 1D vector. > 4. > > This construct also align with the general template construct I am > about to propose - by allowing $+{ or #+{ to represent payload less > object template, similar to what I proposed for messagepack: msgpack/msgpack#267 > (comment) > <msgpack/msgpack#267 (comment)> > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#109 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAGWIMDEOLLRDPMFZZCV6XTRRKYD5ANCNFSM4M7ZM7PQ> > . >

[Steve132]

I think I can draw up a proposed ND-array spec for this that doesn't break backcompatibility with you 99% of the time but fixes these issues. Give me a day or so.

…

On Wed, May 13, 2020 at 11:23 AM Steven Braeger ***@***.***> wrote: "For extending to template objects, I reserved $+{ or #+{ for this purpose, " ${ already has a meaning in UBJSON. (It's an array of objects). Keep in mind that an array of objects is NOT always going to be a template. An ND-array of dynamic type does not always have to be typed, and it doesn't always have to be a template. The template use case is a separate case. On Wed, May 13, 2020 at 11:20 AM Steven Braeger ***@***.***> wrote: > >it does not need to add handling to a new marker in an existing parser, > instead, one can just call the array parsing function to read out the > dimensions when #+[ is encountered. You can see this in below examples: > > That's actually a good argument *against* doing this. Parsing the array > as an array like you've done here in code is a bug, because it actually > allows people to use nd-arrays (and arrays of strings or objects or floats) > as the dimension specifier. We don't *want* parsers to be encouraged to > re-use the existing array parsing code, because if they do that's incorrect > and leads to bugs. Really what your spec says is that you need to either > implement most of the array parser twice (one for the general version, and > again for the constrained version) OR you need to add a 'constrain to 1d > array' flag to the code. > > > it does not limit the maximum dimension to 255 as the @ construct > > I'd say that, in practice, this limitation isn't an actual problem. I've > never seen a 256-dimensional array anywhere in actual usage for any problem > domain. But sure. > > > the row-major byte order aligns with the conventions for the majority > of programming languages, such as C, C++, python, Javascript Typed array > etc, therefore, the data do not require transpose when saving/reading. > > I actually completely agree with this. > > > it feels intuitive because for an ND array, its count is not a single > number but a 1D vector. > > I understand your argument here. > > One concern that I have: in UBJ I'd want to relax the restriction that an > ND-array must have a type. An array can have a count but not a type in > UBJSON. To me it seems like it's reasonable to do an ND array of unknown > things. > > > This construct also align with the general template construct I am > about to propose - by allowing $+{ or #+{ to represent payload less > object template, s > > $+{ already has a meaning in UBJSON. It's a fixed array where each item > is an object. The only available place for extension is #{ > > I think we can come together to find a middle ground spect here that > doesn't break backwards compatibility for you in 99% of cases but fixes > some of these problems. > > > > > On Wed, May 13, 2020 at 10:54 AM Qianqian Fang ***@***.***> > wrote: > >> For the ND array construct, I am aware that @Steve132 >> <https://github.com/Steve132> has a similar proposal using the below >> construct >> >> [[] [$] [type] [@] [1-byte Ndim] [Nx type] [Nx] [Ny type] [Ny] ... [serialized ND array in column-major order] >> >> the ND array optimized container used in BJData was originally proposed >> in this mailing list post back in 2013, it is done via #+a UBJSON 1D >> integer array object, followed by row-major serialized array elements, for >> example >> >> [[] [$] [type] [#] [[] [$] [Nx type] [#] [Ndim type] [Ndim] [Nx Ny Nz ...] [Nx*Ny*Nz*...*sizeof(type)] >> or >> [[] [$] [type] [#] [[] [Nx type] [nx] [Ny type] [Ny] [Nz type] [Nz] ... []] [Nx*Ny*Nz*...*sizeof(type)] >> >> The main reasons that had led to my prefer to the #+[ construct are >> >> 1. it does not need to add handling to a new marker in an existing >> parser, instead, one can just call the array parsing function to read out >> the dimensions when #+[ is encountered. You can see this in below >> examples: >> >> >> - for JSONLab: >> https://github.com/fangq/jsonlab/blob/master/loadubjson.m#L135-L138 >> - for py-ubjson: ***@***.*** >> #diff-fae7cbc0e5df9b9d3d69d5cc4453b6feR226-R229 >> <NeuroJSON/pybj@b90e140#diff-fae7cbc0e5df9b9d3d69d5cc4453b6feR226-R229> >> - for ubj: ***@***.*** >> #diff-572570f413b5480c5985c6a5c9ede5b2R359 >> <fangq/ubj@cca51f6#diff-572570f413b5480c5985c6a5c9ede5b2R359> >> >> in my opinion, this requires the least amount of work for parser updates >> >> 1. >> >> it does not limit the maximum dimension to 255 as the @ construct >> 2. >> >> the row-major byte order aligns with the conventions for the >> majority of programming languages, such as C, C++, python, Javascript Typed >> array etc, therefore, the data do not require transpose when saving/reading. >> 3. >> >> it feels intuitive because for an ND array, its count is not a >> single number but a 1D vector. >> 4. >> >> This construct also align with the general template construct I am >> about to propose - by allowing $+{ or #+{ to represent payload less >> object template, similar to what I proposed for messagepack: msgpack/msgpack#267 >> (comment) >> <msgpack/msgpack#267 (comment)> >> >> — >> You are receiving this because you were mentioned. >> Reply to this email directly, view it on GitHub >> <#109 (comment)>, >> or unsubscribe >> <https://github.com/notifications/unsubscribe-auth/AAGWIMDEOLLRDPMFZZCV6XTRRKYD5ANCNFSM4M7ZM7PQ> >> . >> >

[edgar-bonet]

For the record, I implemented my own proposal for N-dimensional arrays that does not require extra syntax. It just adds the notion that, in a strongly-typed array, all the bytes that come before the first data field form the array's type specifier. Type specifiers can then be parsed recursively in a very natural way:

• [[][$][d][#][i][2] = array of type float32 and length 2
• [[][$] [[][$][d][#][i][2] [#][I][3200] = array of type (array of type float32 and length 2) and length 3200

This is efficient for the kind of data I deal with, which contains lots of very long and narrow matrices of float32, just like in this example. Binary JData would be about as efficient for me, at the cost the extra complexity that comes with adding new syntax. The current UBJSON specification does not permit storing this efficiently.

[Steve132]

@edgar-bonet We sort of went over this at the time, but can you clarify *semantically* what makes your proposal different than either 2 of what we've already sort of proposed here? It seems like your proposal the way you are using it provides the same *semantics* as the other two proposals, but it's much more complicated to parse. (you have to lookahead to complete the entire array template type recursively and then pop up off the stack to get the resulting N-D dimensions) It also (worryingly, to me at least) seems to imply that it's a more generic 'schema' that would allow people to insert templated arrays of arrays into the stack structure, but that's not the case. For example, [ [ ] [$] [ [ ][#][[][i][2] [#][3200] = array of type (array of type (dynamic array) with length 2) with length 3200

…

On Wed, May 13, 2020 at 2:36 PM Edgar Bonet ***@***.***> wrote: For the record, I implemented my own proposal for N-dimensional arrays <http://../issues/43> that does not require extra syntax. It just adds the notion that, in a strongly-typed array, all the bytes that come before the first data field form the array's type specifier. Type specifiers can then be parsed recursively in a very natural way: - [[][$][d][#][i][2] = array of type float32 and length 2 - [[][$] [[][$][d][#][i][2] [#][I][3200] = array of type (array of type float32 and length 2) and length 3200 This is efficient for the kind of data I deal with, which contains lots of very long and narrow matrices of float32, just like in this example. Binary JData would be about as efficient for me, at the cost the extra complexity that comes with adding new syntax. The current UBJSON specification does not permit storing this efficiently. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#109 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGWIME6WAYI4AET6E5I333RRLSCBANCNFSM4M7ZM7PQ> .

[fangq]

We don't want parsers to be encouraged to re-use the existing array parsing code, because if they do that's incorrect and leads to bugs.

not entirely sure what was the main concern against using recursive calls. In a way, we are already doing it in almost all existing parsers because JSON/UBJSON array and object constructs can be embedded in one another. If you are worried about the recursion depth and stack-overflow risks, then I think the same concerns should be applied to regular array/object parsing, and not specific to this size construct.

the only thing to avoid stack-overflow is to change the entire parser structure using an iterator or some sort to convert recursion into loops.

Really what your spec says is that you need to either implement most of the array parser twice (one for the general version, and again for the constrained version) OR you need to add a 'constrain to 1d array' flag to the code.

if I just want to prevent it from being abused by constructing deep-recursion, adding a simple flag, like what I just added to jsonlab will likely eliminate the risk, and the required change is quite minimal and local

NeuroJSON/jsonlab@86efe89

[edgar-bonet]

@Steve132 wrote:

We sort of went over this at the time

Indeed. I am commenting mostly for @fangq, who was not there at the time.

It seems like your proposal the way you are using it provides the same semantics as the other two proposals

Of course. This is all about providing a representation of the most common forms of multidimensional arrays that is more efficient without altering their semantics.

it's much more complicated to parse

I disagree. Recursion makes it easy. Recursion is what makes the JSON data model both simple an powerful: containers hold values which can themselves be containers which hold values which...

you have to lookahead [...]

Yes, but never more than one byte:

1. When the type parser sees [ or {, it reads the following byte in order to see whether it is either $ or #, which would indicate an optimized representation of the container. If it is neither (non optimized container), it “unreads” that byte (with the equivalent of ungetc()) before calling the value parser for reading the first contained element.

2. When reading an non optimized container, the value parser has to read one byte past the current element in order to check whether it is the container's closing delimiter. If it is not, it unreads the byte before calling itself recursively for reading the next element.

But note that this has nothing to do with my proposal, which does not introduce any extra lookahead requirements.

For example [ [ ] [$] [ [ ][#][[][i][2] [#][3200]

I can't visually parse your example. [#][[][i][2] seems to imply that the count field is an array, which is legit in fangq's proposal but not in mine.

[Steve132]

I don't have a problem with recursion in general. Recursion in general is great, especially for a parser. I have two separate arguments about two separate proposals: The first argument is about this claim of your proposal "The implementer can re-use the same parser". My claim is that they cannot. [[] [$] [{] [#] [U] [3] [{][}][{][}][{][}] This is a valid 1-D array specifier. (in particular, it's a 1-D array of 3 empty objects} It is NOT a valid N-D array dimension header according to your spec (the type is object not int). Therefore you *can't* simply use the standard array parser unmodified to parse the N-D array header. You have to have two array parsers. One of them that works generically and the other which is constrained to the N-D array header spec (e.g, only integers, only one dimensional, only TCV). But an implementer who re-uses the 1-D array parser code unmodified to parse your N-D array header would produce a broken implementation, because it would accept the above as a valid ND array header. (in fact, your patch to ubjr actually has this bug). Fixing the bug requires implementing a special case version of the array parser that *only* reads a valid N-D array header and not any array. So it's avoidable. But there's no code reuse here. Reusing the code actually creates a bug. So since you can't reuse code, code reuse isn't a feature of your design. What's more, it encourages implementers to make the bug that you made (because it superficially looks like it works). The second argument is about @edgar-bonet's implementation, which basically boils down to. "What's easier to parse and understand: 'floats, 3 dimensions: 4 rows 10 columns 3 channels' or '( ( ( ( float ) 3 ) 10 ) 4'" They both work, but to me the first one is a lot easier to parse and validate.

…

On Wed, May 13, 2020 at 3:25 PM Qianqian Fang ***@***.***> wrote: We don't *want* parsers to be encouraged to re-use the existing array parsing code, because if they do that's incorrect and leads to bugs. not entirely sure what was the main concern against using recursive calls. In a way, we are already doing it in almost all existing parsers because JSON/UBJSON array and object constructs can be embedded in one another. If you are worried about the recursion depth and stack-overflow risks, then I think the same concerns should be applied to regular array/object parsing, and not specific to this size construct. the only thing to avoid stack-overflow is to change the entire parser structure using an iterator or some sort to convert recursion into loops. Really what your spec says is that you need to either implement most of the array parser twice (one for the general version, and again for the constrained version) OR you need to add a 'constrain to 1d array' flag to the code. if I just want to prevent it from being abused by constructing deep-recursion, adding a simple flag, like what I just added to jsonlab will likely eliminate the risk, and the required change is quite minimal and local ***@***.*** <NeuroJSON/jsonlab@86efe89> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#109 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGWIMDUVLNQBUD7NI5HVALRRLX37ANCNFSM4M7ZM7PQ> .

[Steve132]

@edgar-bonet If I want to read my ND array *as* an ND array, under your proposal, I have to parse the entire header recursively all the way down to get the type, then all the way back up and populate my array in reverse order in order to get all the dimensions. ( ( ( ( float ) 3 ) 10 ) 4 ) vs "float, 3 dimensions, 4,10,3"

…

On Wed, May 13, 2020 at 5:02 PM Steven Braeger ***@***.***> wrote: I don't have a problem with recursion in general. Recursion in general is great, especially for a parser. I have two separate arguments about two separate proposals: The first argument is about this claim of your proposal "The implementer can re-use the same parser". My claim is that they cannot. [[] [$] [{] [#] [U] [3] [{][}][{][}][{][}] This is a valid 1-D array specifier. (in particular, it's a 1-D array of 3 empty objects} It is NOT a valid N-D array dimension header according to your spec (the type is float not int). Therefore you *can't* simply use the standard array parser unmodified to parse the N-D array header. You have to have two array parsers. One of them that works generically and the other which is constrained to the N-D array header spec (e.g, only integers, only one dimensional, only TCV). But an implementer who re-uses the 1-D array parser code unmodified to parse your N-D array header would produce a broken implementation, because it would accept the above as a valid ND array header. (in fact, your patch to ubjr actually has this bug). Fixing the bug requires implementing a special case version of the array parser that *only* reads a valid N-D array header and not any array. So it's avoidable. But there's no code reuse here. Reusing the code actually creates a bug. So since you can't reuse code, code reuse isn't a feature of your design. The second argument is about @edgar-bonet's implementation, which basically boils down to. "What's easier to parse and understand: 'floats, 3 dimensions: 4 rows 10 columns 3 channels' or '( ( ( ( float ) 76 ) 10 ) 4'" They both work, but to me the first one is a lot easier to parse and validate. On Wed, May 13, 2020 at 3:25 PM Qianqian Fang ***@***.***> wrote: > We don't *want* parsers to be encouraged to re-use the existing array > parsing code, because if they do that's incorrect and leads to bugs. > > not entirely sure what was the main concern against using recursive > calls. In a way, we are already doing it in almost all existing parsers > because JSON/UBJSON array and object constructs can be embedded in one > another. If you are worried about the recursion depth and stack-overflow > risks, then I think the same concerns should be applied to regular > array/object parsing, and not specific to this size construct. > > the only thing to avoid stack-overflow is to change the entire parser > structure using an iterator or some sort to convert recursion into loops. > > Really what your spec says is that you need to either implement most of > the array parser twice (one for the general version, and again for the > constrained version) OR you need to add a 'constrain to 1d array' flag to > the code. > > if I just want to prevent it from being abused by constructing > deep-recursion, adding a simple flag, like what I just added to jsonlab > will likely eliminate the risk, and the required change is quite minimal > and local > > ***@***.*** > <NeuroJSON/jsonlab@86efe89> > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#109 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAGWIMDUVLNQBUD7NI5HVALRRLX37ANCNFSM4M7ZM7PQ> > . >

[Steve132]

To be clear, I don't necessarily think the bug caused by people wanting to re-use the array code is so severe as to veto using the JData standard for the syntax. As long as we warn implementers in the spec not to do it it's easy enough to avoid. But the JData standard does use more characters as a result as well...so it's worth considering.

[edgar-bonet]

@Steve132 wrote:

[[] [$] [{] [#] [U] [3] [{][}][{][}][{][}] is a valid 1-D array of 3 empty objects

My understanding is that, under the current spec, it is not. Either the array is untyped and the values have to be spelled in full:

[[] [#][U][3] [{][}][{][}][{][}]

or the array is typed and the type (a single byte) has to be removed from the values:

[[] [$][{] [#][U][3] [}][}][}]

What's easier to parse and understand [...]

I personally find “((float 3) 10) 4” easier, but that may be just me. I can understand that humans may find “floats, 3 dimensions: 4 rows 10 columns 3 channels” easier. :-)