fix(hogql): correctly parse breakdown values with quotes for funnels #21101
Conversation
There was a problem hiding this comment.
Looks good! I'd actually suggest taking it a step further some other time before release, and moving as much python string interpolation in parse_expr into placeholders as possible. This could avoid a bunch of random errors down the line.
Also some mypy troubles.
| return [parse_expr(f"if(has({breakdown_values}, prop), prop, {other_aggregation}) as prop")] | ||
| return [ | ||
| parse_expr( | ||
| f"if(has({{breakdown_values}}, prop), prop, {other_aggregation}) as prop", |
There was a problem hiding this comment.
The more things we can move to placeholders overall, the better. Could other_aggregation also go? It's equally a constant. Same with possibly self._get_breakdown_prop() above?
There was a problem hiding this comment.
Jup, I agree. Would be great to even have a lint rule warning about f-strings with parse_expr.
For now I went over all usages of parse_expr in funnels and checked that the interpolated strings work as expected e.g. there are a lot of places where some number is interpolated latest_{i} and i is validated via pydantic.
| if normalize_url: | ||
| regex = "[\\\\/?#]*$" | ||
| expr = parse_expr( | ||
| f"if( empty( replaceRegexpOne({{breakdown_value}}, '{regex}', '') ), '/', replaceRegexpOne({{breakdown_value}}, '{regex}', ''))", |
There was a problem hiding this comment.
regex could also be passed in through a placeholder
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Problem
Some HogQL funnel insights have been failing, as a breakdown value containing a single quote was parsed erroneously as
ast.Fieldinstead of the correctast.Constant.Edit: checked all occurances of
parse_exprin funnels and added similar fixes.Changes
Fixes it.
How did you test this code?
Added a test and verified with a local insight