Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Front end data type display exception #6888

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mirkan1
Copy link

@mirkan1 mirkan1 commented Apr 13, 2024

This is a fix for #6159

  • Bug Fix

@gaecoli
Copy link
Member

gaecoli commented Apr 15, 2024

Thank you very much for your contribution. To finally solve this problem, you need to introduce the bigint library. The current solution has not completely solved it. 😉

@mirkan1
Copy link
Author

mirkan1 commented Apr 16, 2024

GPT told me something about number.bit_length() > 63 but it is irrelevant

@mirkan1
Copy link
Author

mirkan1 commented Apr 19, 2024

@gaecoli why is the test is failing?

@gaecoli
Copy link
Member

gaecoli commented Apr 19, 2024

@gaecoli why is the test is failing?

https://github.com/getredash/redash/wiki/The-list-of-Redash-make-commands

Your code format does not conform to our style. You should run make format. Also, your solution is not the expected solution, so I don't recommend merging your code. Moreover, you use a double-layer loop to process the data when obtaining the results, which is a very performance-consuming operation.

@justinclift
Copy link
Member

@mirkan1 Was this code written using an LLM (ChatGPT, or any other)?

@mirkan1
Copy link
Author

mirkan1 commented Apr 19, 2024

@justinclift I asked a few question to Chat GPT for BigInt but the other way around is writing a fix into numeral library itself

@mirkan1
Copy link
Author

mirkan1 commented Apr 21, 2024

I wrote a version upgrade for numeral yet I dont know if the maintainer will merge it any time soon...

Copy link

codecov bot commented May 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.83%. Comparing base (10a46fd) to head (3b6ca98).
Report is 2 commits behind head on master.

Current head 3b6ca98 differs from pull request most recent head e7c67a3

Please upload reports for the commit e7c67a3 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6888      +/-   ##
==========================================
- Coverage   63.85%   63.83%   -0.02%     
==========================================
  Files         161      161              
  Lines       13082    13094      +12     
  Branches     1811     1814       +3     
==========================================
+ Hits         8353     8359       +6     
- Misses       4429     4432       +3     
- Partials      300      303       +3     

see 7 files with indirect coverage changes

@mirkan1 mirkan1 requested a review from gaecoli May 4, 2024 22:53
@@ -5,6 +5,7 @@ import { QueryResultError } from "@/services/query";
import { Auth } from "@/services/auth";
import { isString, uniqBy, each, isNumber, includes, extend, forOwn, get } from "lodash";

const JSONbigString = require("json-bigint")({ storeAsString: true });
Copy link
Member

Choose a reason for hiding this comment

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

This code can be changed import style? In order to unify the code style of the entire project, it is recommended to use import to import the package.

Copy link
Member

Choose a reason for hiding this comment

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

And change JSONbigString to jsonParse?

viz-lib/src/visualizations/table/columns/number.tsx Outdated Show resolved Hide resolved
client/app/services/query-result.js Outdated Show resolved Hide resolved
client/app/services/query-result.js Show resolved Hide resolved
@gaecoli
Copy link
Member

gaecoli commented May 6, 2024

PTAL @justinclift

@mirkan1
Copy link
Author

mirkan1 commented May 8, 2024

@eradman would it make any diffirence to store integers as string on api/query_results endpoints?
I dont think if Redash doing any arithmetic equations with the results

@eradman
Copy link
Collaborator

eradman commented May 9, 2024

@eradman would it make any diffirence to store integers as string on api/query_results endpoints?
I dont think if Redash doing any arithmetic equations with the results

There would imply a complicated migration because query_results in the database would have a different storage format. Also numbers stored as JSON in Postgres don't loose precision, so I don't think this would solve anything.

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

Successfully merging this pull request may close these issues.

5 participants