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

Feature/query based parameter #1928

Merged
merged 5 commits into from Sep 13, 2017

Conversation

Projects
None yet
9 participants
@rohithmenon
Contributor

rohithmenon commented Aug 14, 2017

Feature: Query based parameter (drop-down)

The feature is useful for our use-cases where parameters tend to be dynamic and comes from other queries. We have couple of use-cases where we will need such support. This is a feature available in a comparable product offering from Mode analytics (https://help.modeanalytics.com/articles/using-dynamic-parameters/).

The implementation is basically to use a saved query within redash as a parameter. Restriction for this query is that only first column will be used for options of the parameter and it must of a string format. By using saved queries for parameter, we get all the goodies of redash queries (caching, refreshing etc.)

@arikfr

This is really awesome!! Been looking forward for this :-)

There are two small things that popped right out, but there are a few bigger fixes -- like convert the use of controller into a separate component. I'm not sure how much you're experienced with Angular or how much time you have... will you have the time to do this or prefer that I merge it as is?

this.parameter = this.resolve.parameter;
this.onQuerySelect = (query) => {
this.parameter.query = query;

This comment has been minimized.

@arikfr

arikfr Aug 15, 2017

Member

Wouldn't it be more efficient to save only the query.id here?

This comment has been minimized.

@rohithmenon

rohithmenon Aug 15, 2017

Contributor

First of all, thanks for review. I am totally new to frontend stuff, and this seemed like something we needed and hence gave it a shot. My knowledge of angular is as much as w3school, stackoverflow and reading redash code.

Regarding the query.id, I was assuming query.id would be sufficient, but I was not sure if the watch on paramter would trigger when the query changes.

This comment has been minimized.

@arikfr

arikfr Aug 15, 2017

Member

👍

I think the putting only the id should be enough for the watch, if you encounter issues -- let me know and I will give a look.

<input ng-switch-default type="{{param.type}}" class="form-control" ng-model="param.ngModel">
</span>
</div>
</

This comment has been minimized.

@arikfr

arikfr Aug 15, 2017

Member

deleted by mistake?

This comment has been minimized.

@rohithmenon

rohithmenon Aug 15, 2017

Contributor

Good catch. Not sure how to catch this in dev. It did not break stuff when I deployed it locally. Are there some good tricks? I realize that frontend does not have good test coverage, but are there tools that I can use?

This comment has been minimized.

@arikfr

arikfr Aug 15, 2017

Member

Because it works, tests might not help here either. But there might be a linter for HTML we can add to the build process.

@awm33

This comment has been minimized.

awm33 commented Aug 15, 2017

OMG yes! Would love this feature :)

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Aug 15, 2017

convert the use of controller into a separate component. I'm not sure how much you're experienced with Angular or how much time you have...
I can do that. Like mentioned before controller examples I saw on the web used ng-controller. I thought about refactoring it into a component with template for query-based dropdown, but did not do that because it was pretty small and contained within the scope of parameter-settings. That being said, I don't know when to choose between the two. Let me know what you think.

will you have the time to do this or prefer that I merge it as is?
I should be able to convert it into a component. There are enough examples of code snippets within redash. Let me know of your thoughts for components.

@arikfr

This comment has been minimized.

Member

arikfr commented Aug 15, 2017

Components are relatively new feature in Angular. It's been around for over a year, but most examples predate it...

In new code, I tend to encapsulate everything in their own components. It's something I borrowed from React mostly, but seems to make sense in Angular too. In React it's simpler to do, due to the lightweight syntax but maybe I will create some helper functions for lightweight components in Angular/Redash.


Another thing I was thinking about is to improve the heuristics for picking the values from the query:

  • If the query has a single column - use it for both value and name.
  • If the query has a column named value and another named name, then use the first for value and the later for name.
  • If the query has more than one column - use the first as value and name and ignore the rest.

How's that sounds?

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Aug 16, 2017

Fair enough. I will try to make query-dropdown a component.


About heuristics, I did not follow the name/value part. I got the idea of dropdown from values and to check for a column named value if there are more than one columns.

The feature looks pretty similar to dropdown values except that instead of manually entering values in the textarea, it gets it from the output of a query. The concept of name is not very clear to me.

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Aug 16, 2017

Parameter Settings
image


Dropdown
image

@arikfr

This comment has been minimized.

Member

arikfr commented Aug 16, 2017

About heuristics, I did not follow the name/value part. I got the idea of dropdown from values and to check for a column named value if there are more than one columns.

