fix(sqla): preserve float precision when mixing int/float values in IN filters - #42752
fix(sqla): preserve float precision when mixing int/float values in IN filters#42752varadendrasimha511 wants to merge 4 commits into
Conversation
…N filters Fixes apache#33206. When a numeric filter combines integer and float values (e.g. IN (33, 29.02)), SQLAlchemy may infer the bind parameter type from the first value in the list, silently truncating subsequent float values during query compilation. This normalizes mixed int/float values to float before binding when the target column is numeric, preventing silent data loss in filtered results. Adds a regression test verifying the compiled SQL preserves decimal precision for mixed-type IN filters.
Code Review Agent Run #0000d9Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| eq = [ | ||
| float(v) if isinstance(v, (int, float)) else v | ||
| for v in eq | ||
| ] |
There was a problem hiding this comment.
Suggestion: Converting every integer in a mixed list with float(v) loses precision for integers larger than the exact range of IEEE-754 doubles. For example, 9007199254740993 becomes 9007199254740992.0, so an integer-column filter can match the wrong row or fail to match the requested ID. Preserve integer values and only force the bind type to a floating type without changing their numeric representation. [type error]
Severity Level: Major ⚠️
- ❌ BigInteger `IN` filters can lose exact integer values.
- ⚠️ Chart query results can omit or misidentify matching rows.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 4106:4109
**Comment:**
*Type Error: Converting every integer in a mixed list with `float(v)` loses precision for integers larger than the exact range of IEEE-754 doubles. For example, `9007199254740993` becomes `9007199254740992.0`, so an integer-column filter can match the wrong row or fail to match the requested ID. Preserve integer values and only force the bind type to a floating type without changing their numeric representation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
Agreed—mixing 9007199254740993 with a fractional value changes the requested integer to 9007199254740992.0, so exact NUMERIC filters can match the wrong row. Could we avoid coercing the original integers to binary float?
There was a problem hiding this comment.
Agreed—the current conversion can corrupt integers above 2**53, so this should not be fixed by converting the original values to float.
A safer approach is to avoid mixing bind types altogether: keep integer values in one IN predicate and floating-point values in another, then combine them with OR:
if (
target_generic_type == utils.GenericDataType.NUMERIC
and any(type(v) is float for v in eq)
and any(type(v) is int for v in eq)
):
integer_values = [v for v in eq if type(v) is int]
float_values = [v for v in eq if type(v) is float]
cond = or_(
sqla_col.in_(integer_values),
sqla_col.in_(float_values),
)
else:
cond = sqla_col.in_(eq)This preserves values such as 9007199254740993 as integers while ensuring the decimal values are bound as floats. The regression test should also include a large integer mixed with a fractional value to verify that exact integer precision is retained.
| sqla_query.sqla_query.compile( | ||
| dialect=engine.dialect, | ||
| compile_kwargs={"literal_binds": True}, | ||
| ) |
There was a problem hiding this comment.
Suggestion: Using literal_binds=True removes the bind-parameter path that caused the production regression, because SQLAlchemy renders the Python values directly into the SQL text. This assertion can pass even when execution with bound parameters still infers an integer type from the first value and truncates the decimal. Assert the generated bind parameter types/values or execute the query against an engine that reproduces the affected behavior. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Regression test does not cover bound-parameter execution.
- ⚠️ Precision truncation could regress undetected in numeric IN filters.
- ⚠️ Query correctness remains unverified for affected database dialects.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/models/helpers_test.py
**Line:** 2721:2724
**Comment:**
*Incomplete Implementation: Using `literal_binds=True` removes the bind-parameter path that caused the production regression, because SQLAlchemy renders the Python values directly into the SQL text. This assertion can pass even when execution with bound parameters still infers an integer type from the first value and truncates the decimal. Assert the generated bind parameter types/values or execute the query against an engine that reproduces the affected behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42752 +/- ##
==========================================
- Coverage 65.59% 65.59% -0.01%
==========================================
Files 2819 2819
Lines 160166 160168 +2
Branches 36569 36570 +1
==========================================
- Hits 105065 105056 -9
- Misses 53053 53062 +9
- Partials 2048 2050 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #c52c31Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #340247Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| # SQLAlchemy may infer the bind parameter type from the | ||
| # first element and silently truncate other values | ||
| # (see #33206) | ||
| if target_generic_type == utils.GenericDataType.NUMERIC and any( |
There was a problem hiding this comment.
This normalization only runs in the no-NULL branch, so IN [33, 29.02, NULL] still compiles the fractional value as 29 and omits matching rows. Could we hoist the normalization before eq_without_none is derived and cover the NULL-bearing list?
2 participants
SUMMARY
Fixes #33206.
When a numeric filter combines integer and decimal values (e.g.
IN (33, 29.02)), SQLAlchemy may infer the bind parameter type from the first value in the list. If that first value is anint, subsequentfloatvalues in the sameINclause can be silently truncated during query compilation — dropping their decimal precision entirely and causing matching rows to disappear from results without any error.This was reproduced and root-caused end-to-end:
cast_to_num/handle_single_valueFloateq(the filter value list) is still fully correct immediately beforesqla_col.in_(eq)is calledintandfloatare combined in the sameINlist — decimals alone, or ints alone, both work correctlyThe fix normalizes mixed int/float values to
floatright before binding, only when the target column is numeric and the list actually contains a mix — so purely-integer filters (e.g. ID lookups) are unaffected, addressing the earlier concern in #33230 about integer precision loss for large IDs.BEFORE/AFTER
Before: Filtering
global_sales IN (33, 29.02)on a table with both integer and decimal sales values silently drops the row matching29.02, returning only the row matching33.After: Both rows are correctly returned.
TESTING INSTRUCTIONS
video_game_sales.global_sales)33and29.02)Added
test_get_sqla_query_in_filter_preserves_float_precisionintests/unit_tests/models/helpers_test.py, which compiles the generated SQL and asserts the float value's precision is preserved in theINclause.ADDITIONAL INFORMATION