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

Investigate porting issue #57 #555

Closed
jezdez opened this issue Sep 14, 2018 · 8 comments
Closed

Investigate porting issue #57 #555

jezdez opened this issue Sep 14, 2018 · 8 comments
Labels
Milestone

Comments

@jezdez
Copy link

jezdez commented Sep 14, 2018

Give warning/error msg on inaccurate graph config

#57, PR: #91

Investigating since I'm not sure if there is a similar feature now upstream.

@alison985
Copy link

@jasonthomas Can you please re-enable my Athena connection so I can run tests cases on this?

@jasonthomas
Copy link
Member

@alison985 sent you credentials via email.

@rafrombrc rafrombrc added this to the 19 milestone Nov 9, 2018
@alison985
Copy link

I've been going back and forth with Jason. Asked my next question.

@alison985
Copy link

I can test this on my local with upstream master code now.

In PR #91 I implemented 2 warnings: warning_three_column_groupby and warning_three_column_stacking.

The original query with the problem in #57 has 2 dimensions and 1 measure. When I play with making charts based on that outcome I get an inaccurate chart if I don't have a groupby. The chart will work with or without stacking so I won't port that warning.

@alison985
Copy link

PR upstream getredash#3115

@alison985 alison985 added in review in review upstream PR in review in getredash/redash and removed in progress labels Nov 25, 2018
@jezdez
Copy link
Author

jezdez commented Jan 16, 2019

Our PR has been made moot and this is most likely going to be fixed in getredash#1742 by adding the ability to aggregate same values.

I'm not sure if we should continue having this feature, since porting to redash-stmo is much harder given the code path involved. @emtwo @washort Do you have an opinion on this?

@jezdez jezdez added blocked and removed in review upstream PR in review in getredash/redash wontfix labels Jan 16, 2019
@washort
Copy link

washort commented Jan 16, 2019

As was pointed out in the upstream PR this does prohibit valid cases right now. I'd consider removing this and perhaps doing some discussion with users about the UX for creating these plots.

@jezdez
Copy link
Author

jezdez commented Jan 23, 2019

We're closing this for now since we think we won't be porting this to upstream but will consider removing the feature once getredash#1742 is fixed differently. We may also just remove it once we get to a fork-free status since it's not a high-prio feature.

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

No branches or pull requests

5 participants