Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HasNullDefault in TypeScript returns null for 0 scalar value input #7395

Closed
TJKoury opened this issue Jul 28, 2022 · 12 comments
Closed

HasNullDefault in TypeScript returns null for 0 scalar value input #7395

TJKoury opened this issue Jul 28, 2022 · 12 comments
Labels

Comments

@TJKoury
Copy link
Contributor

TJKoury commented Jul 28, 2022

Flatbuffers 2.0.5
Compiled to WASM (link)
Chrome Version 1.38.111

The offset check here will return null for offset 0 if the HasNullDefault flag is set for a property.

Setting a 0 value for a scalar results in offset being set to 0, which means a zero can't be stored in an property that can be null.

@bjornharrtell
Copy link
Collaborator

Seem like a bug to me. I have vague memory of it being noted in the past but as of yet I've been unable to find anything.

I suppose changing this behaviour could potentially be seen as a breaking change, but I think it's likely no one is dependent on that so I think we should just try to find a sensible fix as a patch release.

@bjornharrtell
Copy link
Collaborator

It would be good/interesting to find out if this was also the behavior pre TS port (i.e Flatbuffers 1.x).

@TJKoury
Copy link
Contributor Author

TJKoury commented Aug 4, 2022

I have been looking at this since I posted the issue, hard to think of a portable solution that won’t effect parsing flatbuffers in other languages. The main issue is there doesn’t seem to be a null datatype in the flatbuffer spec. Trying to shoehorn one in at the language level is not the right approach, especially when the null field descriptor is able to be applied to any datatype. There should be a “lack of value” datatype available as a type.

@bjornharrtell
Copy link
Collaborator

Hmm yes there is more to this than I thought. But there does seem to be something previous to read up.. see #6014 and #6215.

@TJKoury
Copy link
Contributor Author

TJKoury commented Aug 4, 2022

I should have read those more closely and then linked them to this issue, thanks. Very strange this was implemented without a null primitive, and seems to be incompletely supported cross-language. I don’t think any implementation will be accepted without engaging the wider community.

@bjornharrtell
Copy link
Collaborator

bjornharrtell commented Aug 4, 2022

@TJKoury not sure I agree, it's clear that the intent is that if you describe a scalar type as optional (with = null in schema) then it is taking up no wire space (which I believe equals offset zero) and should be interpreted as null. A 0 should in this case be stored and as such not result in a zero offset. If my interpretation is correct and that the current implementation returns null for 0 when default is null it's a clear bug.

@TJKoury
Copy link
Contributor Author

TJKoury commented Aug 4, 2022

The issue in this case is that it appears that in all languages an offset of zero is equivalent to zero scalar, to save wire space as you say. To change the interpretation of zero offset to mean “null” in the case of an optional null field would encode that meaning into the flatbuffer without a way to interpret it cross-language. I suppose that’s true now with simply failing to compile in unsupported languages. We could store the default offset for the scalar with a value of 0, and the gain in wire space is the trade-off for using it.

@bjornharrtell
Copy link
Collaborator

@TJKoury but the change to make that interpretation has already been done in most implementations? I don't see how it affect the encoding, it's simply a interpretation (schema generated code) change and I don't understand your claim that it's "without a way to interpret cross-lang".

@TJKoury
Copy link
Contributor Author

TJKoury commented Aug 4, 2022

It seems most implementations regard a zero offset as the value zero, right?

@TJKoury
Copy link
Contributor Author

TJKoury commented Aug 25, 2022

Picking this back up, I'm going to remove using null at all since:

  • It's not supported fully cross-language
  • It is not supported at all in strings or a few other types
  • It is potentially bad abstraction to give fields that can be different datatypes the same potential value that is not technically part of the set of possible values per datatype

@github-actions
Copy link

github-actions bot commented Mar 4, 2023

This issue is stale because it has been open 6 months with no activity. Please comment or label not-stale, or this will be closed in 14 days.

@github-actions
Copy link

This issue was automatically closed due to no activity for 6 months plus the 14 day notice period.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants