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

The seconds format specifier is wrong when outputting to CSV #4895

Closed
Blufe opened this issue May 15, 2020 · 7 comments · Fixed by #4922
Closed

The seconds format specifier is wrong when outputting to CSV #4895

Blufe opened this issue May 15, 2020 · 7 comments · Fixed by #4922

Comments

@Blufe
Copy link

Blufe commented May 15, 2020

Issue Summary

The seconds format specifier is wrong when outputting to CSV.

Steps to Reproduce

  1. Specify "HH:mm:ss.SSS" for Time Format.
  2. Execute a query that uses Time Format (eg. SELECT CURRENT_TIMESTAMP() in BigQuery).
  3. Download CSV file.

Seconds will not be printed, unix time of the date-time data will be printed.
For example, "2020-05-15T19:34:36.664" will output "2020-05-15T19:34:1589538876664".

Technical details:

  • Redash Version: 8.0.2
  • Browser/OS: redash/redash:8.0.2.b37747
  • How did you install Redash:
@susodapop
Copy link
Contributor

This can probably be fixed along with #4820.

@arikfr
Copy link
Member

arikfr commented May 20, 2020

The bug is here:

def _convert_format(fmt):
return (
fmt.replace("DD", "%d")
.replace("MM", "%m")
.replace("YYYY", "%Y")
.replace("YY", "%y")
.replace("HH", "%H")
.replace("mm", "%M")
.replace("ss", "%s")
)

It translates ss to %s, which seems to be the unix timestamp... It also ignores .SSS.

@chulucninh09
Copy link
Contributor

chulucninh09 commented May 22, 2020

Just want to confirm, according to python docs here, the above code should be changed to

 def _convert_format(fmt): 
     return ( 
         fmt.replace("DD", "%d") 
         .replace("MM", "%m") 
         .replace("YYYY", "%Y") 
         .replace("YY", "%y") 
         .replace("HH", "%H") 
         .replace("mm", "%M") 
         .replace("ss", "%S") 
         .replace("SSS", "%f")
     ) 

However, the string will be fixed at 6-digit microsecond, not 3-digit millisecond as the following screenshot
image

If you consider this behavior is not harmful to other functions, I can implement it and make PR.

@arikfr
Copy link
Member

arikfr commented May 26, 2020

If you consider this behavior is not harmful to other functions, I can implement it and make PR.

Considering it currently prints the Unix timestamp, having 6 digits instead of 3 should be a great step forward ;-)

A PR will be appreciated. Thanks!

@kravets-levko
Copy link
Collaborator

However, the string will be fixed at 6-digit microsecond, not 3-digit millisecond as the following screenshot

I guess you can just cut off last three digits after formatting and it will work just fine

@arikfr
Copy link
Member

arikfr commented May 26, 2020

@kravets-levko but then you need to know when microseconds are used or not.. easier to leave as is.

@chulucninh09
Copy link
Contributor

However, the string will be fixed at 6-digit microsecond, not 3-digit millisecond as the following screenshot

I guess you can just cut off last three digits after formatting and it will work just fine

cutting a string after formatting needs extra implementation. I don't think we should do that way. I will stick with 6-digit solution and make a PR

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

Successfully merging a pull request may close this issue.

5 participants