The idea is that sometimes you would like to display a different value to the user than the one you use in a query. Think of users for example -- you would like to show in the dropdown the user name, while use the user id as the value for the parameter.

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Aug 16, 2017

Sounds reasonable. Will add.

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Aug 18, 2017

@arikfr Lemme know if the changes look good.

@azymnis

This comment has been minimized.

azymnis commented Aug 22, 2017

@arikfr any chance we could get this merged? Would be great to have this as a feature in our stack

@arikfr

This comment has been minimized.

Member

arikfr commented Aug 24, 2017

I'm on vacation and will be back next week (29/8). Merging this is going to be one of the first things I do ;)

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Aug 24, 2017

Thanks @arikfr Didn't know that you were out of office.

@highlycaffeinatedcode

This comment has been minimized.

highlycaffeinatedcode commented Aug 28, 2017

Just wondering if queries used for parameters will support parameters themselves?

e.g. if the parameter query were:

select distinct cc.currency_code, cc.currency_name
from currency_codes cc
join transactions t on cc.currency_code = t.currency_code
where t.date = '{{param_date}}'

When viewing a Dashboard for a param date, the dropdown would be filtered to only show the relevant currencies used on that date.

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Aug 28, 2017

A query with parameters will be supported as long as query with parameters has some results with default values. I don't believe your usecase is supported by this change. Params used in drop down does not show up as params themselves.

Also a feature like that would require parameter dependency. Drop-down (A) depends on Drop-down (B). This also means that cyclic dependency should be avoided. This change is pretty simple with the assumption that query results can be used for drop down values and does not recursively check for dependencies.

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Sep 5, 2017

@arikfr Gentle ping

@arikfr arikfr merged commit 398812a into getredash:master Sep 13, 2017

2 checks passed

VersionEye All software dependencies are fine. You are awesome!
Details
codeclimate All good!
Details
@arikfr

This comment has been minimized.

Member

arikfr commented Sep 13, 2017

Merged. Thank you for your patience and for the work done here!

@vabanin

This comment has been minimized.

vabanin commented on 59b7961 Sep 13, 2017

Hi @rohithmenon,

I'm very like this feature and merged your pull request in my clear fork of Redash.
But unfortunately something doesn't work... and I see blank lists in query based drop-downs.
Could you help me to make it work?

Thank you.

This comment has been minimized.

Owner

rohithmenon replied Sep 13, 2017

@vabanin

Are you seeing blank in the drop down of query results that must appear in drop down or in the selector for queries to be used for drop down?

If latter,
Redash starts showing up queries after you enter 3 characters of the query name. Try entering 3 characters that would match the query name.

If former, I would need a little more information:

  1. What are the columns in the output for the query intended for drop-down?

  2. How many rows are present in the output of the query intended for drop-down?

  3. Are there usages of templates in the intended query?
    The above questions should be answerable even without using this feature.

  4. When using this feature, are there any errors that you are able to see on chrome debug console? [Skip this question if you are not familiar with this]

This comment has been minimized.

vabanin replied Sep 13, 2017

@rohithmenon

I'm seeing blank list in drop down of query results.
And I see such picture for all tested queries.
It is for example in case of following simple query:

SELECT 'name1' as name
union all 
SELECT 'name2' as name
union all
SELECT 'name3' as name;

So my answers are following:

What are the columns in the output for the query intended for drop-down?

One text column "name".

How many rows are present in the output of the query intended for drop-down?

3

Are there usages of templates in the intended query?

No

When using this feature, are there any errors that you are able to see on chrome debug console? [Skip this question if you are not familiar with this]

Yes! There are following errors:

angular.js:13920 TypeError: Cannot read property 'id' of undefined
    at parameters.js:49
    at p.$digest (angular.js:17524)
    at p.$apply (angular.js:17790)
    at HTMLSelectElement.<anonymous> (angular.js:31142)
    at Wt (angular.js:3497)
    at HTMLSelectElement.n (angular.js:3485)
(anonymous) @ angular.js:13920 

This comment has been minimized.

Owner

rohithmenon replied Sep 13, 2017

I tested with the same query and works on my local version (latest version of redash with this feature available).

Confirming the steps:

  1. You created a query (similar to the names query above), saved, published and executed.
  2. Selected the above query by query-name in the query selector.
  3. And found the results not appearing in the query drop down?

One way I was able to reproduce no result in drop down is when the query powering the drop down was never executed/refreshed. Try executing the query and try the drop down.

