fix(sql_execution): Fix is large number check to use 2**53 as cutoff#73
Draft
fix(sql_execution): Fix is large number check to use 2**53 as cutoff#73
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces a local large-number helper in deepnote_toolkit/sql/sql_execution.py with is_large_number from deepnote_toolkit.ocelots.pandas.utils and removes the local _is_large_number and Decimal import. Adds MAX_SAFE_FLOAT64_INTEGER, is_large_number, and cast_large_numbers_to_string in deepnote_toolkit/ocelots/pandas/utils.py. Calls cast_large_numbers_to_string(df_copy) from deepnote_toolkit/ocelots/pandas/implementation.py inside to_records when mode="json". Expands tests and fixtures to cover 2**53 boundary behavior, large integers, Decimals, mixed-type columns, and precision-preservation cases. Sequence Diagram(s)sequenceDiagram
participant Caller
participant Ocelots as Ocelots.to_records
participant Utils as utils.cast_large_numbers_to_string
participant DF as DataFrame
Caller->>Ocelots: to_records(df_copy, mode="json")
Ocelots->>Utils: cast_large_numbers_to_string(df_copy)
Utils->>DF: inspect columns using is_large_number / MAX_SAFE_FLOAT64_INTEGER
DF-->>Utils: convert large-number columns to strings
Utils-->>Ocelots: return sanitized df_copy
Ocelots->>Caller: return records (from sanitized df_copy)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
📦 Python package built successfully!
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
🚀 Review App Deployment Started
|
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
…JSON compatibility - Introduced `cast_large_numbers_to_string` function to convert numeric values exceeding the float64 safe integer range (2**53) to strings, preserving precision for JSON serialization. - Updated `PandasImplementation.to_json` method to utilize the new function. - Added unit tests to ensure correct behavior for large numbers in dataframes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepnote_toolkit/ocelots/pandas/utils.py`:
- Around line 119-120: The return statement exceeds the 88-character limit;
break it into multiple shorter lines by splitting the isinstance check and the
comparison across lines or by assigning parts to temporaries: e.g., compute a
boolean like is_num = isinstance(x, (int, float, Decimal, np.integer,
np.floating)) on one line and then return is_num and abs(x) >
MAX_SAFE_FLOAT64_INTEGER on the next, updating the return in the function
containing that line to use these shorter lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa49302c-c577-464b-8417-e4db1b3b012a
📒 Files selected for processing (5)
deepnote_toolkit/ocelots/pandas/implementation.pydeepnote_toolkit/ocelots/pandas/utils.pydeepnote_toolkit/sql/sql_execution.pytests/unit/helpers/testing_dataframes.pytests/unit/test_dataframe_utils.py
- Reformatted the `is_large_number` and `cast_large_numbers_to_string` functions for better readability by using multi-line expressions. - Updated unit tests to call the `is_large_number` function directly instead of through a different module, ensuring consistency and clarity in test cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepnote_toolkit/ocelots/pandas/utils.py`:
- Around line 128-142: The function cast_large_numbers_to_string is missing
explicit type hints; update its signature to include pandas DataFrame input and
return types (e.g., df: pd.DataFrame -> pd.DataFrame) and ensure pandas is
imported as pd in the module if not already; keep the implementation and
references to helpers (is_pure_numeric, is_large_number, safe_convert_to_string)
unchanged so callers and static type checkers recognize the typed signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff36bff8-c27f-468a-83ad-596bc6377706
📒 Files selected for processing (3)
deepnote_toolkit/ocelots/pandas/utils.pydeepnote_toolkit/sql/sql_execution.pytests/unit/test_sql_execution_internal.py
Add explicit pd.DataFrame input and return type annotations to cast_large_numbers_to_string so callers and static type checkers (mypy) recognise the typed signature.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepnote_toolkit/ocelots/pandas/utils.py`:
- Around line 112-125: Add an explicit type hint for the parameter in
is_large_number by changing its signature to is_large_number(x: Any) -> bool and
ensure typing.Any is imported at the top of the module; keep the existing
isinstance checks (Decimal, np.integer, np.floating, int, float) and return
logic using MAX_SAFE_FLOAT64_INTEGER unchanged so type checking tools accept the
function signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 845868d1-da3a-41ce-a2a6-36905b079c94
📒 Files selected for processing (1)
deepnote_toolkit/ocelots/pandas/utils.py
Import typing.Any and annotate the x parameter of is_large_number so static type checkers (mypy) accept the function signature.
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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
New Features
Tests