Conversation
Signed-off-by: Marcin Olko <molko@google.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances flagd's evaluation capabilities by introducing a suite of new JSONLogic operators. These operators, including Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces new JsonLogic operations (starts_with, ends_with, sem_ver, fractional) to the flagd evaluator, along with their implementation, a SemanticVersion class, and integration of MurmurHash3. Review feedback highlights several issues: the SemVer parsing needs to enforce leading zero rules for version components and pre-release identifiers as per SemVer 2.0.0, the Fractional operation requires validation for non-negative weights to prevent underflow, and its sum_of_weights limit should be adjusted for broader compatibility. Additionally, it's recommended to use Abseil's StartsWith and EndsWith for idiomatic code and to refine an unreachable return statement in the Fractional operation.
Signed-off-by: Marcin Olko <molko@google.com>
Signed-off-by: Marcin Olko <molko@google.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new JSON logic operations (starts_with, ends_with, sem_ver, fractional) to the flagd evaluator, implemented within a new flagd_ops library. These operations include semantic version comparison and fractional bucketing using MurmurHash3. Feedback suggests refactoring duplicated SemVer parsing logic, using int32_t for weight in fractional evaluation for consistency, and adding a newline to the fractional ops test file.
Signed-off-by: Marcin Olko <molko@google.com>
Signed-off-by: Marcin Olko <molko@google.com>
tangenti
left a comment
There was a problem hiding this comment.
IIUC the hash code is copied from https://github.com/aappleby/smhasher/blob/master/src/MurmurHash3.cpp
Is this best way we can do to import it?
|
This library is public domain and doesn't have any library with it's implementation in bazel registry nor any other known to me c++ resources. If we would like to download it from the source, we would also download multiple different hashing functions and whole
|
Thanks. Could you document this in the directory and also in the PR description? |
Signed-off-by: Marcin Olko <molko@google.com>
Done |
This PR
Implements all flagd-specific JSONLogic operators:
starts_with,ends_with,semverandfractional.The
semveroperator supports out of the box thevandVprefixes, partial versions and correctly returns on parse errors according to open-feature/flagd#1873The
fractionaloperator supports out of the box the high precision bucketing according to open-feature/flagd#1800The PR also introduces testcases copied from flagd-testbed, as we don't have Gherkin suite introduced here, but written in C++ to ensure that the hashing is consistent.
Third-party Code: MurmurHash3
The MurmurHash3 implementation in providers/flagd/src/evaluator/murmur_hash/ has been copied from the aappleby/smhasher (https://github.com/aappleby/smhasher/blob/master/src/MurmurHash3.cpp) repository.
License & Attribution:
Related Issues
Fixes #19
How to test