fix(dashboard): filter numeric values correctly - #33230
Conversation
fix(dashboard): filter numeric values correctly
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Redundant numeric casting fallback ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/connectors/sqla/models.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
rusackas, I have created the PR that fixes the issue. Can you kindly check and let me know if the checks are successful, since I am not able to test them on my own. Locally I ran the pre-commit hook and it was successful. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #33230 +/- ##
===========================================
+ Coverage 60.48% 72.90% +12.41%
===========================================
Files 1931 559 -1372
Lines 76236 40550 -35686
Branches 8568 4274 -4294
===========================================
- Hits 46114 29564 -16550
+ Misses 28017 9877 -18140
+ Partials 2105 1109 -996
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Possible duplicate of #33222 |
|
I do think I have made a few more changes. Can you please verify once. |
|
Running CI 🤞 |
|
Resolving the changes and giving updated PR |
|
Re-running |
|
Hi, for the docker build failing, it is giving a response 429 - too many requests. Is it possible to rerun it? @rusackas |
|
Can someone help and approve it? |
|
Not sure why CI got stuck, but I'll close and re-open this to kick-start the process. |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Code LGTM, and I appreciate the test. Approving from my viewpoint, but I'm not quite sure if there are edge case risks associated with setting everything as float, so I'll ping @mistercrunch and @betodealmeida here in case they spot any risks... they can merge if not!
|
Thanks @rusackas |
|
All checks have been success @rusackas |
| isinstance(value, (float, int)) | ||
| and target_generic_type == utils.GenericDataType.NUMERIC | ||
| ): | ||
| value = float(value) |
There was a problem hiding this comment.
why do we need to explicitly convert a float to a float here?
| # For backwards compatibility and edge cases | ||
| # where a column data type might have changed | ||
| try: | ||
| value = float(value) |
There was a problem hiding this comment.
wait there's a method right below (cast_to_num) that's attempting to do the same thing. I haven't looked into exactly what it does, but this feels like a patch-on-a-patch and we really need to move away from this type of layering here. My intuition says "let's get cast_to_num right and rely on it"
| if operator == utils.FilterOperator.TEMPORAL_RANGE: | ||
| return value | ||
|
|
||
| if ( |
There was a problem hiding this comment.
there's long-standing issue where someone copied a bunch of methods/logic between the two files in this PR, and we really need to fix this ... @betodealmeida and myself have spoken about this and we're meaning to fix this once and for all. Not sure what exactly it means for this PR, but I'd say if we're going to fix a thing here let's dedup at least the method we're touching here (handle_single_value)
|
Can we clarify why we need to convert float->int? I read "which led to a mismatch among values.", but I'm unclear on what that means ... is this ordering/filtering-related? |
|
Putting a hold for now as this part of the codebase is brittle and oddly layered, and with the premise that ints aren't floats and there might be intricacies around all this. |
|
all the numeric values were converted to float, because otherwise, it was found out that values were invariably truncated without explicitly converting them to float first. |
Not sure if I understand what that means... So first converting float to float is a no-op, but converting int-to-float somehow addresses something here (?) What is it? Feels we should dig and address deeper. |
Can you give an example of the problem? |
|
We most likely do not want to convert integers to floats, especially if they're being used in a comparison. Big integers are often used as IDs, and if they're are converted to floats they lose precision and become a different ID: >>> i = 2**53 + 1
>>> i
9007199254740993
>>> float(i)
9007199254740992.0 |
|
Consider the values [21, 21.8]. Without converting them to float, the code was considering both these values as same and thereby both of them were getting same values, which is not correct. So if there was an entry corresponding to 21 and an entry corresponding to 21.8, then the value corresponding to 21.8 was being shown by the value corresponding to 21. |
Curious on the exact issue, so some part of the code is having issues handling ints (?) Sounds like a pretty specific thing, where we have mixed arrays of floats and ints maybe (?), and, presumably, something that doesn't do well with mixed-type arrays? Or maybe there's logic some place that looks at the type of the first value in the array and converts the whole array to that type? |
|
About the duplicated logic across the two files. I finally got around to clean up this mess here #34177 |
|
Oh, found the issue reference in the PR body. Btw sorry if my comments come across poorly - I'm upset about the issue I address in #34177 and clearly that's not related to this PR in any way ... Let's do this right, turns out to be more intricate than we had thought originally when issue was labeled as a "good first issue". Let's try to understand what exactly isn't playing nice with types and address it at the core. Happy to help get to the bottom of it. |
|
Looking at |
Thanks! In this case, to correct solution is making sure that the comparison is done correctly without having to change the values. Why is the code considering |
|
I did a debug point of this. I will check it once and let you know. |
|
@suraj-mandal just checking in. It seems there's a conflict to resolve/rebase, but hopefully we can keep pushing this forward :) |
|
Sure |
|
Will resolve the conflicts and start update the PR accordingly |
5 participants
SUMMARY
The following changes have been made:
Upon debugging the models.py file, it was observed that the integer values were not being converted to float, which led to a mismatch among values.
Screenshot of the correct changes after the fix
ADDITIONAL INFORMATION