Skip to content

Accept bare int for int32 config fields in parser#265

Open
hbarthels wants to merge 2 commits into
mainfrom
hb-fix-int32-config-extract
Open

Accept bare int for int32 config fields in parser#265
hbarthels wants to merge 2 commits into
mainfrom
hb-fix-int32-config-extract

Conversation

@hbarthels

@hbarthels hbarthels commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Problem

The generated LQP parsers silently lose integer config values that don't carry an exact-width suffix. _extract_value_int32 only accepted a Value whose oneof was int32_value, so a bare literal like -1 — parsed as an int64 int_value — fell through 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 meta/src/meta/grammar.y now also accepts int_value via a checked narrowing conversion, falling back to the default only when neither int32_value nor int_value is present:

def _extract_value_int32(value, default):
    if value is not None and has_proto_field(value, 'int32_value'):
        return value.int32_value
    if value is not None and has_proto_field(value, 'int_value'):
        return int64_to_int32(value.int_value)   # Julia Int32(...) throws on overflow
    return int64_to_int32(default)

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 added int_value branch.

Scope

_extract_value_int32 has exactly one call site: csv_header_row. There are no uint32/float32 extract helpers, and the other configs (export, betree, iceberg) use string/int64/uint128 extractors that don't narrow. So csv_header_row is the only affected config field.

Tests

  • Added regression tests in the Julia and Python SDKs asserting a bare -1 parses identically to -1i32 and differs from the default.
  • Full Julia suite: 174/174 items, 33,739 assertions pass. Python: 73 tests pass. Go builds clean.

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

hbarthels and others added 2 commits July 11, 2026 00:18
`_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>
name = "LogicalQueryProtocol"
uuid = "a92373ee-6cc4-4662-ae66-0c99a03ebae1"
version = "0.5.5"
version = "0.5.6"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the version bumps to separate PRs

Comment thread meta/src/meta/grammar.y
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants