Repository navigation
perf: bound Python lexer buffering and preserve JSON conversion - #183
Closed
RyaliNvidia wants to merge 1 commit into
Closed
RyaliNvidia wants to merge 1 commit into
RyaliNvidia wants to merge 1 commit into
Conversation
Reviewer's GuideThe PR bounds Python lexer buffering through periodic compaction, preserves streaming token and error-offset semantics, adds an explicit opt-in for Python float overflow conversion, and enforces exact JSON number and whitespace syntax with comprehensive regression and memory tests. Sequence diagram for Python float overflow handlingsequenceDiagram
participant Caller
participant Backend as PythonBackend
participant Parser as parse_value
participant Converter as to_number
Caller->>Backend: basic_parse_basecoro(use_float, allow_float_overflow)
alt allow_float_overflow and not use_float
Backend-->>Caller: ValueError
else valid configuration
Backend->>Parser: parse_value(..., use_float, allow_float_overflow)
Parser->>Parser: JSON_NUMBER_RE.fullmatch(symbol)
Parser->>Converter: to_number(symbol)
alt float overflow and option disabled
Parser-->>Caller: JSONError
else float overflow explicitly allowed
Parser-->>Caller: Converted Python float
else valid number
Parser-->>Caller: JSON number event
end
end
Flow diagram for bounded Python lexer bufferingflowchart LR
Chunks[Input chunks] --> Buffer[Lexer buffer]
Buffer --> Tokens[Completed lexemes]
Buffer --> Partial[Unfinished token]
Tokens --> Compact{Position reaches threshold}
Compact -->|yes| Discard[Discard consumed prefix]
Compact -->|no| Buffer
Discard --> Buffer
Partial --> Buffer
Flow diagram for exact JSON number validationflowchart TD
Lexeme[Numeric lexeme] --> Match{JSON_NUMBER_RE.fullmatch}
Match -->|no| Error[Reject malformed JSON number]
Match -->|yes| Convert[to_number]
Convert --> Overflow{Float infinity?}
Overflow -->|yes and not allowed| Error
Overflow -->|no or explicitly allowed| Event[Emit numeric value]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/ijson/backends/python.py" line_range="229" />
<code_context>
+ raise ValueError("Invalid JSON number")
number = to_number(symbol)
- if number == inf:
+ if number == inf and not allow_float_overflow:
raise common.JSONError("float overflow: %s" % (symbol,))
except:
</code_context>
<issue_to_address>
**issue (bug_risk):** Negative floating-point overflow is not rejected by the default path because `float('-1e400')` is `-inf`, which does not equal the positive `inf` sentinel. The Python backend therefore emits `-inf` when `use_float=True` and `allow_float_overflow` is left at its default.
**Triggers:** When valid JSON contains a sufficiently large negative exponent value such as `-1e400`.
**Suggested fix:** Use an infinity check that covers both signs, such as `math.isinf(number)`, before applying `allow_float_overflow`.
</issue_to_address>
### Comment 2
<location path="tests/test_python_streaming.py" line_range="67-69" />
<code_context>
+
+def test_compact_long_string_array_has_bounded_peak_memory(tmp_path):
+ code = '''
+import json, resource, sys
+from ijson.backends import python as backend
+with open(sys.argv[1], 'rb') as stream:
</code_context>
<issue_to_address>
**issue (testing):** The RSS regression subprocess imports the Unix-only `resource` module unconditionally, so the test raises `ModuleNotFoundError` and fails on Windows before measuring memory.
**Triggers:** When the test suite runs on Windows.
**Suggested fix:** Skip this test when `resource` is unavailable, or use a platform-independent process-memory measurement.
```suggestion
def test_compact_long_string_array_has_bounded_peak_memory(tmp_path):
try:
import resource
except ImportError:
pytest.skip('resource module is unavailable')
code = '''
import json, resource, sys
```
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: src/ijson/backends/python.py:229, tests/test_python_streaming.py:69
| raise ValueError("Invalid JSON number") | ||
| number = to_number(symbol) | ||
| if number == inf: | ||
| if number == inf and not allow_float_overflow: |
There was a problem hiding this comment.
issue (bug_risk): Negative floating-point overflow is not rejected by the default path because float('-1e400') is -inf, which does not equal the positive inf sentinel. The Python backend therefore emits -inf when use_float=True and allow_float_overflow is left at its default.
Triggers: When valid JSON contains a sufficiently large negative exponent value such as -1e400.
Suggested fix: Use an infinity check that covers both signs, such as math.isinf(number), before applying allow_float_overflow.
Comment on lines
+67
to
+69
| def test_compact_long_string_array_has_bounded_peak_memory(tmp_path): | ||
| code = ''' | ||
| import json, resource, sys |
There was a problem hiding this comment.
issue (testing): The RSS regression subprocess imports the Unix-only resource module unconditionally, so the test raises ModuleNotFoundError and fails on Windows before measuring memory.
Triggers: When the test suite runs on Windows.
Suggested fix: Skip this test when resource is unavailable, or use a platform-independent process-memory measurement.
Suggested change
| def test_compact_long_string_array_has_bounded_peak_memory(tmp_path): | |
| code = ''' | |
| import json, resource, sys | |
| def test_compact_long_string_array_has_bounded_peak_memory(tmp_path): | |
| try: | |
| import resource | |
| except ImportError: | |
| pytest.skip('resource module is unavailable') | |
| code = ''' | |
| import json, resource, sys |
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.
Large documents containing many strings can retain already-consumed prefixes in the Python lexer's buffer. Compact completed prefixes at a bounded threshold while preserving unfinished tokens and absolute error offsets. A subprocess regression increases generated input from 16 MiB to 256 MiB and requires peak RSS growth of at most 48 MiB.
The Python backend also gains an explicit
allow_float_overflowoption requiringuse_float=True. It preserves Python float conversion for valid JSON exponent overflow while keeping arbitrary-size integer conversion and existing defaults. Exact JSON number syntax and whitespace checks prevent Python's permissive numeric conversions or Unicode whitespace matching from accepting malformed input.Tests cover chunk boundaries, escaped strings and surrogate identities, offsets, overflow/underflow, malformed syntax, and memory growth. No parser fallback or runtime patch is introduced.
Validation: the upstream test suite passed with 621 tests and 18 skips. Independent chunk/offset and standard-library differential checks also passed.
Summary by Sourcery
Bound Python lexer memory usage and make JSON numeric conversion behavior explicit and standards-compliant.
New Features:
Bug Fixes:
Tests: