docs: improve fractional non-string adr - #1963
Conversation
dosubot
Bot
added
the
size:M
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the architectural decision for the fractional operator to improve cross-language consistency. Key changes include explicitly rejecting null as a primary argument, implementing strict number normalization (Integer vs. Float), and requiring CBOR deterministic encoding for map ordering and preferred serialization. Feedback suggests restoring null to the list of supported CBOR major types to handle nested values correctly and recommends explicitly rejecting datetime types with an error instead of leaving their behavior undefined.
|
@cupofcat @toddbaert @beeme1mr Could you take a look and let me know what do you think of those changes? |
|
Ping @toddbaert what are your thoughts on this? |
| To prevent overflow errors in strongly-typed languages (e.g. Go) and inconsistent BigInt tagging in languages with arbitrary-precision integers (e.g. Python), the number normalization is restricted to the range $[-2^{63}, 2^{64}-1]$ (covering both signed and unsigned 64-bit integers): | ||
|
|
||
| 1. If a numeric value has no fractional part (e.g., `val == math.Trunc(val)` in Go, or `val.is_integer()` in Python), the provider must attempt to cast it to a signed (if <0) or unsigned (if >=0) integer before encoding. | ||
| 2. If a numeric value has fractional part, or if it falls outside the range $[-2^{63}, 2^{64}-1]$ (e.g., `1.0e+176`), it **must not** be normalized to an integer. It must be encoded as a float (Major Type 7). |
There was a problem hiding this comment.
We may want to apply this to anything larger than the Javascript MAX_SAFE_INTEGER (2^53 - 1). Things could get messy, especially in JS, otherwise.
Even Gson I think out of the box, would yield double here, IIUC.
If the goal here is just consistency across all languages including JS, I think encoding as float outside the "SAFE" JS range will be easiest to accomplish. We could also perhaps support parsing to bigint in JS, but that seems somewhat heavy for just this purpose.
There was a problem hiding this comment.
My idea behind this proposal was to make this adr future proof. As for now, we have problems with int64 in the whole flagd (JavaScript is only one of the problem, I think Go also uses double for all numbers when parsing flag config, and Java is not supporting even >Int32 values), and from my understanding we aren't officially supporting it anywhere. I have seen some discussions on creating and formalizing that support and thought that we could handle it from beginning here (In JavaScript, we can use BigInt for comparison regardless of number type passed to the fractional evaluator). Thanks to that, when we finally introduce support for full int64 range, we won't have to create breaking changes when trying to support int64 in fractional operator (since it would require again adr update and it would break encodings for numbers from 2^53 to 2^64).
I agree that supporting int64 is definitely out of the scope for this task, but as this range changes only the check in number normalization, and won't modify behavior of parsing, evaluation etc, and we can treat those two tasks as completely separate.
But it's definitely not a dealbreaker for me, so I can modify that range to handle only float-safe values.
There was a problem hiding this comment.
My idea behind this proposal was to make this adr future proof.
I agree that supporting int64 is definitely out of the scope for this task, but as this range changes only the check in number normalization, and won't modify behavior of parsing, evaluation etc, and we can treat those two tasks as completely separate.
That makes sense to me - to be clear - you are proposing interpreting any number passed here in the Javascript implementation as a BigInt in terms of generating the bucketing value? If so, I have no other comments, and I will approve.
There was a problem hiding this comment.
We don't need to interpret that number. We can just compare it with BigInt which from my research have all necessary implementation to handle that comparison safely and reliably with any number type. As we only need to check if the value fits into the range, the comparison operation is the only thing necessary for implementation of this adr.
| 4. Otherwise, if `targetingKey` is missing, report an error and return nil | ||
| 1. If the first element in `fractional` evaluates to `null`, we report an error and return `nil`. | ||
| 2. If the first element in `fractional` evaluates to a non-array type then deterministically encode it to a well defined byte array and hash the bytes. | ||
| 3. Otherwise, if `targetingKey` is a string, build a 2-elements array of `flagKey` and `targetingKey`, deterministically encode that and hash (**NOTE:** This is different than string concatenation used today). |
There was a problem hiding this comment.
Is it possible to maintain the current cat behavior?
If not, that's okay but we did get some complains about the re-random assignment this caused in the past. Not a huge deal <1.0.
There was a problem hiding this comment.
This behavior is copied from original ADR, I just modified the numbering, due to adding additional check at the beginning. I think we can leave the current cat behavior, but it still will introduce rebucketing due to the fact, that cbor encoding that we use for fractional before passing to murmur hash, stores the string type info and its length. So even if we leave current behavior as is, the rebucket will occur.
The only difference would be between evaluation of explicit cat in the code, and leveraging this syntactic sugar:
"fractional-flag-shorthand": {
"state": "ENABLED",
"variants": {
"heads": "heads",
"tails": "tails",
"draw": "draw"
},
"defaultVariant": "draw",
"targeting": {
"fractional": [
["heads", 1],
["tails", 1]
]
}
},
"fractional-flag-explicit-cat": {
"state": "ENABLED",
"variants": {
"heads": "heads",
"tails": "tails",
"draw": "draw"
},
"defaultVariant": "draw",
"targeting": {
"fractional": [
{"cat": [{"var": "$flag.flagKey"}, {"var": "targetingKey"}]},
["heads", 1],
["tails", 1]
]
}
},
In current proposal, they would yield different bucketings, while staying with cat, would cause them to be identical.
There was a problem hiding this comment.
Left a couple comments... this one sticks out to me: https://github.com/open-feature/flagd/pull/1963/changes#r3305815236.
I think as long as we have a solution for JS this all sounds good.
Signed-off-by: Marcin Olko <molko@google.com>
62a3e25 to
43d4d13
Compare
|
4 participants
This PR
While working on the non-string fractional ADR, we identified several critical pitfalls in the initial proposal and wanted to expand the requirements to ensure strict cross-language consistency.
This PR updates the ADR (fractional-non-string-rand-units.md) with the following key changes:
nullRejection: Explicitly rejectnullas the first argument to prevent silent errors from evaluation failures.Related Issues
Under scope of #1737
Notes
No Breaking Change: As none of the languages implemented this ADR yet, those changes won't affect any of the implementations, while avoiding pitfalls that could arise later on. The changes are purely semantic and do not affect the flagd JSON schema and the rest of the adr.
Follow-up Tasks
We should update reference specification of fractional operator on flagd page