pack: format JSON floats using shortest round-trip representation#12129
pack: format JSON floats using shortest round-trip representation#12129vitaly-sinev wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a public helper for shortest round-tripping double formatting, applies it to float32 and float64 JSON serialization, preserves NaN-to-null conversion, and adds tests for formatting, fallback, truncation, invalid arguments, and JSON output. ChangesFloating-point serialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/internal/pack.c`:
- Around line 989-1035: Add failure-path assertions to test_pack_double_to_str
for both a NULL output buffer and a zero buf_size, verifying each call returns
-1 without dereferencing or writing the buffer. Keep the existing valid-buffer
and undersized nonzero-buffer coverage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5cfd107-cc5e-4358-8554-c1aa61ce49d9
📒 Files selected for processing (3)
include/fluent-bit/flb_pack.hsrc/flb_pack.ctests/internal/pack.c
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d49ce3ea7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| int flb_pack_double_to_str(double val, char *out_buf, size_t buf_size) | ||
| { | ||
| yyjson_val num; |
There was a problem hiding this comment.
When Fluent Bit is configured with -DFLB_YYJSON=Off, CMake does not add the yyjson include directory or define FLB_HAVE_YYJSON, but this helper still declares yyjson_val and calls yyjson APIs unconditionally. In that supported build configuration, src/flb_pack.c now fails to compile with unknown type name 'yyjson_val'; please guard this path or provide the legacy snprintf fallback when yyjson is disabled.
Useful? React with 👍 / 👎.
0d49ce3 to
d04dd0a
Compare
|
Thanks for the automated reviews — addressed all three findings and force-pushed:
|
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d04dd0a404
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * Non-finite (nan/inf), unexpected yyjson failure, or a build without the | ||
| * yyjson backend: fall back to legacy formatting. | ||
| */ | ||
| len = snprintf(out_buf, buf_size, "%.17g", val); |
There was a problem hiding this comment.
In builds configured with -DFLB_YYJSON=Off, all finite doubles now reach this fallback. That is not the legacy path removed above: integer-valued floats like 5.0 and -0.0 serialize as 5 and -0, and the #6447 case still serializes with 17-digit noise instead of the advertised shortest token. Please preserve the old integer/float formatting or provide a real dtoa fallback when yyjson is unavailable.
Useful? React with 👍 / 👎.
d04dd0a to
5a99f51
Compare
|
Addressed the P2 (yyjson-disabled formatting) and force-pushed. The fallback now preserves the trailing Added a |
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/flb_pack.c (1)
988-1030: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the required C layout to the changed helper.
The changed
if/elseblocks use same-line opening braces, andtmp,end, andnumare declared inside a conditional block rather than at function start.As per coding guidelines, “Put function opening braces on the next line” and “Declare variables at the start of functions rather than mid-block.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/flb_pack.c` around lines 988 - 1030, Update the changed helper’s formatting to follow the required C layout: place the function’s opening brace on the next line and move the tmp, end, and num declarations to the function start, preserving their existing types and initialization. Reformat the affected if/else blocks so opening braces are on the next line without changing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/flb_pack.c`:
- Around line 1084-1087: Update the double-formatting failure branch in the
map-value serialization path around flb_pack_double_to_str and try_to_write:
when the helper returns a negative result, propagate failure through
msg2json_end instead of resetting i to zero. Preserve normal writing for
successful conversions and ensure no empty JSON value is emitted.
---
Nitpick comments:
In `@src/flb_pack.c`:
- Around line 988-1030: Update the changed helper’s formatting to follow the
required C layout: place the function’s opening brace on the next line and move
the tmp, end, and num declarations to the function start, preserving their
existing types and initialization. Reformat the affected if/else blocks so
opening braces are on the next line without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2907bdca-c479-4b9b-859d-8d768ba77e5b
📒 Files selected for processing (3)
include/fluent-bit/flb_pack.hsrc/flb_pack.ctests/internal/pack.c
🚧 Files skipped from review as they are similar to previous changes (2)
- include/fluent-bit/flb_pack.h
- tests/internal/pack.c
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a99f511d2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| else { | ||
| i = snprintf(temp, sizeof(temp)-1, "%.16g", o->via.f64); | ||
| i = flb_pack_double_to_str(o->via.f64, temp, sizeof(temp)); |
There was a problem hiding this comment.
When flb_msgpack_to_json() is used by callers with fixed buffers, this new formatter can expand each 9-byte MessagePack double such as 1e20 into 100000000000000000000.0 (23 bytes) instead of the old compact exponent form. I checked existing direct callers: the S3 and Chronicle log_key extraction paths allocate only about the input MessagePack size (bytes + bytes / 4) and only treat ret < 0 as failure, so an array/object log key containing several such floats can make flb_msgpack_to_json() return 0 for insufficient space and then be emitted as an empty/truncated value. Please either keep exponent notation for these magnitudes or update the fixed-buffer callers to grow/retry and handle ret == 0 before switching this core path.
Useful? React with 👍 / 👎.
Serialize double values with the shortest decimal string that parses back to the same value using yyjson's dtoa, so e.g. 0.072 is emitted as "0.072" instead of "0.07199999999999999" while preserving round-trip precision. Add flb_pack_double_to_str() and use it in flb_msgpack_to_json(). The integer-valued special case is dropped because the helper already renders whole numbers with a trailing ".0". Non-finite values (nan, inf) keep the previous formatting. Fixes fluent#6447 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: v.sinev <vit.phantom@gmail.com>
Verify that doubles are serialized to JSON using their shortest representation, e.g. 0.072 is emitted as "0.072" and not "0.07199999999999999", covering the regression from fluent#6447. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: v.sinev <vit.phantom@gmail.com>
5a99f51 to
4f33276
Compare
|
Good catch — on a formatting failure the FLOAT case now |
|
Thanks — I looked into this. A few points:
The clean fix is at the API boundary rather than in this float PR: make |
|
Opened #12133 for the truncation-signalling fix discussed above ( |
|
Gentle ping 🙂 — this is the primary fix for #6447 (JSON floats like Would a maintainer be able to take a look / set |
1 participant
Fixes #6447 (the issue was auto-closed by the stale bot, but the bug is still present on
master).Problem
When serializing floats to JSON (
Format json/flb_msgpack_to_json), values are formatted with%.16g, so0.072is emitted as0.07199999999999999. The value0.072is not exactly representable as an IEEE-754 double, and 16 significant digits expose the trailing noise. Beyond being ugly,%.16gdoes not guarantee round-trip: a large fraction of doubles reparse to a different value.Fix
Add
flb_pack_double_to_str()inflb_pack.c, which uses the already-vendored yyjson dtoa (yyjson_write_number) to produce the shortest decimal string that round-trips back to the same double. Non-finite values (nan/inf) fall back to the previous%.17gbehavior. It is applied to the coreflb_msgpack_to_jsonfloat path here; the same helper will be routed through the other double→string sites (flb_json,flb_typecast, OpenTelemetry metrics,out_loki,processor_content_modifier,router_config) in separate follow-up PRs to keep this one focused and easy to review/revert.This is also faster than the old
snprintf("%.16g")on the hot path and removes a latent(long long)doublecast (UB for out-of-range doubles).Behavior notes
5.0),-0.0,nan/infare unchanged (yyjson emits a trailing.0for integer-valued doubles, matching the old%.1fspecial case).1e-05→0.00001,1e+20→100000000000000000000.0); all outputs remain valid JSON numbers.Testing
tests/internal/pack.c:json_pack_float(end-to-end throughflb_msgpack_to_json) andpack_double_to_str(direct helper coverage — round-trip over normal/subnormal/exponent values,nan/inffallback,-0.0, undersized-buffer boundary)../bin/flb-it-packpasses.out_stdoutwithFormat jsonon{"test":0.072}now prints[{"test":0.072}].scenarios/out_stdout(14 tests) passes, both normally and undervalgrind --leak-check=full(--valgrind --valgrind-strict) — no leaks or memory errors.Documentation
fluent/fluent-bit-docsdescribing the JSON float format change (before/after) will follow.Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
🤖 Generated with Claude Code
Summary by CodeRabbit
doublevalues into shortest round-tripping string form with safe buffer/error handling.-0.0and consistent shortest-string output; non-finite values use legacy formatting and existing NaN-to-null behavior is preserved.