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

PostgreSQL: __unixEpochGroup to support arithmetic expression as argument #46764

Merged
merged 2 commits into from
Mar 26, 2022

Conversation

s0nik42
Copy link
Contributor

@s0nik42 s0nik42 commented Mar 20, 2022

Following macro call generates wrong expression :

$__unixEpochGroupAlias(height+42,$__interval)

=> floor(height+42/60)*60 AS "time"

instead of

=> floor((height+42)/60)*60 AS "time"

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Following call generates wrong expression : 
 
$__unixEpochGroupAlias(height+42,$__interval)

=> floor(height+42/60)*60 AS "time"

instead of

=> floor((height+42)/60)*60 AS "time"
@s0nik42 s0nik42 requested a review from a team March 20, 2022 18:29
@grafanabot grafanabot added datasource/Postgres area/backend pr/external This PR is from external contributor labels Mar 20, 2022
@zoltanbedi
Copy link
Member

Hi @s0nik42,
Thanks for fixing this. Can you please update the failing test and add your use case to it?

@s0nik42
Copy link
Contributor Author

s0nik42 commented Mar 22, 2022

@zoltanbedi sure, just not familiar doing that, can you point me to the test file? I will do the modification

@zoltanbedi
Copy link
Member

zoltanbedi sure, just not familiar doing that, can you point me to the test file? I will do the modification

@s0nik42 I'd change this line https://github.com/grafana/grafana/blob/main/pkg/tsdb/postgres/macros_test.go#L140
"SELECT $__unixEpochGroup(time_column + 1,'5m')" and then we have to change the expected output in here https://github.com/grafana/grafana/blob/main/pkg/tsdb/postgres/macros_test.go#L144

@zoltanbedi zoltanbedi added the no-backport Skip backport of PR label Mar 23, 2022
@zoltanbedi zoltanbedi added this to the 8.5.0 milestone Mar 23, 2022
@CLAassistant
Copy link

CLAassistant commented Mar 23, 2022

CLA assistant check
All committers have signed the CLA.

@s0nik42
Copy link
Contributor Author

s0nik42 commented Mar 23, 2022

@zoltanbedi made the change hope I did it well. One question for my future commit how can I run a specific test to verify everything works before committing ?

@zoltanbedi
Copy link
Member

zoltanbedi made the change hope I did it well. One question for my future commit how can I run a specific test to verify everything works before committing ?

go test -v -run <regexp> would match on test name, as far as I know.

@zoltanbedi zoltanbedi merged commit f4b6c40 into grafana:main Mar 26, 2022
@zoltanbedi
Copy link
Member

Thank you @s0nik42!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/backend datasource/Postgres no-backport Skip backport of PR pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants