Fix field names ending in _str/_int/_float being dropped in JSON output#422
Closed
strawgate wants to merge 7 commits into
Closed
Fix field names ending in _str/_int/_float being dropped in JSON output#422strawgate wants to merge 7 commits into
strawgate wants to merge 7 commits into
Conversation
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR universally replaces underscore-type suffixes ( Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
README.md (1)
89-97:⚠️ Potential issue | 🟠 MajorDocumentation inconsistency: SQL examples still use underscore suffixes.
Line 89 states fields are suffixed with
$str,$int,$float, but the SQL examples at lines 93-96 (and throughout the file at lines 33, 57, 72) still reference underscore-suffixed columns likelevel_str,status_int,duration_ms_int.These examples need updating to match the new convention.
Proposed fix
-- Filter by level -SELECT * FROM logs WHERE level_str = 'ERROR' +SELECT * FROM logs WHERE level$str = 'ERROR' -- Extract integer fields -SELECT status_int, duration_ms_int FROM logs +SELECT status$int, duration_ms$int FROM logsAlso update lines 33, 57, and 72 similarly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 89 - 97, The README's SQL examples use underscore-suffixed column names (e.g., level_str, status_int, duration_ms_int) but the doc states JSON fields are suffixed with $str/$int/$float; update every SQL example that references underscore suffixes (including the occurrences currently at lines showing level_str, status_int, duration_ms_int and the other examples at lines 33, 57, 72) to use the new dollar-suffix convention (e.g., level$str, status$int, duration_ms$int) so the examples match the documented schema naming; search for any other underscore-suffixed examples and replace them with the $-suffixed equivalents to keep the README consistent.crates/logfwd/tests/integration.rs (1)
451-457:⚠️ Potential issue | 🟡 MinorStale comment references old naming convention.
The comment at lines 453-454 still says "
{field}_{type}convention" but the code now uses{field}$type. Update the comment to match.Proposed fix
// SQL joins the log batch with the enrichment table on the `service` field. - // CSV columns use plain names (no `_str` suffix); scanner columns use the - // `{field}_{type}` convention. The alias brings the enriched column into + // CSV columns use plain names (no `$str` suffix); scanner columns use the + // `{field}$type` convention. The alias brings the enriched column into // the logfwd naming scheme for downstream compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd/tests/integration.rs` around lines 451 - 457, The comment describing column naming is stale: update the phrase "`{field}_{type}` convention" to the current "`{field}$type` convention" in the block that documents the SQL example (look for the sql variable and the surrounding comment about CSV/scanner columns and the alias: the lines that mention "CSV columns use plain names (no `_str` suffix); scanner columns use the `{field}_{type}` convention"). Replace only the naming example text so the comment matches the actual `{field}$type` pattern used in the SQL.crates/logfwd-transform/src/rewriter.rs (3)
32-51:⚠️ Potential issue | 🟡 MinorStale struct documentation still references underscore convention.
FieldTypesandFieldTypeMapdoc comments reference{field}_str,{field}_int,{field}_float. Update to use$delimiters.Proposed fix
/// Which Arrow type variants the scanner has produced for a JSON field. /// /// When the scanner encounters a JSON field that appears as multiple types -/// across log lines, it creates separate columns for each type: `{field}_str`, -/// `{field}_int`, `{field}_float`. This struct records which variants exist. +/// across log lines, it creates separate columns for each type: `{field}$str`, +/// `{field}$int`, `{field}$float`. This struct records which variants exist. #[derive(Debug, Clone, Default)] pub struct FieldTypes { - /// The `{field}_str` column exists in the schema. + /// The `{field}$str` column exists in the schema. pub has_str: bool, - /// The `{field}_int` column exists in the schema. + /// The `{field}$int` column exists in the schema. pub has_int: bool, - /// The `{field}_float` column exists in the schema. + /// The `{field}$float` column exists in the schema. pub has_float: bool, } /// Maps bare JSON field names (e.g., `"level"`) to the type variants -/// present in the current Arrow schema (e.g., `_str`, `_int`, `_float`). +/// present in the current Arrow schema (e.g., `$str`, `$int`, `$float`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-transform/src/rewriter.rs` around lines 32 - 51, Update the doc comments on FieldTypes and FieldTypeMap to stop using brace-style `{field}_str`/`{field}_int`/`{field}_float` and instead use dollar-delimited names (e.g., `$field_str`, `$field_int`, `$field_float`) so the documentation matches the project's delimiter convention; modify the doc strings above the FieldTypes struct and the FieldTypeMap type alias accordingly.
1-17:⚠️ Potential issue | 🟡 MinorStale module documentation still references underscore convention.
The module-level doc comments describe rewrite rules using
_str,_int,_floatsuffixes (e.g.,level_str = 'ERROR',status_int > 400), but the implementation now uses$delimiters.Update these comments to reflect the new convention.
Proposed fix
// rewriter.rs — SQL rewriter that translates user-friendly bare-column SQL // to typed-column SQL that the scanner's typed Arrow schema can handle. // -// The scanner produces columns named `{field}_str`, `{field}_int`, and -// `{field}_float`. Users naturally write `level = 'ERROR'` or `status > 400`, -// but DataFusion needs `level_str = 'ERROR'` or `status_int > 400`. +// The scanner produces columns named `{field}$str`, `{field}$int`, and +// `{field}$float`. Users naturally write `level = 'ERROR'` or `status > 400`, +// but DataFusion needs `level$str = 'ERROR'` or `status$int > 400`. // // Rewrite rules: // 1. Bare column in SELECT: `duration_ms` -// → `COALESCE(CAST(duration_ms_int AS VARCHAR), duration_ms_str) AS duration_ms` +// → `COALESCE(CAST(duration_ms$int AS VARCHAR), duration_ms$str) AS duration_ms` // 2. Bare column in WHERE with string literal: `level = 'ERROR'` -// → `level_str = 'ERROR'` +// → `level$str = 'ERROR'` // 3. Bare column in WHERE with numeric literal: `status > 400` -// → `status_int > 400` +// → `status$int > 400` // 4. `int(x)` call where `x` is a known field: -// → `COALESCE(x_int, TRY_CAST(x_str AS BIGINT))` +// → `COALESCE(x$int, TRY_CAST(x$str AS BIGINT))`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-transform/src/rewriter.rs` around lines 1 - 17, Update the module-level doc comments in rewriter.rs to reflect the new dollar-delimited column naming convention (e.g., use `{field}$str`, `{field}$int`, `{field}$float`) instead of the old underscore convention; update the example rewrite rules so SELECT shows COALESCE(CAST(duration_ms$int AS VARCHAR), duration_ms$str) AS duration_ms, WHERE string literal uses level$str = 'ERROR', numeric literal uses status$int > 400, and the int(x) rule shows COALESCE(x$int, TRY_CAST(x$str AS BIGINT)) — keep references to the same symbols (COALESCE, CAST, TRY_CAST, int(x)) so readers can find the related logic.
86-106:⚠️ Potential issue | 🟡 MinorStale function documentation still references underscore convention.
The
rewrite_sqldoc comments at lines 88-106 still describe rules using_str,_int,_float. Update to match implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-transform/src/rewriter.rs` around lines 86 - 106, The doc comment for rewrite_sql is stale: it describes typed columns using underscore suffixes (_str/_int/_float) but the implementation uses the dollar-suffix convention ($str/$int/$float); update the comment block in rewriter.rs for the rewrite_sql function to reflect the actual behavior and examples (replace underscore-suffixed examples with $str/$int/$float forms, adjust the SELECT/WHERE/int(x) examples to show the $-prefixed typed column names and any COALESCE/TRY_CAST semantics the function produces) so the docs match the implementation.crates/logfwd-output/src/stdout.rs (1)
198-210:⚠️ Potential issue | 🔴 CriticalUse actual Arrow DataType for dispatch, not the column name suffix.
The
stdout.rsformatter incorrectly dispatches on thesuffixstring instead of the actualfield.data_type(). A column with an empty suffix butInt64type (e.g.,COUNT(*)) falls through tostr_value(), which returns empty string for non-string types, causing silent data loss. Theotlp_sink.rsalready implements the correct pattern (lines 293–310)—match on the actualDataType:match field.data_type() { DataType::Int64 => ..., DataType::Float64 => ..., _ => ..., }Apply the same approach in
stdout.rsto handle type mismatches correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-output/src/stdout.rs` around lines 198 - 210, The match currently dispatches by the column name suffix and should instead match on the Arrow DataType to avoid dropping non-string numeric values; in the same function in stdout.rs where you have let arr = batch.column(idx) and match suffix { ... }, replace that branch with a match on the column field's data_type() (e.g., match field.data_type() { DataType::Int64 => { let arr = arr.as_primitive::<arrow::datatypes::Int64Type>(); write!(self.buf, "{}", arr.value(row))? }, DataType::Float64 => { let arr = arr.as_primitive::<arrow::datatypes::Float64Type>(); write!(self.buf, "{}", arr.value(row))? }, _ => { self.buf.extend_from_slice(str_value(arr, row).as_bytes()); } }) so numeric types are handled via as_primitive and other types fall back to str_value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd-output/src/lib.rs`:
- Around line 87-90: parse_column_name() currently unconditionally strips
trailing $str/$int/$float from any col_name (variable col_name and suffix)
causing collisions for literal names like "cost$int"; change the logic to only
treat the suffix as a type marker when it is provenance-marked or escaped (e.g.,
require a scanner-generated prefix or a single special marker before the type
token, or require that a literal $ is escaped as $$ and implement reversible
unescaping), update parse_column_name() to detect that provenance/escape (using
the existing col_name variable and suffix check) and only remove the suffix in
that case, and add a regression test that writes and parses an output column
literally named "cost$int" to ensure it remains distinct (test the round-trip
serialization/deserialization and escaping behavior).
In `@crates/logfwd-output/src/stdout.rs`:
- Around line 102-104: The console currently binds ts_idx, level_idx, and
msg_idx once from find_col(fields, ...) which causes it to miss non-primary
variants when the chosen column is null; update the rendering to resolve
timestamp, level, and message from the row's ColInfo at render time by checking
each well-known-variant column in order and selecting the first non-null value
(i.e., implement per-row resolution instead of a single static index); likewise
change the extras loop logic so it does not skip a logical field just because
the primary variant matched msg_idx—only skip a logical field when all its
variant columns are null; apply the same per-row resolution fix to the
corresponding code block referenced around lines 154-168.
---
Outside diff comments:
In `@crates/logfwd-output/src/stdout.rs`:
- Around line 198-210: The match currently dispatches by the column name suffix
and should instead match on the Arrow DataType to avoid dropping non-string
numeric values; in the same function in stdout.rs where you have let arr =
batch.column(idx) and match suffix { ... }, replace that branch with a match on
the column field's data_type() (e.g., match field.data_type() { DataType::Int64
=> { let arr = arr.as_primitive::<arrow::datatypes::Int64Type>();
write!(self.buf, "{}", arr.value(row))? }, DataType::Float64 => { let arr =
arr.as_primitive::<arrow::datatypes::Float64Type>(); write!(self.buf, "{}",
arr.value(row))? }, _ => { self.buf.extend_from_slice(str_value(arr,
row).as_bytes()); } }) so numeric types are handled via as_primitive and other
types fall back to str_value.
In `@crates/logfwd-transform/src/rewriter.rs`:
- Around line 32-51: Update the doc comments on FieldTypes and FieldTypeMap to
stop using brace-style `{field}_str`/`{field}_int`/`{field}_float` and instead
use dollar-delimited names (e.g., `$field_str`, `$field_int`, `$field_float`) so
the documentation matches the project's delimiter convention; modify the doc
strings above the FieldTypes struct and the FieldTypeMap type alias accordingly.
- Around line 1-17: Update the module-level doc comments in rewriter.rs to
reflect the new dollar-delimited column naming convention (e.g., use
`{field}$str`, `{field}$int`, `{field}$float`) instead of the old underscore
convention; update the example rewrite rules so SELECT shows
COALESCE(CAST(duration_ms$int AS VARCHAR), duration_ms$str) AS duration_ms,
WHERE string literal uses level$str = 'ERROR', numeric literal uses status$int >
400, and the int(x) rule shows COALESCE(x$int, TRY_CAST(x$str AS BIGINT)) — keep
references to the same symbols (COALESCE, CAST, TRY_CAST, int(x)) so readers can
find the related logic.
- Around line 86-106: The doc comment for rewrite_sql is stale: it describes
typed columns using underscore suffixes (_str/_int/_float) but the
implementation uses the dollar-suffix convention ($str/$int/$float); update the
comment block in rewriter.rs for the rewrite_sql function to reflect the actual
behavior and examples (replace underscore-suffixed examples with
$str/$int/$float forms, adjust the SELECT/WHERE/int(x) examples to show the
$-prefixed typed column names and any COALESCE/TRY_CAST semantics the function
produces) so the docs match the implementation.
In `@crates/logfwd/tests/integration.rs`:
- Around line 451-457: The comment describing column naming is stale: update the
phrase "`{field}_{type}` convention" to the current "`{field}$type` convention"
in the block that documents the SQL example (look for the sql variable and the
surrounding comment about CSV/scanner columns and the alias: the lines that
mention "CSV columns use plain names (no `_str` suffix); scanner columns use the
`{field}_{type}` convention"). Replace only the naming example text so the
comment matches the actual `{field}$type` pattern used in the SQL.
In `@README.md`:
- Around line 89-97: The README's SQL examples use underscore-suffixed column
names (e.g., level_str, status_int, duration_ms_int) but the doc states JSON
fields are suffixed with $str/$int/$float; update every SQL example that
references underscore suffixes (including the occurrences currently at lines
showing level_str, status_int, duration_ms_int and the other examples at lines
33, 57, 72) to use the new dollar-suffix convention (e.g., level$str,
status$int, duration_ms$int) so the examples match the documented schema naming;
search for any other underscore-suffixed examples and replace them with the
$-suffixed equivalents to keep the README consistent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7800e53d-de5e-4de5-bab2-18d6a2c35724
📒 Files selected for processing (11)
README.mdcrates/logfwd-arrow/src/scanner.rscrates/logfwd-arrow/src/storage_builder.rscrates/logfwd-arrow/src/streaming_builder.rscrates/logfwd-output/src/lib.rscrates/logfwd-output/src/stdout.rscrates/logfwd-transform/src/lib.rscrates/logfwd-transform/src/rewriter.rscrates/logfwd-transform/tests/scanner_datafusion_boundary.rscrates/logfwd/tests/compliance.rscrates/logfwd/tests/integration.rs
Contributor
|
Implemented and pushed follow-up commit
Tests run
Note 🔒 Integrity filtering filtered 1 itemIntegrity filtering activated and filtered the following item during workflow execution.
What is this? | From workflow: AI: Address PR Review Feedback Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/logfwd-output/src/lib.rs (1)
218-233:⚠️ Potential issue | 🟠 MajorUse one JSON string escaper for both keys and values.
field_nameis written into the object key verbatim, and the value path only escapes",\,\n,\r, and\t. A key or value containing backspace, form-feed, NUL, or a quote/backslash in the key will emit invalid JSON. Please route both sites through one shared escaper.Also applies to: 259-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-output/src/lib.rs` around lines 218 - 233, The code writes object keys (field_name) and string values with different, incomplete escaping; create a single helper (e.g., escape_json_string or json_escape) and call it from both places (the key path that writes field_name and the value path that writes str_value(arr, row)) so keys and values are escaped identically. The helper should iterate bytes/characters and produce a JSON-safe representation: escape `"` and `\`, map control chars like newline/return/tab to `\n`, `\r`, `\t`, and either escape backspace/form-feed/NUL and any U+0000–U+001F control bytes using `\b`, `\f`, `\0` or `\u00XX` as appropriate, and return a byte/str slice to be wrapped with surrounding quotes when written to out; replace the inlined match blocks at both sites (including the other occurrence referenced at 259-263) with calls to this new helper.
♻️ Duplicate comments (2)
crates/logfwd-output/src/stdout.rs (1)
159-165:⚠️ Potential issue | 🟠 MajorNumeric
messagevariants are still dropped from the main console line.
best_variant_for_well_known()can now pickmessage$int/message$float, but this branch still renders the chosen column withstr_value(). Ifmessage$strexists in the schema but is null for a row whilemessage$intis populated, the main message stays empty and the extras loop skips the logical field because one variant matchedmsg_idx. Use the same type-aware formatter here that extras already use.Minimal fix
- if let Some((idx, _)) = + if let Some((idx, suffix)) = best_variant_for_well_known(&cols, "message", row, batch, msg_idx) .or_else(|| best_variant_for_well_known(&cols, "msg", row, batch, msg_idx)) { let col = batch.column(idx); if !col.is_null(row) { - self.buf.extend_from_slice(str_value(col, row).as_bytes()); + write_console_value(&mut self.buf, col, row, suffix)?; } }Also applies to: 173-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-output/src/stdout.rs` around lines 159 - 165, The branch that handles well-known message columns calls str_value() and appends bytes unconditionally, which drops numeric variants like message$int/message$float; update the logic inside the best-variant handling (the block using best_variant_for_well_known(...), batch.column(idx), col.is_null(row), and self.buf.extend_from_slice(...)) to format the column value using the same type-aware formatter used by the extras loop (instead of always using str_value), so numeric and float variants render into the main message; apply the same change to the analogous second occurrence that currently uses str_value (the other block mentioned in the diff).crates/logfwd-output/src/lib.rs (1)
117-127:⚠️ Potential issue | 🔴 CriticalMatching-type literal aliases still collide.
A suffix/type match is not provenance.
parse_output_column_name()still rewrites direct columns likecost$int(Int64) ornote$str(Utf8) tocost/note, sobuild_col_infos()can merge a literal name with the real base field and drop one value. The new regression only covers the mismatched-type case (Utf8+$int). This still needs provenance/escaping plus a matching-type regression.Also applies to: 611-637
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-output/src/lib.rs` around lines 117 - 127, parse_output_column_name currently strips a type-suffix (e.g., "cost$int" -> "cost") whenever the suffix matches the DataType, which loses provenance and causes collisions in build_col_infos; change parse_output_column_name so it does NOT strip the suffix when the suffix actually matches the column's DataType (i.e., only rewrite/strip when the suffix is present but mismatches the real DataType), and ensure callers such as build_col_infos respect this behavior (only treat the returned base as stripped when the function actually rewrote it). Use the function name parse_output_column_name and callers like build_col_infos to locate and update the logic so matching-type literal aliases keep their suffix (preserving provenance) while the mismatched-type regression handling remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd-output/src/lib.rs`:
- Around line 109-123: The classification allows DataType::Dictionary(_,
value_type) as string-like (is_string_like_type), but str_value and output
writers don't unwrap dictionaries, so dictionary-backed string columns emit
empty strings; update str_value to handle Dictionary variants by downcasting the
ArrayRef to a DictionaryArray, retrieving the value array (Utf8/Utf8View) and
mapping keys to their string values, or alternatively remove Dictionary(...)
from the match in is_string_like_type so parse_output_column_name no longer
treats dictionaries as "str"; modify the logic near is_string_like_type,
parse_output_column_name and str_value to keep the type handling consistent
(prefer implementing dictionary unwrapping in str_value to preserve
functionality).
---
Outside diff comments:
In `@crates/logfwd-output/src/lib.rs`:
- Around line 218-233: The code writes object keys (field_name) and string
values with different, incomplete escaping; create a single helper (e.g.,
escape_json_string or json_escape) and call it from both places (the key path
that writes field_name and the value path that writes str_value(arr, row)) so
keys and values are escaped identically. The helper should iterate
bytes/characters and produce a JSON-safe representation: escape `"` and `\`, map
control chars like newline/return/tab to `\n`, `\r`, `\t`, and either escape
backspace/form-feed/NUL and any U+0000–U+001F control bytes using `\b`, `\f`,
`\0` or `\u00XX` as appropriate, and return a byte/str slice to be wrapped with
surrounding quotes when written to out; replace the inlined match blocks at both
sites (including the other occurrence referenced at 259-263) with calls to this
new helper.
---
Duplicate comments:
In `@crates/logfwd-output/src/lib.rs`:
- Around line 117-127: parse_output_column_name currently strips a type-suffix
(e.g., "cost$int" -> "cost") whenever the suffix matches the DataType, which
loses provenance and causes collisions in build_col_infos; change
parse_output_column_name so it does NOT strip the suffix when the suffix
actually matches the column's DataType (i.e., only rewrite/strip when the suffix
is present but mismatches the real DataType), and ensure callers such as
build_col_infos respect this behavior (only treat the returned base as stripped
when the function actually rewrote it). Use the function name
parse_output_column_name and callers like build_col_infos to locate and update
the logic so matching-type literal aliases keep their suffix (preserving
provenance) while the mismatched-type regression handling remains.
In `@crates/logfwd-output/src/stdout.rs`:
- Around line 159-165: The branch that handles well-known message columns calls
str_value() and appends bytes unconditionally, which drops numeric variants like
message$int/message$float; update the logic inside the best-variant handling
(the block using best_variant_for_well_known(...), batch.column(idx),
col.is_null(row), and self.buf.extend_from_slice(...)) to format the column
value using the same type-aware formatter used by the extras loop (instead of
always using str_value), so numeric and float variants render into the main
message; apply the same change to the analogous second occurrence that currently
uses str_value (the other block mentioned in the diff).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9b9ebb3-512e-43b0-8887-4be17eb51622
📒 Files selected for processing (3)
crates/logfwd-output/src/json_lines.rscrates/logfwd-output/src/lib.rscrates/logfwd-output/src/stdout.rs
Contributor
There was a problem hiding this comment.
Requesting changes: there are still correctness issues in suffix handling and JSON serialization that can cause dropped fields, wrong output types, or panics in concrete query patterns.
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
What is this? | From workflow: AI: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| /// the raw JSON field name. E.g., `level$str` → `level`, `duration_ms$int` → `duration_ms`. | ||
| fn strip_type_suffix(name: &str) -> String { | ||
| for suffix in &["_str", "_int", "_float"] { | ||
| for suffix in &["$str", "$int", "$float"] { |
Contributor
There was a problem hiding this comment.
strip_type_suffix() now strips $str/$int/$float unconditionally, which can still mis-handle literal JSON keys that end with those tokens when they are referenced as bare fields in SQL.
Concrete failure case:
- input key:
"cost$int" - query:
SELECT cost$int FROM logs scan_config()maps referenced columns throughstrip_type_suffix()and requestscostinstead ofcost$int, so scanner pushdown can drop the actual field before SQL runs.
Could we add a regression test for this key pattern and gate suffix stripping to scanner-generated typed refs (or a reversible escape/provenance marker)?
| // Value — type-aware | ||
| match col.type_suffix.as_str() { | ||
| let arr = batch.column(idx); | ||
| match suffix { |
Contributor
There was a problem hiding this comment.
write_row_json() still dispatches on parsed suffix, not Arrow DataType, so there are two failure modes here:
-
Untyped numeric aliases become empty strings
SELECT COUNT(*) AS cntyields anInt64column namedcnt(suffix""), so this path falls into_ => str_value(...);str_value()returns""for non-Utf8/Utf8View, producing{"cnt":""}. -
Suffix/type mismatch can panic
If a string column is aliased as... AS cost$int, this branch downcasts Utf8 to Int64 and panics.
Can we switch JSON value encoding to dispatch by arr.data_type() (or at least validate suffix/type agreement before downcasting) and add a regression covering COUNT(*) AS cnt?
Change the internal type suffix separator from '_' to '$' (e.g., 'status$int' instead of 'status_int') to prevent collisions with user-defined field names that naturally end with _str, _int, or _float. Redesign JSON serialization to group all type variants for a logical field and select the first non-null variant per row. This ensures that data is correctly preserved even when a field's type varies across records in the same batch. Updated scanner builders, SQL rewriter, output sinks, and documentation. Migrated all workspace tests to the new naming convention. Fixes an issue where field names ending in common type suffixes were silently renamed or dropped in JSON output. Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
… resolution - avoid stripping literal `//` names unless type matches\n- serialize JSON/console values by Arrow data type\n- propagate JSON write errors instead of swallowing\n- resolve well-known console fields per row across variants\n- add regression tests for aggregate numeric output and literal suffix fields\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Changed the internal type suffix separator from `_` to `$` (e.g., `field$int` instead of `field_int`) to prevent collisions with user-defined field names ending in `_str`, `_int`, or `_float`. - Redesigned JSON serialization in `logfwd-output` to group all type variants for a logical field and row-wise select the first non-null variant according to type priority (int > float > str). - Updated scanner builders, SQL rewriter, query analyzer, and output sinks to support the new convention. - Migrated all workspace tests, benchmarks, and E2E manifests to the new naming convention. - Updated `README.md` documentation. - Fixed regressions in `otlp` and `stdout` sinks and ensured all tests pass workspace-wide. This fix prevents silent data loss and incorrect renaming of JSON fields when they share names with internal type-suffixed columns. Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
- Resolve rebase conflicts: keep RFC 8259 inf/NaN fix from master - Fix test assertions: update _str/_int/_float refs to $str/$int/$float - Fix _raw column name in compliance_raw_format test (no suffix) - Fix console rendering: dispatch on Arrow DataType instead of suffix string - Update rewriter.rs doc comments to use $ separator convention Addresses CodeRabbit review feedback on #422. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c280757 to
eb92308
Compare
There was a problem hiding this comment.
Actionable comments posted: 46
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
dev-docs/research/wide-crate-evaluation.md (1)
41-54:⚠️ Potential issue | 🟠 MajorInvalid Rust syntax:
find$structural_charscannot be a function name.
$is not valid in Rust identifiers. The PR's$separator is for data column names (Arrow/JSON schema), not Rust code. This should remainfind_structural_chars.Suggested fix
/// Detect all 10 structural characters. One function, all platforms. -pub fn find$structural_chars(block: &[u8; 64]) -> RawBlockMasks { +pub fn find_structural_chars(block: &[u8; 64]) -> RawBlockMasks {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/research/wide-crate-evaluation.md` around lines 41 - 54, The function name find$structural_chars uses an invalid `$` identifier; rename it to find_structural_chars and update every reference/call site accordingly (including tests and any module exports) so the signature pub fn find_structural_chars(block: &[u8; 64]) -> RawBlockMasks { ... } compiles; keep the body as-is using RawBlockMasks and mask64 to locate newline, space, quote, backslash, comma, colon, open_brace, close_brace, open_bracket, close_bracket.crates/logfwd-bench/src/rss.rs (1)
113-126:⚠️ Potential issue | 🔴 CriticalSyntax error:
after$streamingis invalid Rust.Variable names cannot contain
$. Should beafter_streaming.- let after$streaming = rss_mb(); + let after_streaming = rss_mb(); let streaming_cols = batch.num_columns(); let streaming_reported = batch.get_array_memory_size() as f64 / 1_048_576.0; drop(batch); drop(streaming_scanner); println!(" Raw JSON: {:.1} MB", raw_mb); println!(" StorageBuilder: RSS +{:.1} MB (reported {:.1} MB) {} cols", after_storage - before, storage_reported, storage_cols); println!(" StreamingBuilder: RSS +{:.1} MB (reported {:.1} MB) {} cols", - after$streaming - mid, streaming_reported, streaming_cols); + after_streaming - mid, streaming_reported, streaming_cols); println!(" Savings: {:.1} MB ({:.0}% less RSS)", - (after_storage - before) - (after$streaming - mid), - 100.0 * (1.0 - (after$streaming - mid) / (after_storage - before))); + (after_storage - before) - (after_streaming - mid), + 100.0 * (1.0 - (after_streaming - mid) / (after_storage - before)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-bench/src/rss.rs` around lines 113 - 126, The identifier after$streaming is invalid in Rust; rename it to after_streaming everywhere it appears (the binding from rss_mb() and all uses in the println! calls and the final savings calculation). Update the variable declaration (currently let after$streaming = rss_mb();) to let after_streaming = rss_mb(); and replace after$streaming - mid and any other after$streaming references with after_streaming - mid so streaming_reported, streaming_cols, streaming_scanner, and the savings arithmetic use the corrected name.crates/logfwd-transform/tests/scanner_datafusion_boundary.rs (1)
9-9:⚠️ Potential issue | 🟡 MinorUpdate doc comment to reflect new naming convention.
Line 9 states the column naming follows
{field}_{type}but the code now uses{field}$type.-//! Column naming follows the logfwd convention: `{field}_{type}`. +//! Column naming follows the logfwd convention: `{field}$type`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-transform/tests/scanner_datafusion_boundary.rs` at line 9, Update the doc comment that currently says "Column naming follows the logfwd convention: `{field}_{type}`" to reflect the new naming convention used by the code: change it to state `{field}$type` (e.g., update the comment in the scanner_datafusion_boundary test module / the top-level doc comment that begins "Column naming follows the logfwd convention" so it matches the actual column naming used by the code).docs/CONFIG_REFERENCE.md (1)
293-308:⚠️ Potential issue | 🟡 MinorDocumentation table still references underscore convention in description.
Line 296 says "following the
{field}_{type}naming convention" but the table shows$suffix patterns. Update the description for consistency.The scanner maps each JSON field to one or more typed Arrow columns following the -`{field}_{type}` naming convention: +`{field}$type` naming convention:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CONFIG_REFERENCE.md` around lines 293 - 308, Update the "Column naming convention" description to match the table's actual pattern: replace the inaccurate `{field}_{type}` phrase with the `$`-suffix pattern (e.g., `{field}$type` or " `{field}$str`, `{field}$int`, `{field}$float`") so the header text and examples like `level$str` and `status$int` are consistent with the table and subsequent explanation about mixed-type columns (`status$int` and `status$str`).crates/logfwd-transform/src/rewriter.rs (2)
95-106:⚠️ Potential issue | 🟡 MinorDocumentation uses outdated underscore convention.
The doc comments in lines 96, 99, 102-103, and 106 still reference
_int/_str/_floatsuffixes (e.g.,duration_ms_int,_str,_int,_float) while the implementation now uses$separators. This creates confusion for readers.-/// 1. **Bare column in SELECT** — e.g. `SELECT duration_ms FROM logs` -/// becomes `SELECT COALESCE(CAST(duration_ms_int AS VARCHAR), duration_ms_str) AS duration_ms FROM logs` +/// 1. **Bare column in SELECT** — e.g. `SELECT duration_ms FROM logs` +/// becomes `SELECT COALESCE(CAST(duration_ms$int AS VARCHAR), duration_ms$str) AS duration_ms FROM logs` /// /// 2. **String comparison in WHERE** — e.g. `WHERE level = 'ERROR'` -/// becomes `WHERE level$str = 'ERROR'` (uses `_str` variant) +/// becomes `WHERE level$str = 'ERROR'` (uses `$str` variant) /// /// 3. **Numeric comparison in WHERE** — e.g. `WHERE status > 400` -/// becomes `WHERE status$int > 400` (uses `_int` variant, or `_float` if +/// becomes `WHERE status$int > 400` (uses `$int` variant, or `$float` if /// only float exists) /// /// 4. **`int(x)` call** — e.g. `int(duration_ms)` becomes -/// `COALESCE(duration_ms_int, TRY_CAST(duration_ms_str AS BIGINT))` +/// `COALESCE(duration_ms$int, TRY_CAST(duration_ms$str AS BIGINT))`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-transform/src/rewriter.rs` around lines 95 - 106, Update the doc comments in rewriter.rs to use the current `$` suffix convention instead of the outdated `_` underscores: change examples like `duration_ms_int`/`duration_ms_str` to `duration_ms$int`/`duration_ms$str`, `level_str` to `level$str`, `status_int`/`status_float` to `status$int`/`status$float`, and the `int(x)` example from `COALESCE(duration_ms_int, TRY_CAST(duration_ms_str AS BIGINT))` to `COALESCE(duration_ms$int, TRY_CAST(duration_ms$str AS BIGINT))` so the docs match the implementation.
419-422:⚠️ Potential issue | 🟡 MinorComment references outdated naming convention.
// --------------------------------------------------------------------------- -// Rule 4 helpers: int(x) → COALESCE(x_int, TRY_CAST(x_str AS BIGINT)) +// Rule 4 helpers: int(x) → COALESCE(x$int, TRY_CAST(x$str AS BIGINT)) // --------------------------------------------------------------------------- -/// Build `COALESCE(x_int, TRY_CAST(x_str AS BIGINT))` for the `int(x)` rule. +/// Build `COALESCE(x$int, TRY_CAST(x$str AS BIGINT))` for the `int(x)` rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-transform/src/rewriter.rs` around lines 419 - 422, Update the doc comment that currently uses the outdated naming convention for generated columns in the "Rule 4 helpers" section (the comment showing int(x) → COALESCE(x_int, TRY_CAST(x_str AS BIGINT))). Change the example and any textual references to match the current naming scheme used by the codebase (e.g., the actual generated column suffixes used by the rewriter functions in this module), and ensure the comment near the helper that builds the COALESCE expression reflects the current names (so the description and any examples match the symbols produced by the helper).book/src/config/reference.md (1)
294-306:⚠️ Potential issue | 🟡 MinorText says
{field}_{type}but convention is now{field}$type.Line 296 still describes the old underscore convention while the table shows the new
$convention.📝 Proposed fix
### Column naming convention The scanner maps each JSON field to one or more typed Arrow columns following the -`{field}_{type}` naming convention: +`{field}$type` naming convention:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@book/src/config/reference.md` around lines 294 - 306, The doc text still references the old naming convention `{field}_{type}` while the table (and current convention) uses `{field}$type`; update the sentence that reads "The scanner maps each JSON field to one or more typed Arrow columns following the `{field}_{type}` naming convention" to use `{field}$type` (match the table and examples like `level$str`, `status$int`) so the prose and examples are consistent with the `{field}$type` convention.crates/logfwd-output/src/lib.rs (3)
201-241:⚠️ Potential issue | 🔴 CriticalSuffix/type mismatch still causes panic — dispatch on
DataType, not suffix.Lines 204 and 210 downcast based on the parsed
suffix, not the actual ArrowDataType. If a column is namedcount$intbut containsUtf8data,as_primitive::<Int64Type>()panics.The past review at line 202 flagged this. The commit response claims it was fixed, but the code still shows:
match suffix { "int" => { let arr = arr.as_primitive::<Int64Type>(); ... }Fix by checking
arr.data_type()before downcasting, or skip variants where suffix doesn't match the actual type.🐛 Proposed fix: validate type before downcast
let Some((idx, suffix)) = best_variant else { continue; }; // ...key emission... let arr = batch.column(idx); - match suffix { + match (suffix, arr.data_type()) { - "int" => { + ("int", DataType::Int64) => { let arr = arr.as_primitive::<arrow::datatypes::Int64Type>(); // ... } - "float" => { + ("float", DataType::Float64) => { let arr = arr.as_primitive::<arrow::datatypes::Float64Type>(); // ... } _ => { - // str or untyped — treat as string + // str, untyped, or mismatched suffix — treat as string let v = str_value(arr, row); // ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-output/src/lib.rs` around lines 201 - 241, The match on the parsed suffix currently downcasts the Arrow array (variable arr from batch.column(idx)) based only on suffix (e.g., "int" -> as_primitive::<Int64Type>), which can panic if the real array data_type() differs; update the logic in the function that serializes column values so you first inspect arr.data_type() (or the ArrayRef's concrete type) and only perform as_primitive::<...>() / as_any().downcast_ref<...>() when the actual DataType matches the expected suffix, otherwise fall back to the string branch (str_value) or emit null/skip the numeric branch; reference the existing symbols arr, suffix, str_value, and batch.column(idx) to locate and guard each numeric ("int", "float") branch by validating arr.data_type() before downcasting.
732-758: 🛠️ Refactor suggestion | 🟠 MajorAdd JSON equivalent of this OTLP mismatch test.
test_otlp_type_mismatch_no_paniccovers OTLP encoding, butwrite_row_jsonhas the same suffix/type mismatch hazard and no equivalent test. Add a test with aUtf8column namedcount$intto verify JSON output doesn't panic.💚 Proposed test
#[test] fn test_json_type_mismatch_no_panic() { // Column named "count$int" but actually contains strings. let schema = Arc::new(Schema::new(vec![ Field::new("count$int", DataType::Utf8, true), ])); let count = StringArray::from(vec![Some("high")]); let batch = RecordBatch::try_new(schema, vec![Arc::new(count)]).unwrap(); let cols = build_col_infos(&batch); let mut out = Vec::new(); // Must not panic — should fall through to string serialization write_row_json(&batch, 0, &cols, &mut out); let json = String::from_utf8(out).unwrap(); // Should serialize as string, not attempt int downcast assert!(json.contains("\"count\":\"high\"") || json.contains("\"count\":")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-output/src/lib.rs` around lines 732 - 758, Add a new unit test mirroring test_otlp_type_mismatch_no_panic but for JSON: create a RecordBatch with a Utf8 column named "count$int" (value "high"), call build_col_infos(&batch) and then write_row_json(&batch, 0, &cols, &mut out), and assert the output UTF-8 JSON contains the serialized string value (e.g. it contains "\"count\":\"high\"" or at least "\"count\":") to ensure write_row_json falls back to string serialization and does not panic.
86-94:⚠️ Potential issue | 🔴 Critical
write_row_jsondowncasts based on suffix without validating actual Arrow DataType — will panic on literal$int/$str/$floatfield names.
parse_column_nameunconditionally strips these suffixes from any column name. Inwrite_row_json, the code then callsarr.as_primitive::<Int64Type>()orarr.as_primitive::<Float64Type>()based on the suffix string alone (lines 203–217), with no type guard. A user field literally namedcost$intwith DataType::Utf8 becomes("cost", "int"), then panics when downcasting fails.This was fixed in the OTLP encoder (test
test_otlp_type_mismatch_no_panicat line 733 validates the fix), but the JSON output path still lacks it. Add type validation before downcasts inwrite_row_json— check that the actual column DataType matches the suffix before the cast, or skip/treat-as-string on mismatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-output/src/lib.rs` around lines 86 - 94, parse_column_name currently strips suffixes like "$int" unconditionally and write_row_json then downcasts arrays (arr.as_primitive::<Int64Type>() / as_primitive::<Float64Type>() / as_string) based only on that suffix, which will panic on type mismatch; update write_row_json to check the actual Arrow DataType (arr.data_type()) before performing downcasts for the suffixes "int", "float", "str" and only call arr.as_primitive::<Int64Type>() or arr.as_primitive::<Float64Type>() when the DataType matches DataType::Int64 or DataType::Float64 respectively (and treat non-matching cases as string/skip or fallback to Utf8 handling), otherwise fall back to string handling or skip that field to avoid panics; reference the functions parse_column_name and write_row_json and the downcast points where arr.as_primitive::<Int64Type>() / arr.as_primitive::<Float64Type>() / arr.as_string() are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 46: Revert the accidental renaming of the parser state bitmask from
in_string to in$string in documentation: replace any occurrences of "in$string"
with the canonical "in_string" so the docs match the code and architecture notes
(see structural.rs's in_string bitmask and dev-docs/DECISIONS.md references);
ensure AGENTS.md and any other affected markdown files use in_string
consistently and do not treat it like a typed-column suffix.
In `@bench/explore.sh`:
- Around line 193-206: The QUERIES array entries use double quotes so
$str/$int/$float are expanded by the shell; update the QUERIES definition to
prevent variable expansion by either replacing the double quotes with single
quotes for each query string (e.g., make each element 'SELECT timestamp$str,
...') or keep double quotes but escape every $ as \$ in the array elements;
adjust the QUERIES array (the array literal named QUERIES) accordingly so the
SQL placeholders remain intact.
In `@book/src/config/reference.md`:
- Around line 425-433: Update the YAML config key and documentation to use
snake_case instead of the `$`-suffixed form: replace all occurrences of
`metrics$interval_secs` in the table entry and the example YAML snippet with
`metrics_interval_secs` (the table row showing `metrics$interval_secs` and the
YAML block under server:). Ensure the documentation and examples consistently
use `metrics_interval_secs` so parsers and readers use the conventional YAML
key.
In `@book/src/deployment/monitoring.md`:
- Line 34: Update the documented config key to match the Rust config parser by
replacing the dollar-sign variant with the underscore variant: change the
example key to metrics_interval_secs: 60 so it matches the struct field and
config access code that expect metrics_interval_secs.
In `@book/src/development/contributing.md`:
- Line 47: Update the documentation to use the correct Rust function
identifiers: replace occurrences of scan$string with scan_string and scan$into()
with scan_into() so the docs match the actual code symbols; search the
contributing.md references to scan$string and scan$into() and change them to
scan_string and scan_into() (preserving any surrounding punctuation) to avoid
mismatched symbol names.
In `@CODE_STYLE.md`:
- Line 28: The rule text incorrectly references a non-existent Rust method
`.to$string()`; update the CODE_STYLE.md entry which currently reads "**No
`format!()` or `.to$string()` in loops.**" to use the correct standard-library
method name `.to_string()` (i.e., "**No `format!()` or `.to_string()` in
loops.**") so the guidance is accurate; ensure you only change the method token
and leave the rest of the sentence intact.
In `@crates/logfwd-bench/src/rss.rs`:
- Line 33: There's a syntax typo: the call to std::fs::read_to$string should be
std::fs::read_to_string; update the identifier in the if-let expression (the
line using std::fs::read_to$string("/proc/self/status") inside rss.rs) to use
read_to_string so the code compiles and correctly reads the file into a String.
In `@crates/logfwd-competitive-bench/src/agents/logfwd.rs`:
- Around line 25-34: The SQL strings used to build `transform` inside the match
on the `Scenario` enum are duplicated in `bench/dashboard/index.html` and
`bench/dashboard/run.html`; extract these SQL snippets into a single shared
manifest (e.g., a JSON/YAML file or a Rust module with exported constants) and
have both `crates/logfwd-competitive-bench/src/agents/logfwd.rs` (the
`transform` match for `Scenario::Passthrough`, `Scenario::JsonParse`,
`Scenario::Filter`) and the dashboard pages read from that manifest instead of
embedding literals so column-name changes are updated in one place.
In `@crates/logfwd-output/src/stdout.rs`:
- Around line 172-183: The pattern binds an unused variable `suffix` from
`best_variant`, causing a CI unused-variable error; update the destructuring
where `best_variant` is matched (the `let Some((idx, suffix)) = best_variant
else { ... };` after the loop) to either drop `suffix` (e.g. match only `idx`)
or rename it to `_suffix` so the compiler knows it's intentionally unused; you
can also remove the `suffix` capture earlier when assigning `best_variant` in
the loop (set it to `Some(*idx)` or `Some((*idx, _))`) but ensure references to
`col.variants`, `batch.column(*idx).is_null(row)`, and `best_variant` remain
consistent.
In `@dev-docs/ARCHITECTURE.md`:
- Line 127: The documented function name "scan$into" in scanner.rs uses an
illegal dollar sign; rename the function to use underscore (e.g., scan_into) and
update all references (calls, tests, docs) to match, keeping the same signature,
visibility, and doc comments; ensure you run cargo build/test to catch remaining
references and update the ARCHITECTURE.md line to reflect the new name.
- Around line 103-106: The doc uses `$` in internal identifiers which is wrong;
update the identifiers to use underscores and Rust naming (e.g., change
in$string to in_string and scan$string() to scan_string()) throughout the text
and examples so internal bitmask and function names follow Rust conventions, and
make sure any explanatory sentences and code fences reflect the underscore names
consistently.
- Around line 136-141: The trait ScanBuilder contains invalid Rust identifiers
using `$` (append$str_by_idx, append$int_by_idx, append$float_by_idx,
append_null_by_idx, append_raw); rename these methods to snake_case valid Rust
names (e.g., append_str_by_idx, append_int_by_idx, append_float_by_idx,
append_null_by_idx, append_raw) in the ScanBuilder trait and update all
implementations and callers to use the new names so the trait compiles and
follows Rust naming conventions.
In `@dev-docs/DIRECTION.md`:
- Line 81: Update the documentation to correct the function name typo: replace
the incorrect symbol `find$structural_chars_scalar` with the correct
`find_structural_chars_scalar` in the sentence that declares the Kani-provable
specification so references and links resolve to the actual function.
In `@dev-docs/PROVEN_CORE.md`:
- Line 27: The doc shows invalid Rust identifiers parse$int_fast and
parse$float_fast; change any occurrences back to the correct internal function
names parse_int_fast and parse_float_fast inside scan_config.rs/ScanConfig (do
not expose or rename these internal symbols to JSON field names), and if a serde
rename was attempted, instead use serde(field = "...") or a separate
public-facing field name while keeping the internal function/identifier names
intact so Rust syntax remains valid.
- Line 58: The change replaced the original integer parsing function name
parse_int_fast with parse$int_fast; revert all usages and declarations back to
the original parse_int_fast identifier (search for parse$int_fast and replace
with parse_int_fast), ensuring any related tests, imports, exports, and
documentation references use parse_int_fast so the module and callers remain
consistent.
- Around line 132-135: The trait method names shown (field$str, field$int,
field$float) are invalid in Rust; rename them to valid identifiers matching the
existing FieldSink trait (e.g., field_str, field_int, field_float) and update
all call sites and implementations that reference those methods; ensure the
signatures (fn field_str(&mut self, idx: usize, value: &[u8]); fn field_int(&mut
self, idx: usize, value: &[u8]); fn field_float(&mut self, idx: usize, value:
&[u8]); and fn field_null(&mut self, idx: usize);) match the trait so
compilation and trait implementation resolution succeed.
In `@dev-docs/research/BOUNDARY_PATTERNS.md`:
- Line 7: The documentation incorrectly references Serde's Visitor API as
"visit$str"; update the text to use the correct method name "visit_str" and
ensure consistency with other referenced symbols like "visit_i64", "visit_map",
"Deserializer", and "Visitor" so the sentence accurately reflects Serde's trait
method names rather than a typo.
- Line 93: The function name parse_line$into uses an unconventional `$`
character; rename it to snake_case parse_line_into everywhere referenced
(definition and all call sites like parse_line$into) and update any related
documentation or tests to use the new identifier so the code follows Rust naming
conventions and avoids confusion with macro metavariables.
- Line 185: The call site currently invokes typed_value$strategy() in the tuple
(any::<Vec<u8>>(), typed_value$strategy()), 1..16; update this to call the
corrected function name typed_value_strategy() so it becomes (any::<Vec<u8>>(),
typed_value_strategy()), 1..16, ensuring the symbol name matches the corrected
definition (typed_value_strategy).
- Line 172: The function name typed_value$strategy uses an invalid/awkward `$`
separator; rename the symbol to typed_value_strategy and update all references
to it (definitions, calls, tests, module exports/imports, and documentation) so
the project uses the new snake_case name consistently; ensure you run cargo
check/tests to catch any remaining references to typed_value$strategy and fix
them to typed_value_strategy.
- Line 41: The doc incorrectly names Serde Visitor methods with `$` characters;
update the text to reference the correct method names: Visitor trait methods
visit_str(&str), visit_borrowed_str(&'de str), and visit_string(String), and
keep the surrounding explanation about Deserializer<'de> and logfwd's &'a [u8]
slices flowing through the visitor boundary without copies; ensure the paragraph
replaces visit$str, visit_borrowed$str, and visit$string with visit_str,
visit_borrowed_str, and visit_string respectively and retains the lifetime
mention tying the visitor to the input buffer.
In `@dev-docs/research/ENFORCEMENT_AND_MECHANICS.md`:
- Around line 116-122: The test uses invalid Rust identifiers with '$' — rename
all occurrences of parse$int_fast and verify_parse$int_fast to valid identifiers
parse_int_fast and verify_parse_int_fast (including the kani attribute
kani::proof_for_contract(parse_int_fast) and the call
parse_int_fast(&bytes[..len])) so the function and its verifier use the correct
underscore-separated names.
- Line 310: The identifiers parse$int_fast and parse$float_fast are invalid in
Rust; rename them (for example to parse_int_fast and parse_float_fast) in the
original definitions in scan_config.rs, update the pub use in logfwd-core to pub
use logfwd_kernel::{parse_int_fast, parse_float_fast}, and update every callsite
and import across the workspace (tests, crates, and any moved references) to the
new valid names so the code compiles.
In `@dev-docs/research/KANI_LIMITS.md`:
- Line 56: Update the invalid function identifier in the documentation by
replacing the incorrect `parse$string` token with the proper Rust-style
`parse_string` so it matches the adjacent documented functions (`detect_escape`,
`parse_number`) and aligns with actual code naming; locate the occurrence in the
KANI limits doc and update the text to `parse_string(bytes: &[u8]) ->
Result<(String, usize)>`.
In `@dev-docs/research/structural-index-research.md`:
- Line 10: Replace the incorrect identifier `platform$intrinsics` with Kani's
internal path name `platform_intrinsics` in the document and clarify that this
refers to Kani's internal intrinsic handling path (the `platform_intrinsics`
path), not a logfwd column name; update any surrounding text that implies it's a
compiler or log column to instead state it is Kani's intrinsic handling path.
- Line 91: The pseudocode uses `$` in names where Rust-style snake_case is
expected (e.g., "CriParser::extract" showing "space_bitmask" and "in$string");
replace any `$`-styled identifiers in pseudocode examples with normal snake_case
names — for example change in$string → in_string, carry$string → carry_string,
and use function names like find_structural_chars and is_in_string instead of
dollar-formatted names; update all occurrences in the affected pseudocode blocks
(including the instances around CriParser::extract and the ranges noted) so
variable and function names follow snake_case conventions.
In `@dev-docs/research/wide-crate-evaluation.md`:
- Line 183: The doc shows an incorrect function name "find$structural_chars" in
the before/after listings; update both occurrences to the correct identifier
"find_structural_chars" so the examples and any #[cfg] dispatch references match
the real function name (search for the token find$structural_chars and replace
it with find_structural_chars in the document).
- Around line 155-156: The doc uses invalid Rust identifiers with a dollar sign
(e.g., find$structural_chars_scalar, find$structural_chars) which Kani won’t
accept; update the text to use the actual Rust function names (replace '$' with
'_' so use find_structural_chars_scalar and find_structural_chars and the
corresponding _scalar name) and ensure they are formatted as code spans
(backticked) to match the real symbols used in the codebase and Kani
verification.
In `@DEVELOPING.md`:
- Line 68: Update the invalid Rust identifiers in DEVELOPING.md by replacing the
incorrect `scan$string` and `scan$into()` references with the actual function
names `scan_string` and `scan_into` used in the codebase (these appear around
the chunk-level classification description and the other mention at line ~113);
ensure the doc text and any inline code spans match the real symbols
(`scan_string`, `scan_into`) so contributors are not misled.
In `@docs/CONFIG_REFERENCE.md`:
- Around line 425-432: The docs example and any config parsing references use
the internal-style key metrics$interval_secs which must be replaced with the
user-facing metrics_interval_secs; update the YAML example (replace
metrics$interval_secs → metrics_interval_secs) and search for any usages of the
symbol metrics$interval_secs in docs or config-loading code (e.g., where server
config keys are mapped) and change them to metrics_interval_secs so the parser
and examples match the public config name and avoid requiring quoted keys in
YAML.
In `@docs/DEPLOYMENT.md`:
- Line 108: There is a typo in the documentation: the special-column name is
written as `$stream$str` but should be `_stream$str`; update the occurrence of
`$stream$str` to `_stream$str` to follow the underscore-prefix + type-suffix
convention used for `_raw`, `_file`, `_stream`, `_time` so the special column
names are consistent.
In `@docs/references/arrow-v54.md`:
- Line 151: The documentation incorrectly shows a dollar-sign variant; change
the call back to the real Arrow API name by replacing
.with_deduplicate$strings() with the actual Arrow method
.with_deduplicate_strings() (this is a StringViewBuilder/Arrow API method, not a
logfwd typed column), and ensure any surrounding text does not imply the $
separator applies to external library APIs like StringViewBuilder or
with_deduplicate_strings() — the $ convention is only for logfwd typed column
names (e.g., field$int, field$str).
- Line 509: The code incorrectly renames the variable used in the StringBuilder
capacity call to expected$strings; revert the name back to expected_strings in
the call to StringBuilder::with_capacity so the descriptive variable name is
preserved (i.e., restore expected_strings and expected_total_bytes as the
arguments to StringBuilder::with_capacity) and ensure no `$` separator is
introduced into the variable identifier.
- Line 460: The code incorrectly uses the logfwd-style suffix
(`arr.as$string::<i32>()`) instead of the Arrow AsArray trait method; change the
call back to the Arrow API method (e.g., replace arr.as$string::<i32>() with
arr.as_string::<i32>() or the correct AsArray method for StringArray) so that
the variable let strings: &StringArray = arr.as_string::<i32>(); (using
as_string) is used instead of the `$`-suffixed form.
- Around line 132-137: Restore the original example string literal used for the
StringViewArray demo by changing raw_bytes back to
b"helloworld_this_is_a_long_string" (undo the accidental replacement with a
dollar sign), so the code that builds the buffer
(builder.append_block(Buffer::from(raw_bytes))) and subsequent view operations
(builder.try_append_view(block, 0, 5) and builder.try_append_view(block, 5, 27))
reflect the intended example data; ensure only the raw_bytes value is updated
and no other logic around block, append_block, or try_append_view is changed.
In `@docs/references/datafusion-v45.md`:
- Line 382: The code snippet contains an invalid Rust identifier `to$string()`;
change the call on `sql` from `to$string()` to the correct Rust method
`to_string()` so the line reads like the others: replace `user_sql:
sql.to$string()` with `user_sql: sql.to_string()` in the `Ok(Self { ctx, rt,
user_sql: ... })` construction.
- Around line 307-309: The docs incorrectly show `SELECT Level_Str` resolving to
`level$str`; update the text to reflect DataFusion's case-folding (letters
lowercased, non-letter characters preserved): change `level$str` to `level_str`,
and adjust the example to demonstrate preservation of non-letter characters by
using an identifier that originally contains `$` (e.g., reference `Level$Str`
and show that you must query it as the quoted `"Level$Str"`), while keeping the
note that unquoted `Level_Str` becomes `level_str`.
In `@docs/references/kani-verification.md`:
- Line 366: The documentation shows an invalid Rust identifier parse$string;
replace it with a valid identifier like parse_string in the example and anywhere
else it appears (e.g., the list item that currently reads "parse_number,
parse$string") so readers see syntactically correct Rust function names; update
the text to use parse_string and ensure consistency with the surrounding
decomposition example.
In `@docs/references/notify-memchr-zstd.md`:
- Line 39: The example uses an invalid identifier `poll$interval`; replace it
with a valid Rust identifier (e.g., `poll_interval` or `poll_interval_duration`)
and update every occurrence so the line reads something like using
`poll_interval` in the expression that sets `should_poll` (refer to
`should_poll`, `something_changed`, and `self.last_poll.elapsed()` to locate the
usage), ensuring the chosen name follows Rust identifier rules (letters, digits,
and underscores) and matches any surrounding example variables or docs.
In `@docs/references/opentelemetry-otlp.md`:
- Line 35: Documentation shows an invalid method name `.with$interval()` on the
PeriodicReader builder; update the example to call the real OpenTelemetry SDK
method `.with_interval()` (from the opentelemetry_sdk PeriodicReader builder
API) so the snippet compiles and matches the crate's API.
In `@docs/references/production-patterns.md`:
- Around line 49-50: The search-and-replace accidentally renamed the Rust
ToString method `.to_string()` to `.to$string()`; restore all occurrences of
`.to$string()` back to the standard `.to_string()` so Rust code compiles and
type information is preserved, and ensure the replacement only targets the
intended typed-column pattern (not Rust methods) by restricting the regex or
scope that changed `.to_string()` in files like production-patterns.md and any
Rust source where `.to$string()` appears.
In `@docs/references/tokio-async-patterns.md`:
- Line 144: The text contains invalid Rust identifier syntax: replace the
incorrect token `poll$interval` with the valid identifier `poll_interval`
wherever it appears (e.g., the sentence "thread::sleep(poll$interval)" in
docs/references where `poll$interval` is used) so the example shows valid Rust
code; update all occurrences to `poll_interval` to match Rust naming conventions
and the analogous fix made in `notify-memchr-zstd.md`.
In `@docs/research/SCANNER_DESIGN_V1.md`:
- Around line 68-105: The fenced code block starting with "1. Bare column in
SELECT:" (containing examples like duration_ms → COALESCE(...),
int(duration_ms), float(duration_ms), EXCEPT, SELECT *, ORDER BY bare column,
AVG(duration_ms)) lacks a language specifier; update the opening fence from ```
to a language tag such as ```sql or ```text so the block is recognized as
SQL-like and renders/accessibility improves.
In `@docs/SCANNER_CONTRACT.md`:
- Line 18: The documentation incorrectly uses the `$` separator on a Rust
function name; update the sentence to use the correct Rust snake_case function
name `scan_into` (not `scan$into`) and clarify that the `$` separator is only
for logfwd typed column names like `level$str`, so rewrite the line to say "A
`debug_assert!` at the `scan_into` entry point fires in debug builds if..." and
optionally add a short note that `$` is for logfwd column names (e.g.,
`level$str`) not Rust identifiers.
- Line 104: Update the mistaken function name `parse$int_fast` in the docs to
the correct Rust snake_case name `parse_int_fast`; locate the occurrence
(currently written as `parse$int_fast`) and replace it with `parse_int_fast` so
the documentation matches the actual Rust function symbol and naming
conventions.
In `@README.md`:
- Line 89: Update the README sentence "All JSON fields are automatically
suffixed with their type (`$str`, `$int`, `$float`) to ensure schema stability."
to clarify that these type suffixes are applied to database/schema column names
(e.g., SQL columns) and not to the emitted JSON keys returned to clients;
replace or append the sentence to explicitly state "suffixes are added to
schema/column names for stability, but emitted JSON keys remain unchanged" so
readers don't think output JSON keys are renamed.
---
Outside diff comments:
In `@book/src/config/reference.md`:
- Around line 294-306: The doc text still references the old naming convention
`{field}_{type}` while the table (and current convention) uses `{field}$type`;
update the sentence that reads "The scanner maps each JSON field to one or more
typed Arrow columns following the `{field}_{type}` naming convention" to use
`{field}$type` (match the table and examples like `level$str`, `status$int`) so
the prose and examples are consistent with the `{field}$type` convention.
In `@crates/logfwd-bench/src/rss.rs`:
- Around line 113-126: The identifier after$streaming is invalid in Rust; rename
it to after_streaming everywhere it appears (the binding from rss_mb() and all
uses in the println! calls and the final savings calculation). Update the
variable declaration (currently let after$streaming = rss_mb();) to let
after_streaming = rss_mb(); and replace after$streaming - mid and any other
after$streaming references with after_streaming - mid so streaming_reported,
streaming_cols, streaming_scanner, and the savings arithmetic use the corrected
name.
In `@crates/logfwd-output/src/lib.rs`:
- Around line 201-241: The match on the parsed suffix currently downcasts the
Arrow array (variable arr from batch.column(idx)) based only on suffix (e.g.,
"int" -> as_primitive::<Int64Type>), which can panic if the real array
data_type() differs; update the logic in the function that serializes column
values so you first inspect arr.data_type() (or the ArrayRef's concrete type)
and only perform as_primitive::<...>() / as_any().downcast_ref<...>() when the
actual DataType matches the expected suffix, otherwise fall back to the string
branch (str_value) or emit null/skip the numeric branch; reference the existing
symbols arr, suffix, str_value, and batch.column(idx) to locate and guard each
numeric ("int", "float") branch by validating arr.data_type() before
downcasting.
- Around line 732-758: Add a new unit test mirroring
test_otlp_type_mismatch_no_panic but for JSON: create a RecordBatch with a Utf8
column named "count$int" (value "high"), call build_col_infos(&batch) and then
write_row_json(&batch, 0, &cols, &mut out), and assert the output UTF-8 JSON
contains the serialized string value (e.g. it contains "\"count\":\"high\"" or
at least "\"count\":") to ensure write_row_json falls back to string
serialization and does not panic.
- Around line 86-94: parse_column_name currently strips suffixes like "$int"
unconditionally and write_row_json then downcasts arrays
(arr.as_primitive::<Int64Type>() / as_primitive::<Float64Type>() / as_string)
based only on that suffix, which will panic on type mismatch; update
write_row_json to check the actual Arrow DataType (arr.data_type()) before
performing downcasts for the suffixes "int", "float", "str" and only call
arr.as_primitive::<Int64Type>() or arr.as_primitive::<Float64Type>() when the
DataType matches DataType::Int64 or DataType::Float64 respectively (and treat
non-matching cases as string/skip or fallback to Utf8 handling), otherwise fall
back to string handling or skip that field to avoid panics; reference the
functions parse_column_name and write_row_json and the downcast points where
arr.as_primitive::<Int64Type>() / arr.as_primitive::<Float64Type>() /
arr.as_string() are used.
In `@crates/logfwd-transform/src/rewriter.rs`:
- Around line 95-106: Update the doc comments in rewriter.rs to use the current
`$` suffix convention instead of the outdated `_` underscores: change examples
like `duration_ms_int`/`duration_ms_str` to `duration_ms$int`/`duration_ms$str`,
`level_str` to `level$str`, `status_int`/`status_float` to
`status$int`/`status$float`, and the `int(x)` example from
`COALESCE(duration_ms_int, TRY_CAST(duration_ms_str AS BIGINT))` to
`COALESCE(duration_ms$int, TRY_CAST(duration_ms$str AS BIGINT))` so the docs
match the implementation.
- Around line 419-422: Update the doc comment that currently uses the outdated
naming convention for generated columns in the "Rule 4 helpers" section (the
comment showing int(x) → COALESCE(x_int, TRY_CAST(x_str AS BIGINT))). Change the
example and any textual references to match the current naming scheme used by
the codebase (e.g., the actual generated column suffixes used by the rewriter
functions in this module), and ensure the comment near the helper that builds
the COALESCE expression reflects the current names (so the description and any
examples match the symbols produced by the helper).
In `@crates/logfwd-transform/tests/scanner_datafusion_boundary.rs`:
- Line 9: Update the doc comment that currently says "Column naming follows the
logfwd convention: `{field}_{type}`" to reflect the new naming convention used
by the code: change it to state `{field}$type` (e.g., update the comment in the
scanner_datafusion_boundary test module / the top-level doc comment that begins
"Column naming follows the logfwd convention" so it matches the actual column
naming used by the code).
In `@dev-docs/research/wide-crate-evaluation.md`:
- Around line 41-54: The function name find$structural_chars uses an invalid `$`
identifier; rename it to find_structural_chars and update every reference/call
site accordingly (including tests and any module exports) so the signature pub
fn find_structural_chars(block: &[u8; 64]) -> RawBlockMasks { ... } compiles;
keep the body as-is using RawBlockMasks and mask64 to locate newline, space,
quote, backslash, comma, colon, open_brace, close_brace, open_bracket,
close_bracket.
In `@docs/CONFIG_REFERENCE.md`:
- Around line 293-308: Update the "Column naming convention" description to
match the table's actual pattern: replace the inaccurate `{field}_{type}` phrase
with the `$`-suffix pattern (e.g., `{field}$type` or " `{field}$str`,
`{field}$int`, `{field}$float`") so the header text and examples like
`level$str` and `status$int` are consistent with the table and subsequent
explanation about mixed-type columns (`status$int` and `status$str`).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07563fc8-e28f-40bd-bf75-451748ab5ddc
📒 Files selected for processing (64)
.github/workflows/ai-explore-arrow-best-practices.yml.github/workflows/ai-explore-new-user.ymlAGENTS.mdCLAUDE.mdCLAUDE.mdCODE_STYLE.mdDEVELOPING.mdREADME.mdbench/dashboard/competitive.htmlbench/dashboard/index.htmlbench/dashboard/run.htmlbench/explore.shbook/src/README.mdbook/src/architecture/pipeline.mdbook/src/config/reference.mdbook/src/config/sql-transforms.mdbook/src/deployment/kubernetes.mdbook/src/deployment/monitoring.mdbook/src/development/contributing.mdcrates/logfwd-arrow/src/scanner.rscrates/logfwd-arrow/src/storage_builder.rscrates/logfwd-arrow/src/streaming_builder.rscrates/logfwd-bench/benches/pipeline.rscrates/logfwd-bench/src/explore.rscrates/logfwd-bench/src/rss.rscrates/logfwd-competitive-bench/src/agents/logfwd.rscrates/logfwd-core/tests/compliance_data.rscrates/logfwd-core/tests/scanner_conformance.rscrates/logfwd-output/src/lib.rscrates/logfwd-output/src/otlp_sink.rscrates/logfwd-output/src/stdout.rscrates/logfwd-transform/src/lib.rscrates/logfwd-transform/src/rewriter.rscrates/logfwd-transform/src/udf/grok.rscrates/logfwd-transform/src/udf/regexp_extract.rscrates/logfwd-transform/tests/scanner_datafusion_boundary.rscrates/logfwd/src/pipeline.rscrates/logfwd/tests/compliance.rscrates/logfwd/tests/integration.rsdev-docs/ARCHITECTURE.mddev-docs/DIRECTION.mddev-docs/PROVEN_CORE.mddev-docs/research/BOUNDARY_PATTERNS.mddev-docs/research/ENFORCEMENT_AND_MECHANICS.mddev-docs/research/KANI_LIMITS.mddev-docs/research/structural-index-research.mddev-docs/research/wide-crate-evaluation.mddocs/COLUMN_NAMING.mddocs/CONFIG_REFERENCE.mddocs/DEPLOYMENT.mddocs/PREDICATE_PUSHDOWN.mddocs/SCANNER_CONTRACT.mddocs/TROUBLESHOOTING.mddocs/references/arrow-pipeline-patterns.mddocs/references/arrow-v54.mddocs/references/datafusion-v45.mddocs/references/kani-verification.mddocs/references/notify-memchr-zstd.mddocs/references/opentelemetry-otlp.mddocs/references/production-patterns.mddocs/references/tokio-async-patterns.mddocs/research/BENCHMARKS_V1.mddocs/research/SCANNER_DESIGN_V1.mdtests/e2e/manifests/logfwd-config.yaml
| | `log_level` | string | `info` | Log verbosity. One of `error`, `warn`, `info`, `debug`, `trace`. | | ||
| | `metrics_endpoint` | string | none | OTLP endpoint for periodic metrics push, e.g. `http://otel-collector:4318`. | | ||
| | `metrics_interval_secs` | integer | `60` | Push interval for OTLP metrics in seconds. | | ||
| | `metrics$interval_secs` | integer | `60` | Push interval for OTLP metrics in seconds. | |
Contributor
There was a problem hiding this comment.
metrics$interval_secs looks incorrect for the server config key.
The runtime config struct still expects metrics_interval_secs (crates/logfwd-config/src/lib.rs, ServerConfig), so using the $ key from this doc example is ignored and the default interval (60s) is used.
Please keep the $ migration scoped to scanner output column names and leave config keys as metrics_interval_secs in docs/examples.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Addressed the review feedback with two commits:
I resolved all threads directly addressed by these changes and replied inline on the few intentionally skipped/nit threads. Validation
Note 🔒 Integrity filtering filtered 1 itemIntegrity filtering activated and filtered the following item during workflow execution.
What is this? | From workflow: AI: Address PR Review Feedback Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/research/SCANNER_DESIGN_V1.md (1)
135-136:⚠️ Potential issue | 🟡 MinorClarify
_rawas logical vs physical column name.This line uses
_raw(unsuffixed) in a section otherwise describing typed columns. If this refers to the physical Arrow column, it likely should be_raw$str; if logical, call that out explicitly to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/research/SCANNER_DESIGN_V1.md` around lines 135 - 136, The sentence about memcpy'ing the original JSON line for the `_raw` column is ambiguous about whether `_raw` is the logical field or the physical Arrow column name; update the text to explicitly state which is meant—if you mean the physical Arrow column, replace `_raw` with the physical name `_raw$str`, otherwise clarify by calling it the logical column (`_raw` logical) and note that the physical Arrow column is `_raw$str`; keep the rest of the passthrough/memcpy description and mention the physical name when describing the memcpy optimization.docs/CONFIG_REFERENCE.md (1)
296-296:⚠️ Potential issue | 🟠 MajorUpdate naming convention description to match the new
$separator.The description still references the old underscore-based pattern
{field}_{type}, but the table and all examples use{field}$str,{field}$int, etc.📝 Proposed fix
-The scanner maps each JSON field to one or more typed Arrow columns following the -`{field}_{type}` naming convention: +The scanner maps each JSON field to one or more typed Arrow columns following the +`{field}$type` naming convention:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CONFIG_REFERENCE.md` at line 296, Update the naming convention line that currently reads `{field}_{type}` to reflect the new `$` separator format (e.g., `{field}$str`, `{field}$int`) so the prose matches the table and examples; locate the occurrence of `{field}_{type}` in the docs and replace it with a description such as `{field}$<type>` (or explicitly show examples like `{field}$str`, `{field}$int`) and ensure any explanatory sentence refers to the `$` separator instead of underscore.book/src/config/reference.md (1)
296-296:⚠️ Potential issue | 🟠 MajorUpdate naming convention description to match the new
$separator.The description still references the old underscore-based pattern
{field}_{type}, but the table and all examples use{field}$str,{field}$int, etc.📝 Proposed fix
-The scanner maps each JSON field to one or more typed Arrow columns following the -`{field}_{type}` naming convention: +The scanner maps each JSON field to one or more typed Arrow columns following the +`{field}$type` naming convention:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@book/src/config/reference.md` at line 296, Update the naming convention sentence that currently reads `{field}_{type}` to use the new `$` separator—e.g., `{field}$str`, `{field}$int`—so the description matches the table and examples; search for the string `{field}_{type}` and replace it with `{field}$type` (or explicitly show examples `{field}$str`, `{field}$int`) in the reference text to ensure consistency with the rest of the document.
♻️ Duplicate comments (1)
crates/logfwd-output/src/stdout.rs (1)
102-104:⚠️ Potential issue | 🟡 MinorWell-known columns still use static index binding, not per-row resolution.
ts_idx,level_idx, andmsg_idxare bound once viafind_col()and used for all rows. Iftimestamp$stris found but null for a specific row whiletimestamp$inthas data, the timestamp won't display. This differs from the extras loop (lines 172-183) which correctly selects the first non-null variant per row.For consistency with the JSON output's first-non-null rule, consider resolving well-known fields from
ColInfoper row. However, if well-known fields are always consistently typed in practice, this may be acceptable.Also applies to: 112-127, 130-151, 154-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-output/src/stdout.rs` around lines 102 - 104, The code binds ts_idx, level_idx, and msg_idx once via find_col() and then uses those static indices for every row; change to resolve well-known columns per row using the same per-row first-non-null logic used in the extras loop (i.e., inspect the ColInfo entries for the variants ["timestamp$str","timestamp"], ["level$str","level"], ["message$str","message","msg$str","msg"] for the current row and pick the first non-null value), replacing uses of ts_idx/level_idx/msg_idx with per-row resolution; apply the same change to the other similar blocks noted (the ranges where well-known fields are bound once) to ensure consistent first-non-null selection across stdout formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@book/src/config/reference.md`:
- Line 296: Update the naming convention sentence that currently reads
`{field}_{type}` to use the new `$` separator—e.g., `{field}$str`,
`{field}$int`—so the description matches the table and examples; search for the
string `{field}_{type}` and replace it with `{field}$type` (or explicitly show
examples `{field}$str`, `{field}$int`) in the reference text to ensure
consistency with the rest of the document.
In `@docs/CONFIG_REFERENCE.md`:
- Line 296: Update the naming convention line that currently reads
`{field}_{type}` to reflect the new `$` separator format (e.g., `{field}$str`,
`{field}$int`) so the prose matches the table and examples; locate the
occurrence of `{field}_{type}` in the docs and replace it with a description
such as `{field}$<type>` (or explicitly show examples like `{field}$str`,
`{field}$int`) and ensure any explanatory sentence refers to the `$` separator
instead of underscore.
In `@docs/research/SCANNER_DESIGN_V1.md`:
- Around line 135-136: The sentence about memcpy'ing the original JSON line for
the `_raw` column is ambiguous about whether `_raw` is the logical field or the
physical Arrow column name; update the text to explicitly state which is
meant—if you mean the physical Arrow column, replace `_raw` with the physical
name `_raw$str`, otherwise clarify by calling it the logical column (`_raw`
logical) and note that the physical Arrow column is `_raw$str`; keep the rest of
the passthrough/memcpy description and mention the physical name when describing
the memcpy optimization.
---
Duplicate comments:
In `@crates/logfwd-output/src/stdout.rs`:
- Around line 102-104: The code binds ts_idx, level_idx, and msg_idx once via
find_col() and then uses those static indices for every row; change to resolve
well-known columns per row using the same per-row first-non-null logic used in
the extras loop (i.e., inspect the ColInfo entries for the variants
["timestamp$str","timestamp"], ["level$str","level"],
["message$str","message","msg$str","msg"] for the current row and pick the first
non-null value), replacing uses of ts_idx/level_idx/msg_idx with per-row
resolution; apply the same change to the other similar blocks noted (the ranges
where well-known fields are bound once) to ensure consistent first-non-null
selection across stdout formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7260f658-dc8e-4a2f-b440-46445d1ff487
📒 Files selected for processing (12)
README.mdbench/explore.shbook/src/config/reference.mdbook/src/deployment/kubernetes.mdcrates/logfwd-bench/src/rss.rscrates/logfwd-output/src/stdout.rsdev-docs/ARCHITECTURE.mddocs/CONFIG_REFERENCE.mddocs/DEPLOYMENT.mddocs/SCANNER_CONTRACT.mddocs/references/datafusion-v45.mddocs/research/SCANNER_DESIGN_V1.md
- Changed the internal type suffix separator from `_` to `$` (e.g., `field$int` instead of `field_int`) to prevent collisions with user-defined field names ending in `_str`, `_int`, or `_float`. - Redesigned JSON serialization in `logfwd-output` to group all type variants for a logical field and row-wise select the first non-null variant according to type priority (int > float > str). - Updated scanner builders, SQL rewriter, query analyzer, and output sinks to support the new convention. - Migrated all workspace tests, benchmarks, and E2E manifests to the new naming convention. - Updated `README.md` documentation. - Fixed regressions and unused variables to ensure CI passes. This fix prevents silent data loss and incorrect renaming of JSON fields when they share names with internal type-suffixed columns. Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Requesting changes due to two confirmed regressions in crates/logfwd-output/src/lib.rs introduced in this PR:
-
At/around line
212, non-finite floats are written directly (inf,-inf,NaN), producing invalid JSON instead of RFC-8259-compliant output. -
At/around line
226, control-character escaping for bytes< 0x20was removed, so raw control bytes can be emitted in JSON strings, also violating RFC-8259.
Please restore compliant handling (or equivalent), and add regression coverage for both cases.
What is this? | From workflow: AI: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/logfwd-bench/src/rss.rs (1)
113-126:⚠️ Potential issue | 🔴 CriticalFix invalid Rust identifier:
after$streamingcontains disallowed$character.Rust identifiers cannot contain
$. Replace withafter_streaming(1 definition at line 113 + 3 usages at lines 123, 125, 126).Fix
- let after$streaming = rss_mb(); + let after_streaming = rss_mb(); @@ - after$streaming - mid, streaming_reported, streaming_cols); + after_streaming - mid, streaming_reported, streaming_cols); @@ - (after_storage - before) - (after$streaming - mid), - 100.0 * (1.0 - (after$streaming - mid) / (after_storage - before))); + (after_storage - before) - (after_streaming - mid), + 100.0 * (1.0 - (after_streaming - mid) / (after_storage - before)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-bench/src/rss.rs` around lines 113 - 126, The identifier after$streaming uses an invalid '$' char; rename its definition and all usages to after_streaming to be valid Rust identifiers and update the three places where after$streaming is referenced (in the assignment and the println lines) so the variables streaming_cols, streaming_reported, batch, and streaming_scanner remain consistent; ensure you update the binding (let after_streaming = rss_mb();) and the three println occurrences to use after_streaming.crates/logfwd/src/pipeline.rs (1)
477-540:⚠️ Potential issue | 🟠 MajorRestore async dropped-batch accounting.
run()incrementsinc_scan_error()/inc_dropped_batch()on the same failure modes, butflush_batch()now only logs and returns. In async mode the batch is still dropped, yet the diagnostics stay clean.📉 Suggested metric parity fix
let batch = match tokio::task::block_in_place(|| self.scanner.scan(combined.into())) { Ok(b) => b, Err(e) => { + self.metrics.inc_scan_error(); + self.metrics.inc_dropped_batch(); eprintln!("pipeline: scan error (batch dropped): {e}"); return; } }; @@ let result = match self.transform.execute(batch).await { Ok(r) => r, Err(e) => { self.metrics.inc_transform_error(); + self.metrics.inc_dropped_batch(); eprintln!("pipeline: transform error (batch dropped): {e}"); return; } }; @@ if let Err(e) = tokio::task::block_in_place(|| self.output.send_batch(&result, &metadata)) { self.metrics.output_error(); + self.metrics.inc_dropped_batch(); eprintln!("pipeline: output error (batch dropped): {e}"); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd/src/pipeline.rs` around lines 477 - 540, The async flush_batch() currently logs and returns on scanner/transform/output failures but does not update the same metrics that run() increments; restore parity by incrementing the appropriate error counters and the dropped-batch counter in each failure branch: when tokio::task::block_in_place(|| self.scanner.scan(...)) returns Err call self.metrics.inc_scan_error() and self.metrics.inc_dropped_batch() before returning; when self.transform.execute(...) returns Err keep the existing self.metrics.inc_transform_error() and also call self.metrics.inc_dropped_batch(); when tokio::task::block_in_place(|| self.output.send_batch(...)) returns Err keep self.metrics.output_error() and also call self.metrics.inc_dropped_batch(); use the existing metric methods (inc_scan_error, inc_transform_error, output_error, inc_dropped_batch) and update the error branches in flush_batch() accordingly.
♻️ Duplicate comments (39)
DEVELOPING.md (2)
68-68:⚠️ Potential issue | 🔴 CriticalRevert to valid Rust identifier
scan_string.
scan$stringis not a valid Rust function name. The$separator applies to internal type-suffixed column names (e.g.,timestamp$int), not to function identifiers. The actual function in the codebase isscan_string.🔧 Proposed fix
-3. **Chunk-level classification** (`StructuralIndex`): one portable SIMD pass (via `wide` crate) over the entire buffer, detecting 10 structural characters simultaneously. Then `scan$string` is a single `trailing_zeros` bit-scan. +3. **Chunk-level classification** (`StructuralIndex`): one portable SIMD pass (via `wide` crate) over the entire buffer, detecting 10 structural characters simultaneously. Then `scan_string` is a single `trailing_zeros` bit-scan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEVELOPING.md` at line 68, The docs mention a non-idiomatic function name `scan$string`; update the text to use the actual Rust identifier `scan_string` (replace `scan$string` with `scan_string`) and clarify that `$` is only used for type-suffixed column names (e.g., `timestamp$int`), referencing the `scan_string` function name to match the codebase.
113-113:⚠️ Potential issue | 🔴 CriticalRevert to valid Rust identifier
scan_into().
scan$into()is not a valid Rust function name. The$separator is for column names, not function identifiers. The actual function isscan_into().🔧 Proposed fix
-Both implement `ScanBuilder` trait, sharing the generic `scan$into()` loop. +Both implement `ScanBuilder` trait, sharing the generic `scan_into()` loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEVELOPING.md` at line 113, The docs refer to an invalid Rust identifier `scan$into()`; update the text to use the correct function name `scan_into()` where the ScanBuilder trait and its shared generic loop are described (replace any occurrences of `scan$into()` with `scan_into()` in the DEVELOPING.md description of ScanBuilder).docs/references/notify-memchr-zstd.md (1)
39-39:⚠️ Potential issue | 🔴 CriticalInvalid Rust syntax:
$not allowed in identifiers.
poll$intervalis not a valid Rust identifier. Per Unicode XID rules,$(U+0024) has neither XID_Start nor XID_Continue property and cannot appear in Rust variable names. This example will fail to compile.The
$separator introduced in this PR is for data-layer naming (SQL columns likefield$int, JSON fields, Arrow schemas)—not Rust code identifiers. Use a standard Rust identifier here.Note: This was flagged in a previous review with web verification and marked "✅ Addressed in commits 2d687b4 to 57905a0" but the invalid syntax remains.
🔧 Proposed fix
-let should_poll = something_changed || self.last_poll.elapsed() >= poll$interval; +let should_poll = something_changed || self.last_poll.elapsed() >= poll_interval;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/notify-memchr-zstd.md` at line 39, The example uses an invalid Rust identifier `poll$interval`; replace it with a valid Rust identifier (e.g., `poll_interval`) everywhere it's declared and referenced (for example in the expression `let should_poll = something_changed || self.last_poll.elapsed() >= poll$interval;` and any struct field or parameter named `poll$interval`), and update any tests or docs that reference the old name; keep `$` usage only for data-layer names (SQL/JSON/Arrow) and not in Rust code.docs/references/opentelemetry-otlp.md (1)
35-35:⚠️ Potential issue | 🔴 CriticalFix invalid method name in OpenTelemetry example.
Line 35 shows
.with$interval()which does not exist in the OpenTelemetry SDK. The correct method is.with_interval()(underscore, not dollar sign). This documentation example will fail to compile for users following it.Fix
- .with$interval(Duration::from_secs(60)) + .with_interval(Duration::from_secs(60))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/opentelemetry-otlp.md` at line 35, The example uses a non-existent method `.with$interval(Duration::from_secs(60))`; replace that call with the correct OpenTelemetry SDK method name `.with_interval(Duration::from_secs(60))` so the example compiles—update the occurrence of `.with$interval` to `.with_interval` in the documentation snippet.crates/logfwd-bench/src/rss.rs (1)
33-33:⚠️ Potential issue | 🔴 CriticalRegression:
read_to_stringwas renamed into invalid Rust.Line 33 reintroduces
std::fs::read_to$string, which will not compile. The$separator should stay in typed column names, not standard-library API identifiers.Suggested fix
- if let Ok(status) = std::fs::read_to$string("/proc/self/status") { + if let Ok(status) = std::fs::read_to_string("/proc/self/status") {Verify this typo is gone from the file; expected result is no matches.
#!/bin/bash rg -n 'read_to\$string' crates/logfwd-bench/src/rss.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-bench/src/rss.rs` at line 33, The code uses a typo'd standard library call `std::fs::read_to$string` which won't compile; change the identifier to the correct `std::fs::read_to_string` in the `if let Ok(status) = std::fs::...` expression (referenced in the diff) and remove any stray `$` characters from that API name, then grep the file (`crates/logfwd-bench/src/rss.rs`) for `read_to\$string` to verify no occurrences remain.docs/references/arrow-v54.md (4)
509-509:⚠️ Potential issue | 🟡 MinorInvalid: variable name should not contain
$.
expected_stringsis a descriptive variable name.$is not valid in Rust identifiers.Proposed fix
-let mut builder = StringBuilder::with_capacity(expected$strings, expected_total_bytes); +let mut builder = StringBuilder::with_capacity(expected_strings, expected_total_bytes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/arrow-v54.md` at line 509, The identifier expected$strings is invalid in Rust; replace it with a valid name such as expected_strings wherever it is declared and used (e.g., the call to StringBuilder::with_capacity(expected$strings, expected_total_bytes)), updating the variable declaration and all references to match the new snake_case name.
150-151:⚠️ Potential issue | 🟠 MajorInvalid:
with_deduplicate_strings()is an Arrow API method.This is a real Arrow
StringViewBuildermethod, not a logfwd column name. The$separator only applies to logfwd's typed column naming.Proposed fix
let mut builder = StringViewBuilder::new() - .with_deduplicate$strings(); // uses ahash internally + .with_deduplicate_strings(); // uses ahash internally🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/arrow-v54.md` around lines 150 - 151, The doc shows an incorrect use of the `$` separator in the Arrow API example; replace the logfwd-style name with the actual Arrow method call by using StringViewBuilder::new().with_deduplicate_strings() (i.e., remove the `$` and ensure the method name is exactly with_deduplicate_strings) so the example reflects the real Arrow StringViewBuilder API.
460-460:⚠️ Potential issue | 🟠 MajorInvalid:
as_string()is an ArrowAsArraytrait method.This is a real Arrow API method, not a logfwd column name.
Proposed fix
// Correct downcast -let strings: &StringArray = arr.as$string::<i32>(); +let strings: &StringArray = arr.as_string::<i32>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/arrow-v54.md` at line 460, The code mistakenly uses a non-existent `as$string::<i32>()` invocation (looks like a logfwd column name) instead of the Arrow AsArray trait method; replace that call with the Arrow method `as_string()` on `arr` and ensure the result is typed as `&StringArray` (use the `AsArray::as_string` method on `arr`), so change the reference to call `arr.as_string()` and bind it to a `&StringArray`.
132-137:⚠️ Potential issue | 🟡 MinorExample string data incorrectly modified.
The byte string literal is just example data demonstrating
StringViewArray— it's unrelated to logfwd column naming. The$change here is unnecessary and confusing.Proposed fix
-let raw_bytes: &[u8] = b"helloworld_this_is_a_long$string"; +let raw_bytes: &[u8] = b"helloworld_this_is_a_long_string"; let block = builder.append_block(Buffer::from(raw_bytes)); // Point views into the existing buffer -- no copy of string data builder.try_append_view(block, 0, 5).unwrap(); // "hello" -builder.try_append_view(block, 5, 27).unwrap(); // "world_this_is_a_long$string" +builder.try_append_view(block, 5, 27).unwrap(); // "world_this_is_a_long_string"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/arrow-v54.md` around lines 132 - 137, The example byte string literal raw_bytes contains an unnecessary '$' character that confuses the example; restore the original example data (remove the '$' from the literal passed to Buffer::from in raw_bytes) and update the corresponding view length/end offset in builder.try_append_view(block, 5, 27) so the slice bounds match the new byte length (adjust the 27 to the correct end index), leaving builder.append_block and try_append_view usage unchanged.docs/references/tokio-async-patterns.md (1)
144-144:⚠️ Potential issue | 🟡 MinorInvalid Rust syntax:
$not allowed in identifiers.
poll$intervalis not a valid Rust identifier. The$character cannot appear in Rust variable names. This should bepoll_interval.Note: The
$separator convention from this PR applies only to logfwd's typed Arrow column names (e.g.,field$int,field$str), not to Rust code identifiers.Proposed fix
-This replaces the current `thread::sleep(poll$interval)` polling pattern. +This replaces the current `thread::sleep(poll_interval)` polling pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/tokio-async-patterns.md` at line 144, The text contains invalid Rust identifier syntax: `poll$interval` used in the example (e.g., `thread::sleep(poll$interval)`)—replace it with a valid Rust identifier such as `poll_interval` throughout the example so the code reads `thread::sleep(poll_interval)` and any mentions of `poll$interval` are updated to `poll_interval`; ensure this change only affects Rust code snippets (leave logfwd typed Arrow column names like `field$int` unchanged).docs/research/SCANNER_DESIGN_V1.md (1)
68-105: 🧹 Nitpick | 🔵 TrivialAdd language specifier to fenced code block.
Static analysis (MD040) flags this code block as missing a language specifier. Since this contains SQL rewrite examples, use
textorsqlfor better rendering.Proposed fix
-``` +```text 1. Bare column in SELECT: duration_ms → COALESCE(CAST(duration_ms$int AS VARCHAR),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/research/SCANNER_DESIGN_V1.md` around lines 68 - 105, The fenced code block containing the SQL rewrite examples (the block starting with "1. Bare column in SELECT:" and showing transformations for duration_ms, level, status, int(), float(), EXCEPT, SELECT *, ORDER BY, and aggregation) is missing a language specifier; update the opening fence from ``` to a language-tagged fence such as ```text or ```sql so the block is properly recognized (e.g., replace the opening ``` with ```text or ```sql and leave the inner content unchanged).book/src/development/contributing.md (2)
47-47:⚠️ Potential issue | 🟡 MinorInvalid Rust identifier:
scan$string.
$cannot appear in Rust identifiers. This references a real function that should bescan_string.Proposed fix
-3. **Chunk-level classification** (`ChunkIndex`): one NEON/SSE pass over the entire buffer at ~16 GiB/s, pre-computes all quote positions. Then `scan$string` is a single `trailing_zeros` bit-scan. +3. **Chunk-level classification** (`ChunkIndex`): one NEON/SSE pass over the entire buffer at ~16 GiB/s, pre-computes all quote positions. Then `scan_string` is a single `trailing_zeros` bit-scan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@book/src/development/contributing.md` at line 47, The identifier `scan$string` is invalid in Rust and should be renamed to `scan_string`; update the documentation and any references (e.g., the paragraph mentioning `ChunkIndex` and the single `trailing_zeros` bit-scan) to use the correct function name `scan_string` so the text matches the real Rust function symbol.
92-92:⚠️ Potential issue | 🟡 MinorInvalid Rust identifier:
scan$into().Same issue —
$is not valid in Rust function names. Should bescan_into().Proposed fix
-Both implement `ScanBuilder` trait, sharing the generic `scan$into()` loop. +Both implement `ScanBuilder` trait, sharing the generic `scan_into()` loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@book/src/development/contributing.md` at line 92, The doc references an invalid Rust identifier `scan$into()` in the description of the ScanBuilder trait; update the text to use the correct Rust function name `scan_into()` everywhere (e.g., in the sentence "Both implement `ScanBuilder` trait, sharing the generic `scan$into()` loop." change `scan$into()` to `scan_into()`) so the documented symbol matches the actual method name used by the ScanBuilder implementations.docs/references/kani-verification.md (1)
366-367:⚠️ Potential issue | 🟡 MinorInvalid Rust syntax:
$not allowed in function identifiers.
parse$stringis not valid Rust. The$separator convention applies only to logfwd's typed Arrow column names (e.g.,field$int), not to Rust function names in documentation examples.Proposed fix
-1. **Decompose** into small, pure functions (e.g., `parse_number`, `parse$string`, +1. **Decompose** into small, pure functions (e.g., `parse_number`, `parse_string`, `encode_field`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/kani-verification.md` around lines 366 - 367, The docs show an invalid Rust function name `parse$string`; update the example to use a valid identifier (e.g., `parse_string`) and keep `parse_number` and `encode_field` as-is, and clarify that the `$` separator is only for logfwd typed Arrow column names like `field$int` (not for Rust function names) so readers won't confuse the column-naming convention with Rust identifiers.crates/logfwd-output/src/lib.rs (2)
202-216:⚠️ Potential issue | 🔴 CriticalStop using the parsed suffix to choose the JSON value type.
When a numeric column has no
$int/$floatsuffix, this path falls intostr_value()and emits""instead of a number;COUNT(*) AS cntis the easy reproducer. The same suffix-driven dispatch also panics if a string column is aliased ascost$int. Encode from the selected column’s actual type after variant selection, and add a regression for an untyped aggregate column.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd-output/src/lib.rs` around lines 202 - 216, The code currently uses the parsed `suffix` to decide JSON encoding in the match block (the branch handling "int" / "float" vs default calling `str_value`), which causes numeric columns without suffixes to be emitted as strings and panics when a string column is aliased like `cost$int`; change the logic to inspect the selected column's actual Arrow data type (e.g., use the `arr`/`as_*` variant or the array’s data_type()) after variant selection and encode values accordingly (write integers/floats without quotes, strings via `str_value`), remove reliance on `suffix` for type dispatch in this match, and add a regression test covering an untyped aggregate column (e.g., `COUNT(*) AS cnt`) to ensure numbers are emitted as JSON numbers.
87-90:⚠️ Potential issue | 🔴 CriticalDon't strip
$int/$float/$strfrom every column name.
parse_column_name()still rewrites any literal alias ending in those suffixes. A user field likecost$intis still serialized ascost, can collide with a realcost, and can be deduplicated/dropped again. This needs provenance or reversible escaping for scanner-generated names, not unconditional suffix parsing. Please add a regression that preserves a literalcost$intfield.book/src/deployment/monitoring.md (1)
34-34:⚠️ Potential issue | 🟡 MinorKeep this config key snake_case.
metrics$interval_secsreads like an internal typed-field name, not a config key. Users copying this example will end up with an unknown setting; keepmetrics_interval_secs.As per coding guidelines "Config fields should use snake_case matching YAML keys".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@book/src/deployment/monitoring.md` at line 34, The config key uses an invalid/internal-style name "metrics$interval_secs"; change this to snake_case "metrics_interval_secs" so the example YAML matches expected config keys and users won't get an unknown setting—update the key name wherever "metrics$interval_secs" appears (e.g., example YAML snippet) to "metrics_interval_secs".CODE_STYLE.md (1)
28-28:⚠️ Potential issue | 🟡 MinorFix the hot-path rule to reference
.to_string(), not.to$string().The current token is not a valid Rust method and makes the rule misleading.
Proposed fix
-- **No `format!()` or `.to$string()` in loops.** +- **No `format!()` or `.to_string()` in loops.**#!/bin/bash # Confirm style docs do not mention the invalid method token. rg -n --type md 'to\$string\(' CODE_STYLE.md docs/references/production-patterns.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CODE_STYLE.md` at line 28, Update the hot-path rule text in CODE_STYLE.md to reference the correct Rust method `.to_string()` instead of the invalid token `.to$string()`; locate the bullet mentioning "No `format!()` or `.to$string()` in loops" and replace `.to$string()` with `.to_string()` so the rule correctly warns against using `format!()` or `.to_string()` in loops.docs/SCANNER_CONTRACT.md (1)
18-18:⚠️ Potential issue | 🟠 MajorUse Rust snake_case symbols in the contract (
scan_into,parse_int_fast).At Line 18 and Line 104,
$was applied to function names; that convention is only for typed column names.Proposed fix
-- A `debug_assert!` at the `scan$into` entry point fires in debug builds if +- A `debug_assert!` at the `scan_into` entry point fires in debug builds if -- - A value without either is first tried as `Int64` via `parse$int_fast`. On +- - A value without either is first tried as `Int64` via `parse_int_fast`. On#!/bin/bash # Validate doc symbol names for scanner entry points/helpers. rg -n --type md 'scan\$into|parse\$int_fast' docs/SCANNER_CONTRACT.md # Optional cross-check for real Rust symbols in repository: rg -nP --type rust '\bscan_into\b|\bparse_int_fast\b'Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/SCANNER_CONTRACT.md` at line 18, The docs use a `$`-style symbol for function names which is wrong for Rust identifiers; replace occurrences of `scan$into` and `parse$int_fast` with proper Rust snake_case `scan_into` and `parse_int_fast` in SCANNER_CONTRACT.md (search for the tokens `scan$into` and `parse$int_fast` and update them to `scan_into` and `parse_int_fast`), and run the provided ripgrep checks to verify the document now matches Rust symbol usage.docs/references/production-patterns.md (1)
49-49:⚠️ Potential issue | 🟡 MinorRestore the Rust method name to
.to_string().
$should only be used for typed column suffixes (e.g.,field$str), not Rust method names.Proposed fix
-- Don't erase error types with `.to$string()` — preserve the original +- Don't erase error types with `.to_string()` — preserve the original#!/bin/bash # Verify invalid method token is gone from docs. rg -n --type md 'to\$string\(' docs/references/production-patterns.md CODE_STYLE.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/production-patterns.md` at line 49, The docs contain an incorrect Rust method token ".to$string()" that erases the original error type and misuses "$"; restore the correct Rust method name ".to_string()" everywhere (e.g., replace occurrences of ".to$string(" with ".to_string(") and ensure "$" remains reserved only for typed column suffixes like "field$str"); update the sentence and any examples referencing ".to$string()" to use ".to_string()" so the documentation accurately reflects Rust method names.dev-docs/research/structural-index-research.md (2)
10-10:⚠️ Potential issue | 🟡 MinorUse
platform_intrinsicsfor Kani internals path.At Line 10,
platform$intrinsicsis incorrect terminology.Proposed fix
-LLVM-specific lowering, not Kani's `platform$intrinsics` path). +LLVM-specific lowering, not Kani's `platform_intrinsics` path).#!/bin/bash # Verify wrong token is removed. rg -n --type md 'platform\$intrinsics|platform_intrinsics' dev-docs/research/structural-index-research.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/research/structural-index-research.md` at line 10, Replace the incorrect Kani internals token platform$intrinsics with the correct platform_intrinsics everywhere in the doc; locate occurrences (e.g., the phrase at Line 10 in structural-index-research.md and any other matches) and update the text to use platform_intrinsics (preserve surrounding punctuation/backticks if present) and then verify no residual platform$intrinsics tokens remain.
91-91:⚠️ Potential issue | 🟠 MajorKeep pseudocode identifiers in snake_case (
in_string,carry_string,find_structural_chars).These are variable/function names, not typed column names, so
$should not be used.Proposed fix
- CriParser::extract(buf, &space_bitmask, &in$string) → field ranges + CriParser::extract(buf, &space_bitmask, &in_string) → field ranges -| Quote parity (in$string) | YES | Opening quote means next block starts in string | +| Quote parity (in_string) | YES | Opening quote means next block starts in string | - let raw = find$structural_chars(block); + let raw = find_structural_chars(block); - // Stage 3: string interior (needs prev_in$string carry) - let in$string = prefix_xor(real_q) ^ carry$string; - carry$string = ...; + // Stage 3: string interior (needs prev_in_string carry) + let in_string = prefix_xor(real_q) ^ carry_string; + carry_string = ...; - let sp = raw.space & !in$string; - let comma = raw.comma & !in$string; + let sp = raw.space & !in_string; + let comma = raw.comma & !in_string; - // Nothing stored! Just carry_bs and carry$string to next block. + // Nothing stored! Just carry_bs and carry_string to next block. -lookups (`next_quote(pos)`, `is_in$string(pos)`). Streaming eliminates +lookups (`next_quote(pos)`, `is_in_string(pos)`). Streaming eliminates#!/bin/bash # Check for invalid `$` pseudocode identifiers in this doc. rg -n --type md 'in\$string|carry\$string|find\$structural_chars|is_in\$string' dev-docs/research/structural-index-research.mdAlso applies to: 118-118, 131-131, 136-138, 142-143, 146-146, 155-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/research/structural-index-research.md` at line 91, Replace pseudocode identifiers that mistakenly include '$' with snake_case names (e.g., change in$string → in_string, carry$string → carry_string, find$structural_chars → find_structural_chars, is_in$string → is_in_string) throughout the doc; update every occurrence in the pseudocode examples and prose (references at lines mentioning CriParser::extract and the other listed occurrences) so variable/function names use underscores not dollar signs to match the intended pseudocode style.dev-docs/research/wide-crate-evaluation.md (1)
41-41:⚠️ Potential issue | 🟠 MajorRevert
$-style function names to valid Rust identifiers.
find$structural_chars*is not valid Rust naming and does not match real symbols.Proposed fix
-pub fn find$structural_chars(block: &[u8; 64]) -> RawBlockMasks { +pub fn find_structural_chars(block: &[u8; 64]) -> RawBlockMasks { -1. **Kani proves `find$structural_chars_scalar`** (pure Rust, no SIMD) -2. **proptest proves `find$structural_chars` ≡ `_scalar`** (SIMD output +1. **Kani proves `find_structural_chars_scalar`** (pure Rust, no SIMD) +2. **proptest proves `find_structural_chars` ≡ `_scalar`** (SIMD output - fn find$structural_chars() — 15 lines, #[cfg] dispatch + fn find_structural_chars() — 15 lines, #[cfg] dispatch - fn find$structural_chars() — 15 lines, safe, all platforms + fn find_structural_chars() — 15 lines, safe, all platforms#!/bin/bash # Detect invalid `$` Rust-like function identifiers in this doc. rg -n --type md 'find\$structural_chars(_scalar)?' dev-docs/research/wide-crate-evaluation.md # Cross-check repository Rust symbols. rg -nP --type rust '\bfind_structural_chars(_scalar)?\b'Also applies to: 155-156, 183-183, 195-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/research/wide-crate-evaluation.md` at line 41, Replace the invalid `$`-style Rust identifier find$structural_chars (and any variants like find$structural_chars_scalar) with valid Rust names used in the codebase—e.g., rename to find_structural_chars and find_structural_chars_scalar so the doc matches real symbols; update all occurrences in the markdown (including the examples at the other noted lines) to use the underscored identifiers and ensure they exactly match the function names present in the repository.README.md (1)
89-89:⚠️ Potential issue | 🟡 MinorClarify that suffixing applies to SQL/schema columns, not emitted JSON keys.
Current wording at Line 89 is ambiguous and can be interpreted as renaming output JSON fields.
Suggested wording
-All JSON fields are automatically suffixed with their type (`$str`, `$int`, `$float`) to ensure schema stability. +In SQL transforms, fields are exposed as typed columns with suffixes (`$str`, `$int`, `$float`) to keep internal schema stable; emitted JSON keys remain unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 89, The sentence "All JSON fields are automatically suffixed with their type (`$str`, `$int`, `$float`) to ensure schema stability." is ambiguous; update the README wording to clarify that the suffixing applies to database/schema column names (e.g., SQL table columns) and NOT to the JSON keys emitted to users. Replace or rewrite that line to explicitly state: schema/column names are suffixed for stability while output JSON keys remain unchanged, and include an example if helpful (e.g., column_name$str -> JSON key: column_name). Ensure you update the exact string mentioned so readers editing README.md see the clarified intent.docs/DEPLOYMENT.md (1)
108-108:⚠️ Potential issue | 🟡 MinorTypo:
$stream$strshould be_stream$str.Special columns use underscore prefix (
_raw,_file,_stream,_time), with$only for the type suffix.Suggested fix
_time_ns$int, - $stream$str + _stream$str FROM logs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/DEPLOYMENT.md` at line 108, The docs contain a typo: the special column name is written as `$stream$str` but should use the underscore prefix; update the occurrence of `$stream$str` to `_stream$str` (consistent with the other special columns `_raw`, `_file`, `_stream`, `_time`) so the documentation reflects the correct column naming convention.docs/references/datafusion-v45.md (4)
382-382:⚠️ Potential issue | 🟡 MinorInvalid Rust syntax:
$not allowed in method names.
sql.to$string()is invalid Rust. The$character is reserved for macro metavariables.Suggested fix
- Ok(Self { ctx, rt, user_sql: sql.to$string() }) + Ok(Self { ctx, rt, user_sql: sql.to_string() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/datafusion-v45.md` at line 382, The code uses an invalid method name `sql.to$string()` when constructing Self (setting user_sql), which uses a `$` from macro syntax; replace that call with the proper Rust string conversion method `sql.to_string()` so the return line becomes Ok(Self { ctx, rt, user_sql: sql.to_string() }) ensuring the user_sql field receives a String.
41-41:⚠️ Potential issue | 🟡 MinorIncorrect Serde method names: use underscores, not dollar signs.
Serde's Visitor provides
visit_str(&str),visit_borrowed_str(&'de str), andvisit_string(String)— not$variants.Suggested fix
-Serde's zero-copy mechanism also maps perfectly to logfwd. The `Deserializer<'de>` lifetime parameter lets borrowed slices flow through the boundary. The Visitor trait provides three string methods — `visit$str(&str)` (transient), `visit_borrowed$str(&'de str)` (zero-copy from input), and `visit$string(String)` (owned). **logfwd's kernel already uses `&'a [u8]` slices — these can flow directly through a visitor trait boundary with no copy**, as long as the visitor's lifetime is tied to the input buffer. +Serde's zero-copy mechanism also maps perfectly to logfwd. The `Deserializer<'de>` lifetime parameter lets borrowed slices flow through the boundary. The Visitor trait provides three string methods — `visit_str(&str)` (transient), `visit_borrowed_str(&'de str)` (zero-copy from input), and `visit_string(String)` (owned). **logfwd's kernel already uses `&'a [u8]` slices — these can flow directly through a visitor trait boundary with no copy**, as long as the visitor's lifetime is tied to the input buffer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/datafusion-v45.md` at line 41, The Serde Visitor implementation uses incorrect method names with dollar-sign variants; update the Visitor methods to use the correct names visit_str(&str), visit_borrowed_str(&'de str) and visit_string(String) instead of the $-variants wherever the Visitor is implemented (search for implementations referencing Visitor, visit$*, or the struct/type used as the custom deserializer) so that the methods match Serde's trait signatures.
7-7:⚠️ Potential issue | 🟡 MinorIncorrect Serde API reference:
visit$strshould bevisit_str.This line discusses Serde's actual API, not logfwd's naming convention. Serde's Visitor trait uses
visit_str,visit_i64,visit_map— underscores, not dollar signs.Suggested fix
-Serde defines a fixed set of **29 kernel types** (bool, i8–i128, u8–u128, f32, f64, char, string, bytes, option, unit, seq, map, struct, and their variants) that constitute its data model. The critical insight, noted by Josh McGuigan's source analysis, is that **this data model never materializes as an enum or struct**. It exists purely as trait method signatures — `visit_i64`, `visit$str`, `visit_map` — and data flows directly from format bytes through the Deserializer into the Visitor with no intermediate allocation. +Serde defines a fixed set of **29 kernel types** (bool, i8–i128, u8–u128, f32, f64, char, string, bytes, option, unit, seq, map, struct, and their variants) that constitute its data model. The critical insight, noted by Josh McGuigan's source analysis, is that **this data model never materializes as an enum or struct**. It exists purely as trait method signatures — `visit_i64`, `visit_str`, `visit_map` — and data flows directly from format bytes through the Deserializer into the Visitor with no intermediate allocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/datafusion-v45.md` at line 7, The docs currently reference Serde's Visitor methods using an incorrect token `visit$str`; update the text to use the correct Serde API names (e.g., replace `visit$str` with `visit_str`) and ensure other examples use the proper underscore forms like `visit_i64` and `visit_map` instead of any dollar-sign variants so the documentation matches Serde's Visitor trait (search for occurrences of `visit$str`, `visit$i64`, etc., and correct to `visit_str`, `visit_i64`, `visit_map`).
307-309:⚠️ Potential issue | 🟡 MinorCase-folding example is incorrect.
SELECT Level_Str FROM logsresolves tolevel_str(lowercase), notlevel$str. DataFusion lowercases unquoted identifiers but doesn't introduce$. To demonstrate$preservation, use an identifier that originally contains$.Suggested fix
-`SELECT Level_Str FROM logs` resolves to column `level$str`. Column names in Arrow +`SELECT Level$Str FROM logs` resolves to column `level$str`. Column names in Arrow schemas are **case-sensitive**, but DataFusion's SQL planner lowercases unquoted -identifiers. If your schema has `Level_Str`, query it as `"Level_Str"` (quoted). +identifiers. If your schema has `Level$Str`, query it as `"Level$Str"` (quoted).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/datafusion-v45.md` around lines 307 - 309, The doc example incorrectly states that SELECT Level_Str FROM logs resolves to column level$str; update it to say it resolves to level_str (DataFusion lowercases unquoted identifiers and does not inject `$`), and clarify that to match a schema field named Level_Str you must query it as "Level_Str" (quoted). Also add or replace the `$`-preservation example with one that uses an identifier that actually contains `$` (e.g., an original schema field with `$` in its name) to demonstrate that quoted vs unquoted behavior preserves existing `$` characters rather than introducing them.dev-docs/research/BOUNDARY_PATTERNS.md (3)
41-41:⚠️ Potential issue | 🟡 MinorSerde method names:
visit_str,visit_borrowed_str,visit_string.These are Serde's actual API, not logfwd's internal convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/research/BOUNDARY_PATTERNS.md` at line 41, The documentation uses incorrect Serde method names; replace the non-API names with Serde's real Visitor methods: use visit_str, visit_borrowed_str, and visit_string in the sentence that references the Visitor trait and clarify that these are Serde methods (not logfwd conventions); also ensure the surrounding text still references Deserializer<'de>, Visitor trait, and logfwd's kernel using &'a [u8] slices to explain zero-copy flow through the visitor lifetime.
7-7:⚠️ Potential issue | 🟡 MinorSerde API reference should use
visit_str, notvisit$str.This references Serde's actual trait method names, not logfwd's naming.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/research/BOUNDARY_PATTERNS.md` at line 7, The doc incorrectly references a Serde method as `visit$str`; update the text to use the correct Serde trait method name `visit_str` (preserving surrounding context that lists `visit_i64`, `visit_str`, `visit_map`) so the API reference matches actual Serde method names and avoids confusion with logfwd-specific identifiers.
26-28: 🧹 Nitpick | 🔵 TrivialNote:
$in function/method names is invalid Rust syntax.For illustrative pseudocode in a research doc, this may be acceptable. However, if this code is ever copied or referenced, the
$characters in identifiers likevisit$str_field,parse_line$into,typed_value$strategywould fail to compile.Also applies to: 71-71, 93-93, 172-172, 185-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/research/BOUNDARY_PATTERNS.md` around lines 26 - 28, The doc contains invalid Rust identifiers using '$' (e.g., visit$str_field, parse_line$into, typed_value$strategy) which won't compile if copied; replace each '$' in those pseudocode names with a valid character (commonly '_' ) so visit$str_field -> visit_str_field, parse_line$into -> parse_line_into, typed_value$strategy -> typed_value_strategy, and update all other occurrences (notably the instances at the other reported locations) to use the corrected identifiers so the examples are valid Rust identifiers.bench/explore.sh (1)
193-206:⚠️ Potential issue | 🔴 CriticalShell variable expansion will mangle SQL queries.
In double-quoted strings,
$str,$int,$floatare expanded as shell variables (undefined = empty). E.g.,"timestamp$str"becomes"timestamp".Option 1: Escape dollar signs
QUERIES=( "SELECT * FROM logs" - "SELECT timestamp$str, level$str, message$str FROM logs" - "SELECT * FROM logs WHERE level$str = 'ERROR'" - "SELECT * FROM logs WHERE level$str IN ('ERROR', 'WARN')" - "SELECT * FROM logs WHERE duration_ms$int > 100" - "SELECT * FROM logs WHERE level$str = 'ERROR' AND duration_ms$int > 200" - "SELECT level$str, COUNT(*) as cnt FROM logs GROUP BY level$str" - "SELECT service$str, level$str, COUNT(*) FROM logs GROUP BY service$str, level$str" - "SELECT level$str, AVG(duration_ms$int) as avg_dur, MAX(duration_ms$int) as max_dur FROM logs GROUP BY level$str" - "SELECT *, CASE WHEN duration_ms$int > 200 THEN 'slow' ELSE 'fast' END as speed$str FROM logs" - "SELECT * FROM logs WHERE message$str LIKE '%users%'" - "SELECT * FROM logs ORDER BY duration_ms$int DESC LIMIT 100" + "SELECT timestamp\$str, level\$str, message\$str FROM logs" + "SELECT * FROM logs WHERE level\$str = 'ERROR'" + "SELECT * FROM logs WHERE level\$str IN ('ERROR', 'WARN')" + "SELECT * FROM logs WHERE duration_ms\$int > 100" + "SELECT * FROM logs WHERE level\$str = 'ERROR' AND duration_ms\$int > 200" + "SELECT level\$str, COUNT(*) as cnt FROM logs GROUP BY level\$str" + "SELECT service\$str, level\$str, COUNT(*) FROM logs GROUP BY service\$str, level\$str" + "SELECT level\$str, AVG(duration_ms\$int) as avg_dur, MAX(duration_ms\$int) as max_dur FROM logs GROUP BY level\$str" + "SELECT *, CASE WHEN duration_ms\$int > 200 THEN 'slow' ELSE 'fast' END as speed\$str FROM logs" + "SELECT * FROM logs WHERE message\$str LIKE '%users%'" + "SELECT * FROM logs ORDER BY duration_ms\$int DESC LIMIT 100" )The same fix is needed for lines 223-225, 231-235, and 239-242.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bench/explore.sh` around lines 193 - 206, The QUERIES array contains SQL strings with $str/$int/$float which are being expanded by the shell; fix by preventing shell expansion either by switching those entries to single-quoted strings or escaping each dollar sign (e.g., \$str) so the literal column markers remain in the SQL; update all affected entries in QUERIES (and the other occurrences you noted around lines 223-225, 231-235, 239-242) to use one consistent approach so the SQL remains intact.book/src/config/reference.md (1)
425-432:⚠️ Potential issue | 🟠 MajorKeep the YAML key snake_case.
metrics$interval_secsapplies the typed-column convention to configuration. Users copying this example will likely configure a key that does not bind, because the server setting should remainmetrics_interval_secs.As per coding guidelines,
Config fields should use snake_case matching YAML keys.📝 Suggested doc fix
-| `metrics$interval_secs` | integer | `60` | Push interval for OTLP metrics in seconds. | +| `metrics_interval_secs` | integer | `60` | Push interval for OTLP metrics in seconds. | - metrics$interval_secs: 30 + metrics_interval_secs: 30 - metrics$interval_secs: 60 + metrics_interval_secs: 60Also applies to: 517-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@book/src/config/reference.md` around lines 425 - 432, The example uses the incorrect typed-column key `metrics$interval_secs`; change it to the snake_case server config key `metrics_interval_secs` in the YAML example (and any other occurrences, e.g., the duplicate at line 517) so the documented setting matches the actual config field name; update the example under the server block where `metrics$interval_secs` appears to `metrics_interval_secs`.dev-docs/research/ENFORCEMENT_AND_MECHANICS.md (1)
116-121:⚠️ Potential issue | 🟠 MajorKeep
$out of Rust identifiers in these examples.
parse$int_fast,verify_parse$int_fast, andparse$float_fastare invalid Rust identifiers. The$separator belongs in generated column names, not in Rust symbols, so these snippets now document code that cannot compile.Also applies to: 310-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/research/ENFORCEMENT_AND_MECHANICS.md` around lines 116 - 121, The examples use invalid Rust identifiers containing `$` (e.g., parse$int_fast, verify_parse$int_fast, parse$float_fast); rename these symbols to valid Rust identifiers (for example parse_int_fast, verify_parse_int_fast, parse_float_fast) and update all references and the kani proof attribute (e.g., #[kani::proof_for_contract(parse_int_fast)]) so the examples compile; apply the same replacement to the other occurrence noted in the comment.dev-docs/ARCHITECTURE.md (1)
103-106:⚠️ Potential issue | 🟠 MajorKeep internal Rust identifiers underscore-based in the docs.
in$string,scan$string,scan$into, andappend$str_by_idx/append$int_by_idx/append$float_by_idxare not valid Rust. These are internal symbols, not column names, so switching them to$makes the architecture doc inaccurate.Also applies to: 127-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/ARCHITECTURE.md` around lines 103 - 106, The doc uses dollar-sign variants of internal Rust identifiers (e.g., in$string, scan$string, scan$into, append$str_by_idx, append$int_by_idx, append$float_by_idx) which are invalid Rust and misleading; update the architecture doc to use the underscore-based internal symbols instead (in_string, scan_string, scan_into, append_str_by_idx, append_int_by_idx, append_float_by_idx) wherever they appear (including the other noted range 127–138) so the documentation matches the actual Rust identifiers.dev-docs/PROVEN_CORE.md (1)
27-27:⚠️ Potential issue | 🟠 MajorThese documented Rust symbols are invalid.
parse$int_fast,parse$float_fast,field$str,field$int, andfield$floatare Rust identifiers, not output column names. Renaming them to$forms makes this design doc syntactically wrong and will mislead future implementations.Also applies to: 58-58, 132-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/PROVEN_CORE.md` at line 27, The doc mistakenly shows Rust identifiers as output-column "$-forms", which is syntactically wrong; update PROVEN_CORE.md to use the actual Rust identifiers instead of the "$-form" output names—specifically correct the entries that reference parse$int_fast, parse$float_fast, field$str, field$int, and field$float so they appear as the real Rust identifiers (remove any introduced "$-form" renaming or mis-formatting) and apply the same corrections to the other occurrences noted (the other lines mentioned in the comment).docs/CONFIG_REFERENCE.md (2)
425-432:⚠️ Potential issue | 🟠 MajorConfig key
metrics$interval_secsmust remainmetrics_interval_secs.The
$separator applies only to internal Arrow column names, not YAML config keys. The runtime config struct (ServerConfig) expectsmetrics_interval_secs— using$here silently falls back to the default (60s).-| `metrics$interval_secs` | integer | `60` | Push interval for OTLP metrics in seconds. | +| `metrics_interval_secs` | integer | `60` | Push interval for OTLP metrics in seconds. |- metrics$interval_secs: 30 + metrics_interval_secs: 30🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CONFIG_REFERENCE.md` around lines 425 - 432, The docs show the YAML key as metrics$interval_secs but the runtime ServerConfig expects metrics_interval_secs, causing the value to be ignored; update the example and description to use metrics_interval_secs (underscore) everywhere so the config maps correctly to ServerConfig.metrics_interval_secs and the runtime honors the provided interval instead of falling back to the default.
517-517:⚠️ Potential issue | 🟠 MajorSame issue: config key should be
metrics_interval_secs.- metrics$interval_secs: 60 + metrics_interval_secs: 60🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CONFIG_REFERENCE.md` at line 517, The docs currently show the config key as metrics$interval_secs; change this to the correct config key metrics_interval_secs everywhere it appears (update the example line that reads "metrics$interval_secs: 60" to "metrics_interval_secs: 60") and run a quick grep/search for any other occurrences of metrics$interval_secs to replace with metrics_interval_secs so docs match the actual config key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08f1aef8-6579-4d60-b7dd-5c6f0874a1d9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (58)
.github/workflows/ai-duplicate-issues.yml.github/workflows/bench.ymlAGENTS.mdCODE_STYLE.mdDEVELOPING.mdREADME.mdbench/dashboard/competitive.htmlbench/dashboard/run.htmlbench/dashboard/style.cssbench/explore.shbook/src/config/reference.mdbook/src/deployment/kubernetes.mdbook/src/deployment/monitoring.mdbook/src/development/contributing.mdcrates/logfwd-bench/src/rss.rscrates/logfwd-competitive-bench/src/runner.rscrates/logfwd-competitive-bench/src/summarize.rscrates/logfwd-core/Cargo.tomlcrates/logfwd-core/fuzz/Cargo.tomlcrates/logfwd-core/fuzz/fuzz_targets/format_parser.rscrates/logfwd-core/fuzz/fuzz_targets/pipe_event.rscrates/logfwd-core/src/lib.rscrates/logfwd-core/src/otlp.rscrates/logfwd-core/src/pipeline/batch.rscrates/logfwd-core/src/pipeline/lifecycle.rscrates/logfwd-core/src/pipeline/mod.rscrates/logfwd-core/src/scan_config.rscrates/logfwd-core/tests/compliance_data.rscrates/logfwd-io/src/format.rscrates/logfwd-io/src/lib.rscrates/logfwd-output/Cargo.tomlcrates/logfwd-output/src/lib.rscrates/logfwd-output/src/stdout.rscrates/logfwd-transform/src/rewriter.rscrates/logfwd/Cargo.tomlcrates/logfwd/src/pipeline.rsdev-docs/ARCHITECTURE.mddev-docs/DECISIONS.mddev-docs/DIRECTION.mddev-docs/PHASES.mddev-docs/PROVEN_CORE.mddev-docs/research/BOUNDARY_PATTERNS.mddev-docs/research/ENFORCEMENT_AND_MECHANICS.mddev-docs/research/KANI_LIMITS.mddev-docs/research/offset-checkpoint-research.mddev-docs/research/structural-index-research.mddev-docs/research/wide-crate-evaluation.mddocs/CONFIG_REFERENCE.mddocs/DEPLOYMENT.mddocs/SCANNER_CONTRACT.mddocs/references/arrow-v54.mddocs/references/datafusion-v45.mddocs/references/kani-verification.mddocs/references/notify-memchr-zstd.mddocs/references/opentelemetry-otlp.mddocs/references/production-patterns.mddocs/references/tokio-async-patterns.mddocs/research/SCANNER_DESIGN_V1.md
💤 Files with no reviewable changes (10)
- .github/workflows/ai-duplicate-issues.yml
- crates/logfwd/Cargo.toml
- crates/logfwd-core/src/scan_config.rs
- crates/logfwd-core/src/lib.rs
- crates/logfwd-output/Cargo.toml
- dev-docs/DECISIONS.md
- dev-docs/research/offset-checkpoint-research.md
- crates/logfwd-core/src/pipeline/mod.rs
- crates/logfwd-core/src/pipeline/batch.rs
- crates/logfwd-core/src/pipeline/lifecycle.rs
| level$str, | ||
| msg$str, | ||
| _time_ns$int, | ||
| $stream$str |
There was a problem hiding this comment.
Column name typo: use _stream$str, not $stream$str.
The field name is _stream; dropping _ changes the identifier and makes the example query fail.
Proposed fix
- $stream$str
+ _stream$str📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $stream$str | |
| _stream$str |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@book/src/deployment/kubernetes.md` at line 108, The example uses the wrong
column identifier `$stream$str` instead of the actual field name `_stream$str`;
update the example occurrence of `$stream$str` to `_stream$str` so the query
references the correct `_stream` field (replace the token `$stream$str` with
`_stream$str` in the markdown example).
Comment on lines
343
to
+344
| let (_lines_done, _timed_out) = | ||
| wait_blackhole_done(blackhole, ctx.lines, Duration::from_secs(300)); | ||
| wait_blackhole_done(blackhole, ctx.lines, Duration::from_secs(120)); |
There was a problem hiding this comment.
Profiling runs may need more time than 120s.
run_agent_perf instruments with perf record which adds overhead. The previous 300s timeout was more conservative for profiling workloads. Consider keeping a higher timeout for profiling modes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-competitive-bench/src/runner.rs` around lines 343 - 344, The
timeout for waiting on the blackhole is hard-coded to 120s in the call to
wait_blackhole_done(blackhole, ctx.lines, Duration::from_secs(120)), which is
too short for profiling runs like run_agent_perf that use perf record; change
the logic so profiling modes use a larger timeout (e.g., 300s) — either pass a
variable timeout computed from ctx or a boolean is_profiling flag and call
wait_blackhole_done with Duration::from_secs(300) for profiling and keep 120s
for normal runs, referencing wait_blackhole_done, run_agent_perf, blackhole, and
ctx.lines to locate the change.
Comment on lines
431
to
+432
| let (_lines_done, _timed_out) = | ||
| wait_blackhole_done(blackhole, ctx.lines, Duration::from_secs(300)); | ||
| wait_blackhole_done(blackhole, ctx.lines, Duration::from_secs(120)); |
There was a problem hiding this comment.
dhat profiling also has significant overhead.
Same concern as perf — run_agent_dhat instruments heap allocations which can substantially slow execution. 120s may be too aggressive for memory profiling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-competitive-bench/src/runner.rs` around lines 431 - 432, The
current call to wait_blackhole_done(blackhole, ctx.lines,
Duration::from_secs(120)) uses a fixed 120s timeout which is too long when
run_agent_dhat (heap-allocation instrumentation) is enabled; change this to use
a shorter timeout or a configurable value when memory profiling is active.
Detect the dhat profiling path (where run_agent_dhat is used) and pass a reduced
Duration (e.g., 30s) or wire a new configurable timeout (constant/CLI/config
field) instead of the hard-coded Duration::from_secs(120) so wait_blackhole_done
and ctx.lines use the appropriate, profile-aware timeout. Ensure references to
wait_blackhole_done, run_agent_dhat, Duration::from_secs(120), ctx.lines, and
blackhole are updated accordingly.
Comment on lines
+439
to
444
| // NOTE: if the same agent appears in both binary and docker mode, | ||
| // the second insert overwrites the first. This is acceptable while | ||
| // CI only runs binary mode, but would need a (mode, agent) key to | ||
| // support both modes simultaneously. | ||
| if let Value::Object(map) = scenario_entry { | ||
| map.insert(g.name.clone(), agent_data); |
There was a problem hiding this comment.
Don't silently overwrite one mode's dashboard entry.
This object is keyed only by agent name even though the grouping key already distinguishes mode. If the same agent/scenario is emitted for both binary and docker, the later insert wins and the earlier result disappears from the dashboard JSON.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-competitive-bench/src/summarize.rs` around lines 439 - 444, The
dashboard JSON map keyed by agent name is overwriting entries when the same
agent appears in different modes; in the block where you match
Value::Object(map) from scenario_entry and call map.insert(g.name.clone(),
agent_data), change the key to include the mode (e.g., format!("{}:{}", g.name,
mode) or use a tuple-like string) so entries are unique per (mode, agent);
update any readers to expect the new composite key and ensure you still clone
g.name and use agent_data as the value.
Comment on lines
793
to
+795
| let ts = b"2024-01-15T10:30:00Z____extra___"; | ||
| let nanos = parse_timestamp_nanos(&ts[..20]); | ||
| assert!(nanos == Some(1_705_314_600_000_000_000)); | ||
| assert!(nanos == 1_705_314_600_000_000_000); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this is a compilation error
cd crates/logfwd-core
cargo check --features kani 2>&1 | head -50Repository: strawgate/memagent
Length of output: 427
🏁 Script executed:
#!/bin/bash
# Check parse_timestamp_nanos signature
sed -n '180,190p' crates/logfwd-core/src/otlp.rs
# Check the assertion context
sed -n '790,800p' crates/logfwd-core/src/otlp.rsRepository: strawgate/memagent
Length of output: 903
Fix type mismatch in assertion: nanos is Option<u64>, not u64.
Line 795 compares Option<u64> to a raw u64 literal. The assertion won't compile.
Fix
let ts = b"2024-01-15T10:30:00Z____extra___";
let nanos = parse_timestamp_nanos(&ts[..20]);
-assert!(nanos == 1_705_314_600_000_000_000);
+assert!(nanos == Some(1_705_314_600_000_000_000));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let ts = b"2024-01-15T10:30:00Z____extra___"; | |
| let nanos = parse_timestamp_nanos(&ts[..20]); | |
| assert!(nanos == Some(1_705_314_600_000_000_000)); | |
| assert!(nanos == 1_705_314_600_000_000_000); | |
| let ts = b"2024-01-15T10:30:00Z____extra___"; | |
| let nanos = parse_timestamp_nanos(&ts[..20]); | |
| assert!(nanos == Some(1_705_314_600_000_000_000)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-core/src/otlp.rs` around lines 793 - 795, The assertion
compares an Option<u64> to a u64; update the test to assert against the Option
variant returned by parse_timestamp_nanos: either assert_eq!(nanos,
Some(1_705_314_600_000_000_000)) or unwrap nanos and assert the inner u64 (e.g.,
assert_eq!(nanos.unwrap(), 1_705_314_600_000_000_000)); locate the call to
parse_timestamp_nanos and the variable nanos in the test near that snippet and
apply one of these fixes so the types match.
Comment on lines
+116
to
+119
| b if b < 0x20 => { | ||
| // Escape control characters per RFC 8259. | ||
| let _ = std::io::Write::write_fmt(out, format_args!("\\u{:04x}", b)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
format_args! in per-byte loop may impact hot-path performance.
Control characters < 0x20 are rare, but write_fmt involves formatting overhead. Consider a small lookup table or direct byte writes:
♻️ Suggested optimization
b if b < 0x20 => {
- // Escape control characters per RFC 8259.
- let _ = std::io::Write::write_fmt(out, format_args!("\\u{:04x}", b));
+ // Escape control characters per RFC 8259.
+ const HEX: &[u8; 16] = b"0123456789abcdef";
+ out.extend_from_slice(b"\\u00");
+ out.push(HEX[(b >> 4) as usize]);
+ out.push(HEX[(b & 0xf) as usize]);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-io/src/format.rs` around lines 116 - 119, The per-byte match
arm handling control chars (the branch "b if b < 0x20" in format.rs) uses
std::io::Write::write_fmt with format_args! which allocates formatting overhead
on the hot path; replace it by writing the four hex digits directly (or using a
tiny static lookup) into the output writer — e.g., precompute the ASCII bytes
for "\\u00" and the two hex nibbles of b and call out.write_all(&[b'\\', b'u',
b'0', b'0', hex1, hex2]) or equivalent to avoid format_args!/write_fmt; update
the match arm in the same function accordingly and ensure error propagation
remains unchanged.
Comment on lines
+165
to
+181
| fn process(&mut self, bytes: &[u8], out: &mut Vec<u8>) -> (usize, usize) { | ||
| self.partial.extend_from_slice(bytes); | ||
|
|
||
| let Some(last_nl) = memchr::memrchr(b'\n', &self.partial) else { | ||
| // No complete line yet — keep buffering. | ||
| return (0, 0); | ||
| }; | ||
|
|
||
| let process_end = last_nl + 1; | ||
| let result = cri::process_cri_to_buf( | ||
| &self.partial[..process_end], | ||
| &mut self.reassembler, | ||
| None, | ||
| out, | ||
| ); | ||
| self.partial.drain(..process_end); | ||
| result |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unconditional copy into partial buffer on every call.
Line 166 copies all input bytes into self.partial regardless of whether complete lines exist. This causes allocation on every chunk even when unnecessary.
Consider optimizing: if input ends with \n and partial is empty, process directly without buffering.
♻️ Suggested optimization sketch
fn process(&mut self, bytes: &[u8], out: &mut Vec<u8>) -> (usize, usize) {
- self.partial.extend_from_slice(bytes);
-
- let Some(last_nl) = memchr::memrchr(b'\n', &self.partial) else {
+ // Fast path: no partial state and input ends with newline
+ if self.partial.is_empty() {
+ if let Some(last_nl) = memchr::memrchr(b'\n', bytes) {
+ let process_end = last_nl + 1;
+ let result = cri::process_cri_to_buf(
+ &bytes[..process_end],
+ &mut self.reassembler,
+ None,
+ out,
+ );
+ if process_end < bytes.len() {
+ self.partial.extend_from_slice(&bytes[process_end..]);
+ }
+ return result;
+ }
+ // No newline — buffer for later
+ self.partial.extend_from_slice(bytes);
+ return (0, 0);
+ }
+
+ // Slow path: have partial data to prepend
+ self.partial.extend_from_slice(bytes);
+ let Some(last_nl) = memchr::memrchr(b'\n', &self.partial) else {
// No complete line yet — keep buffering.
return (0, 0);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-io/src/format.rs` around lines 165 - 181, The process method
unconditionally appends all input to self.partial causing unnecessary
allocations; change logic to first check if self.partial.is_empty() and the
incoming bytes contain a trailing newline (bytes.last() == Some(&b'\n')) and if
so call cri::process_cri_to_buf directly on bytes (passing &mut self.reassembler
and out) and return its result without copying into self.partial; otherwise fall
back to the current behavior (append to self.partial, find last newline with
memchr::memrchr, compute process_end, call cri::process_cri_to_buf on
&self.partial[..process_end], drain processed bytes, and return the result).
Ensure you still handle the case where no newline is found by returning (0,0)
when appropriate.
Comment on lines
209
to
+212
| "float" => { | ||
| let arr = arr.as_primitive::<arrow::datatypes::Float64Type>(); | ||
| let v = arr.value(row); | ||
| // RFC 8259: JSON numbers cannot be inf, -inf, or NaN. | ||
| // Emit as null instead of producing invalid JSON. | ||
| if v.is_finite() { | ||
| let _ = Write::write_fmt(out, format_args!("{}", v)); | ||
| } else { | ||
| out.extend_from_slice(b"null"); | ||
| } | ||
| let _ = Write::write_fmt(out, format_args!("{}", v)); |
There was a problem hiding this comment.
Restore full JSON escaping and non-finite float handling.
The float branch now writes NaN/inf verbatim, and the string branch leaves other 0x00..=0x1F bytes unescaped. Both are invalid JSON, so shared stdout/JSONL output will break on non-finite metrics or strings containing NUL/backspace/form-feed.
Also applies to: 218-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-output/src/lib.rs` around lines 209 - 212, The float match arm
(where arr.as_primitive::<arrow::datatypes::Float64Type>() and v is obtained and
written via Write::write_fmt) must check v.is_finite() and emit a JSON-safe
representation (e.g., write the numeric literal when finite, and write null for
non-finite values) instead of printing "NaN"/"inf" verbatim; the string match
arm should perform full JSON string escaping (escaping control bytes 0x00..0x1F,
backslash, quotes, and control sequences) rather than writing raw bytes—best
approach is to delegate both cases to a JSON-safe serializer (for example use
serde_json::to_writer or its string-escaping helper) to write the float or
string into out so all control chars and non-finite floats become valid JSON.
Comment on lines
+637
to
642
| fn build_format_parser(format: &Format) -> Box<dyn FormatParser> { | ||
| match format { | ||
| Format::Cri => Box::new(CriParser::new(2 * 1024 * 1024)), | ||
| Format::Raw => Box::new(RawParser::new()), | ||
| _ => Box::new(JsonParser::new()), | ||
| } |
There was a problem hiding this comment.
Format::Auto is being downgraded to JSON-only here.
This helper falls back to JsonParser for every non-Cri/Raw variant. Because input construction still defaults missing formats to Auto, the documented default now silently becomes JSON-only, and unsupported input formats are accepted instead of rejected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd/src/pipeline.rs` around lines 637 - 642, build_format_parser
currently treats any non-Cri/Raw variant (including Format::Auto) as JsonParser,
silently downgrading Auto to JSON; change build_format_parser to explicitly
handle Format::Auto (and any other unsupported variants) instead of falling back
to JsonParser: add a match arm for Format::Auto that returns an explicit error
(e.g., Result::Err with a clear message) and change the function signature to
return Result<Box<dyn FormatParser>, SomeErrorType>, update callers to
propagate/handle that Result, and keep the existing arms for Format::Cri ->
CriParser::new(...), Format::Raw -> RawParser::new(), and for Format::Json
explicitly return JsonParser::new() so unsupported formats are rejected rather
than accepted.
| Split logfwd-core into proven core + satellite crates. | ||
|
|
||
| ```text | ||
| ``` |
There was a problem hiding this comment.
Put the text info strings back on these fences.
These edits introduce new MD040 warnings. Keep them as text fences so docs lint stays clean.
Also applies to: 37-37, 47-47
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev-docs/PHASES.md` at line 14, Restore the missing "text" info string on the
fenced code blocks causing MD040 by changing the bare triple backticks to
"```text" for the three affected fences in PHASES.md (the ones that previously
had "```text" and now are plain "```"), so the doc linter stops reporting MD040
and the examples remain text-fenced.
Owner
Author
|
Closing — the global find-and-replace of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug where JSON field names ending in
_str,_int, or_floatwere incorrectly identified as internal type suffixes and silently renamed or dropped during serialization.Key changes:
_to$(e.g.,field$int) across all crates.logfwd-outputto group multiple type variants per logical field and pick the best non-null value for each row.README.mddocumentation and all internal test suites.Fixes #407
PR created automatically by Jules for task 2196699203870188821 started by @strawgate