This comment has been minimized.

vabanin replied Sep 14, 2017

You created a query (similar to the names query above), saved, published and executed.

Yes.

Selected the above query by query-name in the query selector.

Yes. And I found that error in chrome debug console mentioned in my previous message appears after this step.

And found the results not appearing in the query drop down?

Yes.

This comment has been minimized.

vabanin replied Sep 14, 2017

Selected the above query by query-name in the query selector.

Yes. And I found that error in chrome debug console mentioned in my previous message appears after this step.

Additional info - error appears after selection parameter type 'Query based dropdown List'.

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Sep 14, 2017

@vabanin From your response, I believe the query is not getting an id assigned which is strange.

Are you on the latest master of redash without no other changes? You don't have to pull my change now that the change is merged to master. Could you try the same steps with just redash master?

@vabanin

This comment has been minimized.

vabanin commented Sep 15, 2017

@rohithmenon

Could you try the same steps with just redash master?

Yes, I have made this on redash master - the result is the same...

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Sep 15, 2017

I believe I know what is happening. Can you try a data source that is not sqlite?

@vabanin

This comment has been minimized.

vabanin commented Sep 15, 2017

Can you try a data source that is not sqlite?

My Redash (it's internal DB) works on Postgres, data sources - Postgres and Redshift.

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Sep 15, 2017

To ensure that the problem that I am able to reproduce with sqlite is the same as that you are facing, could you try this:
In file parameters.js, lines 63 to 74 (https://github.com/getredash/redash/pull/1928/files#diff-9fece99dbc1acba86ef0205621bedb48R63) remove the condition (column.type === 'string')

@vabanin

This comment has been minimized.

vabanin commented Sep 15, 2017

In file parameters.js, lines 63 to 74 (https://github.com/getredash/redash/pull/1928/files#diff-9fece99dbc1acba86ef0205621bedb48R63) remove the condition (column.type === 'string')

Bingo! In drop-down has been appeared the result of the query!

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Sep 15, 2017

I am using BigQuery as the backend, and column.type === 'string' works. In some other backends, it is not the case I believe. If you are up for it, you can send a PR to @arikfr

@rockwotj

This comment has been minimized.

Contributor

rockwotj commented Sep 27, 2017

It seems that sqlite sends a column type of null always, so I am not able to get this to work with sqlite.

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Sep 27, 2017

@rockwotj I have made a PR (#1975) with the fix. Most of my tests were of BigQuery and hence did not realize this.

@vabanin

This comment has been minimized.

vabanin commented Oct 4, 2017

@rohithmenon thank you very much!

@arikfr

This comment has been minimized.

Member

arikfr commented Oct 27, 2017

@rohithmenon
Finally had time to properly review this and found some new things:

  • Everytime you change the param value, it will load again query result (because of the $watch which is triggered for any changed in param).
  • On a related note, we prefer not to use $watch but rather $onChanges.
  • The query name is saved with the parameter. This might create an inconsistency when the query is renamed. Not a big deal, but I think I will just change it to store the query id and load the query name when rendering.
  • When you first setup the parameter, if there is no value for it, there is an empty value selected. Which disappears after you select something (the same happens with the Dropdown List option too). I think we should select the first item in this case.

I will fix those, just wanted to share though.

@rohithmenon

This comment has been minimized.

Contributor

rohithmenon commented Oct 30, 2017

@arikfr Sounds good!

@idoDavid

This comment has been minimized.

idoDavid commented May 6, 2018

Any reason I don't see this "Query based dropdown list" type? I'm on 4.0.1 and I see this:

image

@arikfr

This comment has been minimized.

Member

arikfr commented May 18, 2018

@idoDavid no reason I can think of. Are you sure your UI is updated?

@tiagoajacobi

This comment has been minimized.

tiagoajacobi commented Jun 28, 2018

Hey guys, this feature is being very useful for me.

I would like to discuss the possibility of adding one extra thing on it, which is to make the "value" column variable.

For example I have a query that results 4 columns:

  • The Month which can be the "name"
  • days_in_month
  • transactions
  • amount_spend

So I would like to have only one "Query based dropdown list", but use the different comuns in each parameter of my query. For example:

SELECT {{month_result}}.days_in_month,
       {{month_result}}.transactions,
       {{month_result}}.amount_spend

OR

SELECT {{month_result.days_in_month}},
       {{month_result.transactions}},
       {{month_result.amount_spend}}

Do you think that something like that can be supported?
Thanks

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