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

Field Filter Category - Allow no default value for optional clause #5541

Closed
tobbbles opened this issue Jul 21, 2017 · 7 comments
Closed

Field Filter Category - Allow no default value for optional clause #5541

tobbbles opened this issue Jul 21, 2017 · 7 comments
Assignees
Labels
Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/Native The SQL/native query editor Querying/Parameters & Variables Filter widgets, field filters, variables etc. Type:Bug Product defects
Milestone

Comments

@tobbbles
Copy link
Contributor

Category Field Filters should allow no default option, so they can be used properly in optional clauses

Reproduce

Example clause:

[[ AND id IN (SELECT id FROM `dataset.with-id-and-name` WHERE {{name}} LIMIT 1) ]]

These field filter settings
2017-07-21-114031_265x308_scrot

This 'optional clause' is always included in the native query, even when the widget has no value set.

Desired Behaviour

There should be a "No Default" field, which is the default (default no default 🤔)

EG:
2017-07-21-114832_206x368_scrot 2017-07-21-114918_263x75_scrot

When "No Default" is used on this widget, then the clause should be optional and excluded from the native query that's constructed, similar to other, non-fieldfilter widgets.

@tobbbles
Copy link
Contributor Author

Here's some handy debug logs, with some info redacted.

Query:

#standardSql
SELECT sum(wins) AS wins
FROM `data.secretSauce*`
WHERE {{dateRange}}
[[ AND id IN (SELECT id FROM `data.sauceMakers` WHERE {{name}} LIMIT 1) ]]
ORDER BY wins DESC

Debug:

{:query
 "#standardSql\nSELECT sum(wins) AS wins\nFROM `aggregate.data*`\nWHERE CAST(date(ts) AS timestamp) BETWEEN ? AND ?\n AND id IN (SELECT id FROM `aggregate.sauceMakers` WHERE 1 = 1 LIMIT 1) \n\nORDER BY wins DESC",
 :template_tags
 {:name {:id "14762412-b62d-DOOT-2f37-863ddad99b84", :name "name", :display_name "Sauce Maker", :type "dimension", :dimension ["field-id" 13330], :widget_type "category"},
  :dateRange
  {:id "333e696b-e258-1dd9-f8b5-e8e4354b9622", :name "dateRange", :display_name "Daterange", :type "dimension", :dimension ["field-id" 1050], :widget_type "date/relative", :default "yesterday"}},
 :params (#inst "2017-06-01T00:00:00.000000000-00:00" #inst "2017-06-30T00:00:00.000000000-00:00")}

The bit to focus on here is the subselect from [[ ... WHERE {{name}} .. ]], as you can see the parameter is substituted with WHERE 1 = 1 `, which shouldn't be the case; that whole clause should be emitted.

@tobbbles
Copy link
Contributor Author

Looks like it's related to this area of code: https://github.com/metabase/metabase/blob/master/src/metabase/query_processor/middleware/parameters/sql.clj#L165

@camsaul As you were the author of this bit, wanna weigh into what you think's up? 😸

@tobbbles
Copy link
Contributor Author

Digging into the frontend a little I can see that the ParameterValueWidget for the variable's default value for the Category dimension has value: null.

Whereas if I checkout the the ParameterValueWidget for say, DateRange, there is no value field.

This seems to add up to my comment ☝️ addressing what if value is specified but is 'nil'?

@maliayas
Copy link
Contributor

I had another usecase that lead to this bug. See #8112

@Ucinorn
Copy link

Ucinorn commented Mar 26, 2019

I'd like to bump this bug, as I'm seeing the same behaviour, and I notice that in both cases the variable is within a subquery:

SELECT orig.year as year, orig.quarter as quarter, orig.month as month, orig.sum as current_sum, orig.count as current_count, orig.average as current_avg, FROM ( SELECT year(event_start_datetime) as year, quarter(event_start_datetime) as quarter, month(event_start_datetime) as month, sum(cost) AS sum, count(cost) AS count, sum(cost) / count(cost) AS average FROM _reports_booked_jumpers WHERE 1 = 1 [[AND _reports_booked_jumpers.venue = {{venue}}]] [[AND _reports_booked_jumpers.sales_category = {{sales_category}}]] GROUP BY year, quarter, month ORDER BY year DESC, quarter DESC, month DESC ) as orig

And the query i get in the returned is below, not the use of AND venue = 1 = 1 instead of just omitting the whole line:

  orig.year as year, 
  orig.quarter as quarter, 
  orig.month as month, 
  orig.sum as current_sum, 
  orig.count as current_count, 
  orig.average as current_avg, 
  prevyear.sum as prevyear_sum, 
  prevyear.count as prevyear_count, 
  prevyear.average as prevyear_avg
FROM 
(
  SELECT
    year(event_start_datetime) as year, 
    quarter(event_start_datetime) as quarter, 
    month(event_start_datetime) as month, 
    sum(cost) AS sum, 
    count(cost) AS count, 
    sum(cost) / count(cost) AS average 
  FROM 
    _reports_booked_jumpers 
    WHERE 1=1
      AND venue = 1 = 1
  GROUP BY 
    year, 
    quarter, 
    month 
  ORDER BY 
    year DESC, 
    quarter DESC, 
    month DESC
) as orig

@lp-lima
Copy link

lp-lima commented May 13, 2019

Community post about the same issue:

https://discourse.metabase.com/t/option-clause-for-field-filter-in-native-sql-parameter/6193

This would allow us dinamically hide or show columns according to the user selection

@camsaul camsaul self-assigned this Sep 20, 2019
@camsaul camsaul added Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Type:Bug Product defects labels Sep 20, 2019
@camsaul camsaul added this to P1 Bugs in Bug Bashing Bonanza via automation Sep 20, 2019
@camsaul camsaul added this to the 0.33.4 milestone Sep 20, 2019
@camsaul
Copy link
Member

camsaul commented Sep 20, 2019

Fixed by #10947

@camsaul camsaul closed this as completed Sep 20, 2019
Bug Bashing Bonanza automation moved this from P1 Bugs to Done Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/Native The SQL/native query editor Querying/Parameters & Variables Filter widgets, field filters, variables etc. Type:Bug Product defects
Projects
No open projects
Development

No branches or pull requests

6 participants