Fix LookML filter-expression to SQL conversion#241
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 345f8c29cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # before / after <value> (date or numeric bound) | ||
| bm = re.match(r"(?i)^(before|after)\s+(.+)$", v) | ||
| if bm: | ||
| op = "<" if bm.group(1).lower() == "before" else ">" |
There was a problem hiding this comment.
For LookML date filters, after 2018-10-05 includes the boundary date per Looker's filter docs (https://docs.cloud.google.com/looker/docs/filter-expressions#date_and_time), but this branch emits > for every after expression. Any imported measure using filters: [created_date: "after 2020-01-01"] will drop rows from 2020-01-01; after should be generated as >= while before remains exclusive.
Useful? React with 👍 / 👎.
| for p in parts: | ||
| cond = single(p) | ||
| is_exclude = (p.startswith("-") and not re.match(r"^-(\d|\.)", p)) or bool(re.match(r"(?i)^not\s", p)) | ||
| (excludes if is_exclude else includes).append(cond) |
There was a problem hiding this comment.
Looker's numeric filter syntax treats NOT 66, 99, 4 as excluding all three values (https://docs.cloud.google.com/looker/docs/filter-expressions#number), but this mixed-list path only marks the first NOT 66 token as an exclusion and ORs the remaining plain values as inclusions, producing (f = 99 OR f = 4) AND f != 66. Measures imported from that documented syntax will count only 99/4 instead of everything except 66/99/4; detect a leading NOT with no other NOTs and emit a negated list/range for all parts.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
345f8c2 to
00ff35d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00ff35dc91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| op = "<" if bm.group(1).lower() == "before" else ">=" | ||
| operand = bm.group(2).strip() | ||
| rhs = operand if is_number(operand) else q(operand) | ||
| return f"{col} {op} {rhs}" |
There was a problem hiding this comment.
When a LookML date filter uses documented relative bounds such as before 3 days ago or after Monday, this branch treats the operand as a literal SQL value before the date-expression warning can run, producing conditions like {model}.created_date < '3 days ago'. On dialects that don't parse those English phrases as dates, imported measures either fail or count the wrong rows; detect relative operands and leave them on the warning/fallback path instead of translating them as absolute bounds.
Useful? React with 👍 / 👎.
| cm = re.match(r"^(>=|<=|!=|<>|>|<)\s*(.+)$", v) | ||
| if cm: | ||
| operator, operand = cm.group(1), cm.group(2).strip() |
There was a problem hiding this comment.
When a numeric filter uses Looker's documented AND range syntax inside one condition, for example >1 AND <100, NOT 2, this regex captures 1 AND <100 as the operand for a single > comparison and quotes it because it isn't a number. The mixed-list path then emits a number-to-string comparison instead of field > 1 AND field < 100, so imported filtered measures fail or return nonsense for that supported syntax; split/parse AND subconditions before applying the single-comparison fallback.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
00ff35d to
0c91a36
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c91a360a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Comma-separated list | ||
| if "," in value: | ||
| parts = [p.strip() for p in value.split(",") if p.strip() != ""] |
There was a problem hiding this comment.
Looker's number filter docs (https://docs.cloud.google.com/looker/docs/filter-expressions#number) support interval lists such as [0,9],[20,29] and [0,10],20, but this unconditional split treats the commas inside each interval as list separators. Those fragments then flow through the plain/mixed list paths and produce string IN values like '[0'/'9]' instead of ORed numeric ranges, so imported filtered measures with these documented filters count the wrong rows.
Useful? React with 👍 / 👎.
| return f"{col} NOT LIKE {q(neg)}" | ||
| if is_number(neg): | ||
| return f"{col} != {neg}" | ||
| return f"{col} != {q(neg)}" |
There was a problem hiding this comment.
When a numeric filter uses Looker's documented NOT 3 to 80.44 form (https://docs.cloud.google.com/looker/docs/filter-expressions#number), the NOT branch strips the prefix but then treats 3 to 80.44 as an ordinary string because it is neither a wildcard nor a number. That emits {model}.f != '3 to 80.44' instead of the inverted range (< 3 OR > 80.44), so any imported measure using this filter returns incorrect rows.
Useful? React with 👍 / 👎.
| # Numeric AND range in a single condition, e.g. ">1 AND <100". | ||
| if re.search(r"(?i)\sand\s", v): | ||
| subs = [s.strip() for s in re.split(r"(?i)\s+and\s+", v)] | ||
| if len(subs) > 1 and all(re.match(r"^(>=|<=|!=|<>|>|<)", s) for s in subs): |
There was a problem hiding this comment.
For documented numeric filters such as >10 AND <=20 OR 90 (https://docs.cloud.google.com/looker/docs/filter-expressions#number), this check accepts the second subcondition because re.match only verifies that it starts with an operator. single('<=20 OR 90') then emits {model}.f <= '20 OR 90' instead of ORing the exact value, so LookML filters that use Looker's OR syntax return the wrong rows; split OR clauses first or require full matches here.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
0c91a36 to
8654cb6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8654cb639c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if re.search(r"(?i)\sor\s", v): | ||
| or_parts = [s.strip() for s in re.split(r"(?i)\s+or\s+", v)] | ||
| if len(or_parts) > 1 and all(_is_numeric_clause(p) for p in or_parts): | ||
| return "(" + " OR ".join(single(p) for p in or_parts) + ")" |
There was a problem hiding this comment.
When a Looker numeric filter uses the documented natural-language OR form 3 to 10 OR 30 to 100, _is_numeric_clause() accepts each branch because _numeric_range_bounds() is truthy, but this recursive single(p) call bypasses the top-level range handling at lines 412-415. The result is f = '3 to 10' OR f = '30 to 100' instead of two range predicates, so imported filtered measures return wrong rows; route range branches through _range_sql() before recursion.
Useful? React with 👍 / 👎.
| if is_number(neg): | ||
| return f"{col} != {neg}" | ||
| return f"{col} != {q(neg)}" |
There was a problem hiding this comment.
When the filter is the documented numeric form NOT NULL, this negation branch sets neg to NULL and then falls through to != 'NULL' because only numeric operands get special handling here. On numeric columns that can error due to casting, and on string columns it excludes the literal NULL while relying on SQL null-comparison behavior; NOT NULL should emit IS NOT NULL like -NULL.
Useful? React with 👍 / 👎.
| if operand.replace(".", "").replace("-", "").isdigit(): | ||
| return f"{{model}}.{field} {operator} {operand}" | ||
| s = s.strip() | ||
| m = re.match(r"^([\[\(])\s*(-?\d*\.?\d*)\s*,\s*(-?\d*\.?\d*)\s*([\]\)])$", s) |
There was a problem hiding this comment.
Looker supports explicit infinity in interval notation, such as (500, inf) and (-inf, 10], but this parser only allows digit or empty bounds. Those values therefore miss range handling and later fall into the comma-list split, producing string IN fragments rather than open-ended comparisons; accept inf and -inf as open bounds here.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
8654cb6 to
9c31524
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c31524b27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lead_not = re.match(r"(?i)^not\s+(.+)$", value) | ||
| if lead_not: | ||
| neg_parts = [p.strip() for p in lead_not.group(1).split(",") if p.strip() != ""] | ||
| if neg_parts and all(is_plain(p) for p in neg_parts): |
There was a problem hiding this comment.
When the documented numeric form NOT >1, 2, <100 is imported, this all(is_plain(...)) guard rejects the leading-NOT path because the list contains comparison operators, so the mixed-list fallback renders NOT >1 as a string inequality and ORs the remaining positive clauses. That broadens the filter to values under 100 instead of negating each numeric condition as Looker does; handle leading NOT before the plain-value restriction for comparison/range operands too.
Useful? React with 👍 / 👎.
| _DATE_FILTER_RE = re.compile( | ||
| r"(?i)\b(ago|day|days|week|weeks|month|months|year|years|quarter|quarters|" | ||
| r"hour|hours|minute|minutes|second|seconds|today|yesterday|tomorrow|now|fiscal|" | ||
| r"week|month|year)\b" |
There was a problem hiding this comment.
Looker supports weekday date expressions such as Monday, before Monday, and after Monday, but weekday names are absent from this warning detector. Fresh evidence after this patch is that the new before/after guard refuses to translate after Monday, then the fallback emits a literal equality without the warning path, so imported date filters silently match the string 'after Monday' instead of the intended relative weekday window.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
9c31524 to
ab41cd8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab41cd801d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Split only on top-level commas so commas inside brackets are preserved. | ||
| if ("[" in value or "(" in value) and "," in value: | ||
| segs = self._split_top_level_commas(value) | ||
| if any(self._numeric_range_bounds(s) for s in segs): |
There was a problem hiding this comment.
With a documented numeric filter such as NOT (3,12) (https://docs.cloud.google.com/looker/docs/filter-expressions#number), _split_top_level_commas() returns one segment but this guard tests _numeric_range_bounds() before removing the leading NOT, so it skips interval handling and the later value.split(',') path emits {model}.f NOT IN ('(3', '12)'). Any imported measure using a negated interval filter will compare string fragments instead of applying the inverted numeric interval; handle NOT intervals before the raw comma-list fallback.
Useful? React with 👍 / 👎.
| if is_number(neg): | ||
| return f"{col} != {neg}" | ||
| return f"{col} != {q(neg)}" |
There was a problem hiding this comment.
For a numeric filter like NOT >1 (the single-condition form of Looker's documented leading-NOT syntax), this negation branch never recognizes comparison operands, so neg is not a number/range and the fallback emits {model}.f != '>1'. On numeric fields that is a type-mismatched comparison or the wrong predicate instead of <= 1; route neg through the comparison operator-flip logic used for leading-NOT lists before falling back to string inequality.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
ab41cd8 to
0325713
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03257137b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| nm = re.match(r"(?i)^not\s+(.+)$", v) | ||
| neg = nm.group(1).strip() if nm else None |
There was a problem hiding this comment.
NOT parsing to actual filter syntax
Because this helper is used for every LookML filter without knowing the field type, treating any token that starts with not as negation changes valid string values such as filters: [status: "not started"]: Looker's string filter docs describe bare strings as exact matches and use -FOO for string negation, while the NOT examples are in the number-filter section (https://docs.cloud.google.com/looker/docs/filter-expressions). This now imports that filter as {model}.status != 'started' instead of matching the literal status, so NOT should only be consumed for the numeric/null/empty/range forms you can positively recognize.
Useful? React with 👍 / 👎.
| return f"{{model}}.{field} = '{value}'" | ||
| lead_not = re.match(r"(?i)^not\s+(.+)$", value) | ||
| if lead_not: | ||
| neg_parts = [p.strip() for p in lead_not.group(1).split(",") if p.strip() != ""] |
There was a problem hiding this comment.
Fresh evidence after this patch is that the new pre-split guard handles only a whole-value NOT (3,12), but this leading-NOT list path still splits on every comma. For supported combinations like NOT [0,10],20 or NOT (3,12), 20, the comma inside the interval is split into fragments, so the converter emits string NOT IN values such as '[0'/'10]' instead of negating the interval and the extra value per Looker's leading-NOT list rules; use the top-level comma splitter before building neg_parts.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
0325713 to
87fbf6d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87fbf6d408
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return f"{col} != {p}" | ||
| return None | ||
|
|
||
| lead_not = re.match(r"(?i)^not\s+(.+)$", value) |
There was a problem hiding this comment.
Fresh evidence after the single-value not started fix: this branch still runs before knowing the field is numeric and before the plain string-list path, so a valid string filter like filters: [status: "not started,pending"] is imported as status NOT IN ('started', 'pending'). Looker string filters treat comma-separated bare tokens as exact alternatives and use -FOO for string negation, so this flips both the not started value and the rest of the list instead of matching either string.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
87fbf6d to
0b87991
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b8799152f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Split only on top-level commas so commas inside brackets are preserved. | ||
| if ("[" in value or "(" in value) and "," in value: | ||
| segs = self._split_top_level_commas(value) | ||
| if any(self._numeric_range_bounds(s) for s in segs): |
There was a problem hiding this comment.
Fresh evidence in the current code is that a filter like filters: [amount: "NOT [0,10], 20 to 30"] enters this positive range-list block before the leading-NOT branch because any(...) is true for the second to range. That ORs the inverted first interval with the positive 20 to 30 range, so values from 20 through 30 are included even though Looker's number-filter rules negate all conditions when only the first condition has NOT; handle leading NOT before this block or keep NOT ... segments out of the positive list path.
Useful? React with 👍 / 👎.
| bm = re.match(r"(?i)^(before|after)\s+(.+)$", v) | ||
| if bm: | ||
| operand = bm.group(2).strip() | ||
| if is_number(operand) or re.match(r"^\d{4}", operand): |
There was a problem hiding this comment.
This treats any operand that starts with four digits as an absolute bound, so documented relative/offset date expressions such as before 2016-1 are translated to {model}.created_date < '2016-1' instead of being left on the untranslated date-expression path. Looker only treats absolute dates as the documented full YYYY-MM-DD or YYYY/MM/DD forms with two-digit month/day, so this branch can silently count the wrong rows for imported date filters using truncated offset syntax.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
0b87991 to
7b09cac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b09cac6bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if parts and all(is_plain(p) for p in parts): | ||
| if all(is_number(p) for p in parts): | ||
| return f"{col} IN ({', '.join(parts)})" | ||
| return f"{col} IN ({', '.join(q(p) for p in parts)})" |
There was a problem hiding this comment.
Looker supports comma-separated date OR filters such as today, 7 days ago, but this plain-list branch runs before single() can hit the date-expression warning and emits {model}.created_date IN ('today', '7 days ago'). For imported measure filters on date fields, warehouses will compare the column to English date phrases with no warning instead of applying Looker's relative-date logic, so date-looking list parts need to be routed through the same warning/fallback path rather than treated as plain strings.
Useful? React with 👍 / 👎.
| ncmp = re.match(r"^(>=|<=|!=|<>|>|<)\s*(.+)$", neg) | ||
| if ncmp: | ||
| _flip = {">": "<=", ">=": "<", "<": ">=", "<=": ">", "!=": "=", "<>": "="} | ||
| nop, noperand = ncmp.group(1), ncmp.group(2).strip() | ||
| return f"{col} {_flip[nop]} {noperand if is_number(noperand) else q(noperand)}" |
There was a problem hiding this comment.
For a numeric filter like NOT >1 AND <100, this re.match accepts the entire AND range as the operand of the first > comparison and returns {model}.f <= '1 AND <100'. Looker's number filters support AND ranges and leading NOT negates the condition, so imported measures with that filter generate a string comparison instead of the outside-of-range predicate; parse the AND range before this single-comparison flip or require a full comparison match here.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
7b09cac to
3e60168
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e6016814e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "%" in neg or "_" in neg: | ||
| return f"{col} NOT LIKE {q(neg)}" |
There was a problem hiding this comment.
Fresh evidence in this revision is that the bare-string not started path was fixed, but this wildcard branch still consumes word-NOT before the converter knows the field is numeric. For a valid string filter such as filters: [status: "not %complete%"], Looker string syntax treats the value as a positive wildcard pattern starting with not (string negation uses -FOO), while this emits status NOT LIKE '%complete%' and excludes every value containing complete; only numeric/null/range NOT forms should be handled here.
Useful? React with 👍 / 👎.
| try: | ||
| float(s) | ||
| return True |
There was a problem hiding this comment.
Because float() accepts non-finite tokens like nan, inf, and Infinity, a valid string LookML filter such as filters: [status: "nan"] now passes _filter_is_number() and later emits {model}.status = nan without quotes. This converter does not know the field type, and Looker only uses inf as an interval bound, so the numeric check should be limited to actual decimal numeric syntax to keep these string values quoted.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
3e60168 to
3206828
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3206828e1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| so a string filter like ``status: "nan"`` must stay quoted, not emit ``= nan``. | ||
| """ | ||
| try: | ||
| return math.isfinite(float(s)) |
There was a problem hiding this comment.
Fresh evidence after the non-finite guard is that float() still accepts finite Python-specific spellings such as 1_000 and 1e2, so a valid string filter like filters: [status: "1e2"] is now emitted as {model}.status = 1e2 instead of a quoted literal. Because this converter does not know the field type, these values should not be classified as numeric unless they match the decimal grammar the emitted SQL can safely preserve; otherwise string codes can change meaning or generate invalid SQL on warehouses that reject underscores in numeric literals.
Useful? React with 👍 / 👎.
| includes, excludes = [], [] | ||
| for p in parts: | ||
| cond = single(p) | ||
| is_exclude = (p.startswith("-") and not re.match(r"^-(\d|\.)", p)) or bool(re.match(r"(?i)^not\s", p)) |
There was a problem hiding this comment.
For string filter lists where a non-first value begins with not, such as filters: [status: "pending,not started"], single() correctly leaves not started as a literal equality, but this classifier still puts that condition in the exclusion bucket. The result is status = 'pending' AND status = 'not started' instead of an OR/list match, dropping all rows for valid string statuses; word NOT should only mark exclusions here when the token was positively parsed as a numeric/null/range negation.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
3206828 to
f298a34
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f298a341b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for s in segs: | ||
| r = self._numeric_range_bounds(s) | ||
| parts_sql.append(self._range_sql(r, col) if r else single(s)) | ||
| return "(" + " OR ".join(parts_sql) + ")" |
There was a problem hiding this comment.
When a range/interval list also contains a non-leading exclusion, such as filters: [amount: "[0,30], NOT 20"], this range-list shortcut runs before the mixed include/exclude logic and ORs the negated condition with the range, yielding (amount BETWEEN 0 AND 30 OR amount != 20). That admits almost every value outside the intended range; Looker's numeric comma syntax combines positive alternatives and then ANDs NOT exclusions, so these imported filtered measures can be materially overcounted. Skip this OR-only path when any top-level segment is an exclusion.
Useful? React with 👍 / 👎.
| # instead of silently emitting IN ('today', '7 days ago'). | ||
| if any(self._DATE_FILTER_RE.search(p) for p in parts): | ||
| return "(" + " OR ".join(single(p) for p in parts) + ")" | ||
| return f"{col} IN ({', '.join(q(p) for p in parts)})" |
There was a problem hiding this comment.
For comma-separated natural-language ranges like filters: [amount: "1 to 10, 20 to 30"], the bracket-only range-list special case above does not run, so this plain-list return emits amount IN ('1 to 10', '20 to 30') instead of ORing two numeric range predicates. Looker supports a to b numeric ranges and comma-separated numeric alternatives, so measures using those imported filters compare strings and return wrong rows.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
f298a34 to
9669a0a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9669a0a7db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rhs = operand if is_number(operand) else q(operand) | ||
| return f"{col} {op} {rhs}" | ||
|
|
||
| _cmp = r"(>=|<=|!=|<>|>|<)\s*-?\d+(\.\d+)?" |
There was a problem hiding this comment.
Because this regex requires a digit before the decimal point, it disagrees with _filter_is_number() and the single-comparison path, which both accept bare fractions like .5. A numeric filter such as filters: [amount: ">.5 AND <1"] therefore fails the AND-range branch and falls through to the single-comparison regex, emitting amount > '.5 AND <1' instead of two numeric predicates, so measures with fractional bounds can fail or miscount; use the same numeric-literal grammar for AND subconditions.
Useful? React with 👍 / 👎.
| re.fullmatch(_cmp, s) | ||
| or self._numeric_range_bounds(s) | ||
| or is_number(s) | ||
| or re.search(r"(?i)\sand\s", s) |
There was a problem hiding this comment.
This makes _is_numeric_clause() return true for any OR arm that merely contains the word and, even if it is not a numeric comparison/range. The added test protects cats OR dogs as a literal string, but filters: [status: "red and blue OR 90"] is still split into status = 'red and blue' OR status = 90 instead of matching the literal value; require the AND arm to fully parse as numeric subconditions before enabling the numeric OR path.
Useful? React with 👍 / 👎.
| cm2 = re.match(r"^(>=|<=|!=|<>|>|<)\s*(.+)$", p) | ||
| if cm2: | ||
| op, operand = cm2.group(1), cm2.group(2).strip() | ||
| return f"{col} {_flip[op]} {operand if is_number(operand) else q(operand)}" |
There was a problem hiding this comment.
Fresh evidence in this revision is that the list-specific negate() helper still applies a prefix comparison match before handling compound ranges. For a numeric filter like filters: [amount: "NOT >1 AND <100, 200"], this path emits amount <= '1 AND <100' AND amount != 200 instead of negating the AND range and excluding 200, so imported filtered measures can fail type comparisons or count the wrong rows; handle AND ranges here before the single-comparison match or require a full match.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
9669a0a to
08f2939
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08f2939ef4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| neg_dash = ( | ||
| v[1:] if (v.startswith("-") and len(v) > 1 and not re.match(r"^-(>=|<=|!=|<>|>|<|\d|\.)", v)) else None |
There was a problem hiding this comment.
When a valid string exclusion starts with a period, such as filters: [ext: "-.csv"], this guard suppresses dash-negation just because the next character is ., so single() falls through to {model}.ext = '-.csv' instead of excluding .csv. Looker string filters use a leading - for negation, while literal leading hyphens are escaped, so this regression flips valid dot-prefixed exclusions; -.5 can be handled by checking the whole token as numeric rather than blocking every -. value.
Useful? React with 👍 / 👎.
| operand.upper() in ("NULL", "EMPTY") | ||
| or is_number(operand) | ||
| or self._numeric_range_bounds(operand) | ||
| or re.match(r"^(>=|<=|!=|<>|>|<)\s*-?\d", operand) |
There was a problem hiding this comment.
For a numeric comma filter with a non-leading fractional NOT comparison, for example 1, NOT >.5, single() renders NOT >.5 as {model}.f <= .5, but this classifier returns false because it only recognizes an operator followed by a digit. The mixed-list path then puts that condition in the include bucket and ORs it with {model}.f = 1 instead of ANDing it as an exclusion, so filters using the bare-fraction syntax supported above can overcount; use the same _cmp / _is_numeric_and_range grammar here.
Useful? React with 👍 / 👎.
The LookML adapter treated Looker filter values as opaque strings, so anything beyond simple comparisons produced wrong or invalid SQL: - date/numeric ranges and intervals ([1,10], "5 to 10") became string equality or malformed IN lists (matched zero rows) - "not X" / negation -> literal "= 'not X'" - EMPTY -> omitted the NULL case - single quotes were not escaped (broken SQL / injection risk) Rewrite _convert_lookml_filter_to_sql to implement the representable parts of Looker's filter expression language (numeric ranges, interval brackets, NOT/negation, EMPTY = NULL OR '', wildcard NOT LIKE, mixed lists) and escape quoted string literals. Untranslated date/interval expressions now log a warning instead of silently emitting a zero-row equality.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
08f2939 to
7421d74
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7421d74896
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # leading-NOT branch -- otherwise "NOT [0,10], 20 to 30" would mis-handle here. | ||
| if "," in value and not re.match(r"(?i)^not\s", value): | ||
| segs = self._split_top_level_commas(value) | ||
| if any(self._numeric_range_bounds(s) for s in segs): |
There was a problem hiding this comment.
For numeric filters like 20, NOT [0,10], segs contains NOT [0,10], but this gate only looks for positive ranges, so it falls through to the raw value.split(",") below and splits the interval into NOT [0 / 10]. Looker supports interval notation and non-leading NOT exclusions, so this emits string equalities instead of combining the positive value with the inverted interval; treat NOT <range> as a range-list case before the raw split.
Useful? React with 👍 / 👎.
| if p.startswith("-") and len(p) > 1 and not re.match(r"^-(\d|\.)", p): | ||
| return True |
There was a problem hiding this comment.
Fresh evidence in the current code is that the single-token -.csv path was fixed, but this classifier still rejects every -. token. In a valid string filter such as FOO%,-.csv, single("-.csv") returns ext != '.csv' but _is_exclusion() returns false, so the mixed-list combiner ORs it with the include and produces (ext LIKE 'FOO%' OR ext != '.csv'), admitting almost every value instead of applying the exclusion.
Useful? React with 👍 / 👎.
…and dot-dash strings
Two numeric/string filter-list bugs where an exclusion was silently ORed in
(admitting almost every value) or an interval was shattered:
- _is_exclusion rejected dot-prefixed dash strings (-.csv) because its guard
blocked every '-.' token, disagreeing with single() which negates them. In a
mixed list like 'FOO%,-.csv' the exclusion was ORed -> matched everything.
Mirror single()'s neg_dash guard exactly (only negative numbers / -<op> forms
are non-exclusions).
- A negated interval in a non-leading list position ('20, NOT [0,10]') did not
route through the bracket-aware range-list branch, so the naive comma split
broke it into 'NOT [0' / '10]'. Recognize negated ranges as range-ish so the
interval's inner comma is preserved and it is excluded (AND).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ca8e93a0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # AND them (De Morgan), e.g. "NOT >1, [0,10]" -> (<=1 AND (<0 OR >10)). | ||
| negated = [negate(p) for p in neg_parts] | ||
| if neg_parts and all(n is not None for n in negated): | ||
| return "(" + " AND ".join(negated) + ")" |
There was a problem hiding this comment.
When importing Looker's documented numeric filter NOT >1, 2, <100 on a field that can contain NULLs, this path returns f <= 1 AND f != 2 AND f >= 100, which SQL evaluates to no rows and also filters out NULLs. Looker's number-filter docs state that this impossible leading-NOT list is instead written as IS NULL (https://docs.cloud.google.com/looker/docs/filter-expressions#number), so filtered measures using this form will undercount NULL rows unless the impossible all-negated case is detected before joining the clauses.
Useful? React with 👍 / 👎.
Looker documents that an impossible all-negated numeric list -- e.g. "NOT >1, 2, <100" (De Morgan: <=1 AND >=100, which no value satisfies) -- is written as IS NULL rather than an always-false predicate. The always-false AND also excludes NULL rows, so filtered measures using this form undercounted nulls. Detect an empty numeric intersection across the simple comparison clauses of a leading-NOT list and emit IS NULL. Lists containing a negated range (an OR clause) keep the AND-join, since interval-union impossibility is not evaluated.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Part of a series fixing correctness bugs in the LookML/Looker import adapter (
sidemantic/adapters/lookml.py), found by a deep audit._convert_lookml_filter_to_sqltreated Looker filter values as opaque strings, so anything past simple comparisons produced wrong or invalid SQL:last 7 daysf = 'last 7 days'(0 rows)5 to 10f = '5 to 10'f >= 5 AND f <= 10[1,10]f IN ('[1','10]')f >= 1 AND f <= 10NOT 5f = 'NOT 5'f != 5EMPTYf = ''(f IS NULL OR f = '')-%foo%f != '%foo%'f NOT LIKE '%foo%'O'Brienf = 'O'Brien'(broken / injection)f = 'O''Brien'Changes
a to b, open-ended), interval brackets[](),NOT/-negation,EMPTY = NULL OR '', wildcardNOT LIKE, and mixed comma lists (includes OR'd, excludes AND'd).Notes
this month, etc.) is deferred — it needs field-type + dialect handling.