redash icon indicating copy to clipboard operation
redash copied to clipboard

Front end data type display exception

Open mirkan1 opened this issue 1 year ago • 11 comments

This is a fix for https://github.com/getredash/redash/issues/6159

  • [x] Bug Fix

mirkan1 avatar Apr 13 '24 19:04 mirkan1

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. 😉

gaecoli avatar Apr 15 '24 02:04 gaecoli

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

mirkan1 avatar Apr 16 '24 21:04 mirkan1

@gaecoli why is the test is failing?

mirkan1 avatar Apr 19 '24 07:04 mirkan1

@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.

gaecoli avatar Apr 19 '24 08:04 gaecoli

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

justinclift avatar Apr 19 '24 09:04 justinclift

@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 avatar Apr 19 '24 10:04 mirkan1

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

mirkan1 avatar Apr 21 '24 19:04 mirkan1

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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

codecov[bot] avatar May 04 '24 22:05 codecov[bot]

PTAL @justinclift

gaecoli avatar May 06 '24 09:05 gaecoli

@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

mirkan1 avatar May 08 '24 21:05 mirkan1

@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.

eradman avatar May 09 '24 13:05 eradman