Accept bare int for int32 config fields in parser#265
Open
hbarthels wants to merge 2 commits into
Open
Conversation
`_extract_value_int32` only accepted a `Value` whose oneof was
`int32_value`, so a bare integer literal (parsed as int64 `int_value`,
e.g. `-1` rather than `-1i32`) silently fell back to the field default.
Concretely `(csv_config {:csv_header_row -1})` yielded `header_row = 1` —
the opposite meaning — with no error.
Fix `_extract_value_int32` in the grammar to also accept `int_value` via
a checked narrowing conversion (Julia `Int32(...)` throws on overflow),
and regenerate the Python/Julia/Go parsers. `csv_header_row` is the only
config field using this helper. Add regression tests asserting a bare
`-1` parses identically to `-1i32`.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
comnik
reviewed
Jul 10, 2026
| name = "LogicalQueryProtocol" | ||
| uuid = "a92373ee-6cc4-4662-ae66-0c99a03ebae1" | ||
| version = "0.5.5" | ||
| version = "0.5.6" |
Collaborator
There was a problem hiding this comment.
Please keep the version bumps to separate PRs
comnik
reviewed
Jul 10, 2026
| if value is not None and builtin.has_proto_field(builtin.unwrap_option(value), 'int32_value'): | ||
| return builtin.unwrap_option(value).int32_value | ||
| if value is not None and builtin.has_proto_field(builtin.unwrap_option(value), 'int_value'): | ||
| return builtin.int64_to_int32(builtin.unwrap_option(value).int_value) |
Collaborator
There was a problem hiding this comment.
Given that the S-expression syntax is meant for human readability, not necessarily writing convenience, I'd argue that the thing to do here for now is to assert that it's an int32_value.
Because if we allow promotion, we should do it generally, meaning anywhere that I write 1i32 today, I can just write 1 instead. But I'm not sure that convenience is worth having an implicit behaviour. The real problem here is the silent failure, not that we're strict.
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
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.
Problem
The generated LQP parsers silently lose integer config values that don't carry an exact-width suffix.
_extract_value_int32only accepted aValuewhose oneof wasint32_value, so a bare literal like-1— parsed as an int64int_value— fell through to the field default.Concretely,
(csv_config {:csv_header_row -1})yieldedheader_row = 1(the opposite meaning) with no error.Fix
_extract_value_int32inmeta/src/meta/grammar.ynow also acceptsint_valuevia a checked narrowing conversion, falling back to the default only when neitherint32_valuenorint_valueis present:All three parsers (Python/Julia/Go) were regenerated via
make force-parsers; the large generated diffs are gensym-counter renumbering — the only semantic change is the addedint_valuebranch.Scope
_extract_value_int32has exactly one call site:csv_header_row. There are nouint32/float32extract helpers, and the other configs (export, betree, iceberg) use string/int64/uint128 extractors that don't narrow. Socsv_header_rowis the only affected config field.Tests
-1parses identically to-1i32and differs from the default.Includes a version bump to v0.5.6 so the vendored copy in raicode can be re-synced (via
scripts/import-lqp.sh) once this lands.🤖 Generated with Claude Code