[Bugfix] Allowing sqlExpression to be blank

[michellethomas]

If you remove the sql in the custom sql filter, you'll see a message null undefined null. adhocFilter.sqlExpression || translateToSql(adhocFilter, { useSimple: true }) was running translateToSql with simple: true on an empty string. Change it to specifically check for undefined.

@GabeLoins

[michellethomas]

I'm not sure in what cases this would have been undefined if expressionType === sql. Gabe do you have more context here? Is checking for undefined sufficient or do I also need to check for nulls?

[gabe-lyons]

sqlExpression can be set to null here, so you can't predict whether it will be undefined or null

[gabe-lyons]

what if you just removed the useSimple parameter? that might be the problem 🤔

[codecov-io]

Codecov Report

Merging #4991 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #4991   +/-   ##
======================================
  Coverage    77.1%   77.1%           
======================================
  Files          44      44           
  Lines        8636    8636           
======================================
  Hits         6659    6659           
  Misses       1977    1977

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d5195a...ad4912d. Read the comment docs.

[michellethomas]

Like we discussed, I changed this to explicitly check if sqlExpression a string.

{ useSimple: true } is needed because if a metric filter is automatically added as expressionType SQL (in AdhocFilterControl) so when creating the AdhocFilter we need to check to make sure it is actually sql (with an expression).

[gabe-lyons]

LGTM