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

Timeseries query #34

Merged
merged 3 commits into from
May 1, 2023
Merged

Conversation

AlirezaRa94
Copy link
Contributor

Using time series queries to address #29

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

@AlirezaRa94 LGTM except for the comment below.
Is there a reason why this is a draft?
Can you indicate the testing done, so that I can merge it as well?

utils/db_utils.py Show resolved Hide resolved
@shankari
Copy link
Contributor

I don't see any high level issues, and am happy to merge this once you have verified that it works
(aka indicated testing done)

@AlirezaRa94
Copy link
Contributor Author

I don't see any high level issues, and am happy to merge this once you have verified that it works (aka indicated testing done)

I have tested the dashboard with the development docker-compose on both types of authentication and didn't see any errors on different pages.

@AlirezaRa94 AlirezaRa94 marked this pull request as ready for review April 24, 2023 04:32
@shankari
Copy link
Contributor

@AlirezaRa94 have you tested it with the use case of invalid columns?
e.g. what if the user specifies a column that doesn't exist in the data?
This can happen due to misconfiguration, or even for properly configured systems, before the users have entered any labels

@AlirezaRa94
Copy link
Contributor Author

@AlirezaRa94 have you tested it with the use case of invalid columns? e.g. what if the user specifies a column that doesn't exist in the data? This can happen due to misconfiguration, or even for properly configured systems, before the users have entered any labels

Yes, It works in these scenarios, and I have tried to consider them.

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

Looks good in general. @AlirezaRa94 would like to see a detailed "testing done" description and testing screenshots in the future so I know what you mean exactly by "it works in these scenarios".

@shankari shankari merged commit f58a052 into e-mission:master May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants