From 8148b29e64fa330142fd8334923fc2f1153acde7 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Wed, 18 Jan 2023 22:52:06 +0100 Subject: [PATCH 01/19] rename to "hogql" and "HogQLContext" --- posthog/hogql/{expr_parser.py => hogql.py} | 8 +- .../{test_expr_parser.py => test_hogql.py} | 104 +++++++++--------- posthog/models/event/query_event_list.py | 14 +-- 3 files changed, 63 insertions(+), 63 deletions(-) rename posthog/hogql/{expr_parser.py => hogql.py} (98%) rename posthog/hogql/test/{test_expr_parser.py => test_hogql.py} (61%) diff --git a/posthog/hogql/expr_parser.py b/posthog/hogql/hogql.py similarity index 98% rename from posthog/hogql/expr_parser.py rename to posthog/hogql/hogql.py index a9342a33f846a..f7dc1e9d1ccc0 100644 --- a/posthog/hogql/expr_parser.py +++ b/posthog/hogql/hogql.py @@ -118,7 +118,7 @@ @dataclass -class ExprParserContext: +class HogQLContext: """Context given to a HogQL expression parser""" # If set, will save string constants to this list instead of inlining them @@ -129,7 +129,7 @@ class ExprParserContext: is_aggregation: bool = False -def translate_hql(hql: str, context: Optional[ExprParserContext] = None) -> str: +def translate_hogql(hql: str, context: Optional[HogQLContext] = None) -> str: """Translate a HogQL expression into a Clickhouse expression.""" if hql == "*": return f"tuple({','.join(SELECT_STAR_FROM_EVENTS_FIELDS)})" @@ -146,11 +146,11 @@ def translate_hql(hql: str, context: Optional[ExprParserContext] = None) -> str: except SyntaxError as err: raise ValueError(f"SyntaxError: {err.msg}") if not context: - context = ExprParserContext() + context = HogQLContext() return translate_ast(node, [], context) -def translate_ast(node: ast.AST, stack: List[ast.AST], context: ExprParserContext) -> str: +def translate_ast(node: ast.AST, stack: List[ast.AST], context: HogQLContext) -> str: """Translate a parsed HogQL expression in the shape of a Python AST into a Clickhouse expression.""" stack.append(node) if isinstance(node, ast.Module): diff --git a/posthog/hogql/test/test_expr_parser.py b/posthog/hogql/test/test_hogql.py similarity index 61% rename from posthog/hogql/test/test_expr_parser.py rename to posthog/hogql/test/test_hogql.py index 13f11bce17f78..d2a8997ff1840 100644 --- a/posthog/hogql/test/test_expr_parser.py +++ b/posthog/hogql/test/test_hogql.py @@ -1,46 +1,46 @@ from typing import Any, Dict -from posthog.hogql.expr_parser import ExprParserContext, translate_hql +from posthog.hogql.hogql import HogQLContext, translate_hogql from posthog.test.base import APIBaseTest, ClickhouseTestMixin -class TestExprParser(APIBaseTest, ClickhouseTestMixin): +class TestHogQLContext(APIBaseTest, ClickhouseTestMixin): def test_hogql_literals(self): - self.assertEqual(translate_hql("1 + 2"), "plus(1, 2)") - self.assertEqual(translate_hql("-1 + 2"), "plus(-1, 2)") - self.assertEqual(translate_hql("-1 - 2 / (3 + 4)"), "minus(-1, divide(2, plus(3, 4)))") - self.assertEqual(translate_hql("1.0 * 2.66"), "multiply(1.0, 2.66)") - self.assertEqual(translate_hql("1.0 % 2.66"), "modulo(1.0, 2.66)") - self.assertEqual(translate_hql("'string'"), "'string'") - self.assertEqual(translate_hql('"string"'), "'string'") + self.assertEqual(translate_hogql("1 + 2"), "plus(1, 2)") + self.assertEqual(translate_hogql("-1 + 2"), "plus(-1, 2)") + self.assertEqual(translate_hogql("-1 - 2 / (3 + 4)"), "minus(-1, divide(2, plus(3, 4)))") + self.assertEqual(translate_hogql("1.0 * 2.66"), "multiply(1.0, 2.66)") + self.assertEqual(translate_hogql("1.0 % 2.66"), "modulo(1.0, 2.66)") + self.assertEqual(translate_hogql("'string'"), "'string'") + self.assertEqual(translate_hogql('"string"'), "'string'") def test_hogql_fields_and_properties(self): self.assertEqual( - translate_hql("properties.bla"), + translate_hogql("properties.bla"), "replaceRegexpAll(JSONExtractRaw(properties, 'bla'), '^\"|\"$', '')", ) self.assertEqual( - translate_hql("properties['bla']"), + translate_hogql("properties['bla']"), "replaceRegexpAll(JSONExtractRaw(properties, 'bla'), '^\"|\"$', '')", ) self.assertEqual( - translate_hql('properties["bla"]'), + translate_hogql('properties["bla"]'), "replaceRegexpAll(JSONExtractRaw(properties, 'bla'), '^\"|\"$', '')", ) self.assertEqual( - translate_hql("properties.$bla"), + translate_hogql("properties.$bla"), "replaceRegexpAll(JSONExtractRaw(properties, '$bla'), '^\"|\"$', '')", ) self.assertEqual( - translate_hql("person.properties.bla"), + translate_hogql("person.properties.bla"), "replaceRegexpAll(JSONExtractRaw(person_properties, 'bla'), '^\"|\"$', '')", ) - self.assertEqual(translate_hql("uuid"), "uuid") - self.assertEqual(translate_hql("event"), "event") - self.assertEqual(translate_hql("timestamp"), "timestamp") - self.assertEqual(translate_hql("distinct_id"), "distinct_id") - self.assertEqual(translate_hql("person_id"), "person_id") - self.assertEqual(translate_hql("person.created_at"), "person_created_at") + self.assertEqual(translate_hogql("uuid"), "uuid") + self.assertEqual(translate_hogql("event"), "event") + self.assertEqual(translate_hogql("timestamp"), "timestamp") + self.assertEqual(translate_hogql("distinct_id"), "distinct_id") + self.assertEqual(translate_hogql("person_id"), "person_id") + self.assertEqual(translate_hogql("person.created_at"), "person_created_at") def test_hogql_materialized_fields_and_properties(self): try: @@ -50,20 +50,20 @@ def test_hogql_materialized_fields_and_properties(self): self.assertEqual(1 + 2, 3) return materialize("events", "$browser") - self.assertEqual(translate_hql("properties['$browser']"), '"mat_$browser"') + self.assertEqual(translate_hogql("properties['$browser']"), '"mat_$browser"') materialize("events", "$initial_waffle", table_column="person_properties") - self.assertEqual(translate_hql("person.properties['$initial_waffle']"), '"mat_pp_$initial_waffle"') + self.assertEqual(translate_hogql("person.properties['$initial_waffle']"), '"mat_pp_$initial_waffle"') def test_hogql_methods(self): - self.assertEqual(translate_hql("count()"), "count(*)") - self.assertEqual(translate_hql("count(event)"), "count(distinct event)") + self.assertEqual(translate_hogql("count()"), "count(*)") + self.assertEqual(translate_hogql("count(event)"), "count(distinct event)") def test_hogql_functions(self): - self.assertEqual(translate_hql("abs(1)"), "abs(1)") - self.assertEqual(translate_hql("max2(1,2)"), "max2(1, 2)") - self.assertEqual(translate_hql("toInt('1')"), "toInt64OrNull('1')") - self.assertEqual(translate_hql("toFloat('1.3')"), "toFloat64OrNull('1.3')") + self.assertEqual(translate_hogql("abs(1)"), "abs(1)") + self.assertEqual(translate_hogql("max2(1,2)"), "max2(1, 2)") + self.assertEqual(translate_hogql("toInt('1')"), "toInt64OrNull('1')") + self.assertEqual(translate_hogql("toFloat('1.3')"), "toFloat64OrNull('1.3')") def test_hogql_expr_parse_errors(self): self._assert_value_error("", "Module body must contain only one 'Expr'") @@ -91,23 +91,23 @@ def test_hogql_expr_parse_errors(self): self._assert_value_error("1;2", "Module body must contain only one 'Expr'") def test_hogql_returned_properties(self): - context = ExprParserContext() - translate_hql("avg(properties.prop) + avg(uuid) + event", context) + context = HogQLContext() + translate_hogql("avg(properties.prop) + avg(uuid) + event", context) self.assertEqual(context.attribute_list, [["properties", "prop"], ["uuid"], ["event"]]) self.assertEqual(context.is_aggregation, True) - context = ExprParserContext() - translate_hql("coalesce(event, properties.event)", context) + context = HogQLContext() + translate_hogql("coalesce(event, properties.event)", context) self.assertEqual(context.attribute_list, [["event"], ["properties", "event"]]) self.assertEqual(context.is_aggregation, False) - context = ExprParserContext() - translate_hql("count() + sum(timestamp)", context) + context = HogQLContext() + translate_hogql("count() + sum(timestamp)", context) self.assertEqual(context.attribute_list, [["timestamp"]]) self.assertEqual(context.is_aggregation, True) - context = ExprParserContext() - translate_hql("event + avg(event + properties.event) + avg(event + properties.event)", context) + context = HogQLContext() + translate_hogql("event + avg(event + properties.event) + avg(event + properties.event)", context) self.assertEqual( context.attribute_list, [["event"], ["event"], ["properties", "event"], ["event"], ["properties", "event"]] ) @@ -115,54 +115,54 @@ def test_hogql_returned_properties(self): def test_hogql_logic(self): self.assertEqual( - translate_hql("event or timestamp"), + translate_hogql("event or timestamp"), "or(event, timestamp)", ) self.assertEqual( - translate_hql("properties.bla and properties.bla2"), + translate_hogql("properties.bla and properties.bla2"), "and(replaceRegexpAll(JSONExtractRaw(properties, 'bla'), '^\"|\"$', ''), replaceRegexpAll(JSONExtractRaw(properties, 'bla2'), '^\"|\"$', ''))", ) self.assertEqual( - translate_hql("event or timestamp or true or count()"), + translate_hogql("event or timestamp or true or count()"), "or(event, timestamp, true, count(*))", ) self.assertEqual( - translate_hql("event or not timestamp"), + translate_hogql("event or not timestamp"), "or(event, not(timestamp))", ) def test_hogql_comparisons(self): - self.assertEqual(translate_hql("event == 'E'"), "equals(event, 'E')") - self.assertEqual(translate_hql("event != 'E'"), "notEquals(event, 'E')") - self.assertEqual(translate_hql("event > 'E'"), "greater(event, 'E')") - self.assertEqual(translate_hql("event >= 'E'"), "greaterOrEquals(event, 'E')") - self.assertEqual(translate_hql("event < 'E'"), "less(event, 'E')") - self.assertEqual(translate_hql("event <= 'E'"), "lessOrEquals(event, 'E')") + self.assertEqual(translate_hogql("event == 'E'"), "equals(event, 'E')") + self.assertEqual(translate_hogql("event != 'E'"), "notEquals(event, 'E')") + self.assertEqual(translate_hogql("event > 'E'"), "greater(event, 'E')") + self.assertEqual(translate_hogql("event >= 'E'"), "greaterOrEquals(event, 'E')") + self.assertEqual(translate_hogql("event < 'E'"), "less(event, 'E')") + self.assertEqual(translate_hogql("event <= 'E'"), "lessOrEquals(event, 'E')") def test_hogql_special_root_properties(self): self.assertEqual( - translate_hql("*"), + translate_hogql("*"), "tuple(uuid,event,properties,timestamp,team_id,distinct_id,elements_chain,created_at,person_id,person_created_at,person_properties)", ) self.assertEqual( - translate_hql("person"), + translate_hogql("person"), "tuple(distinct_id, person_id, person_created_at, replaceRegexpAll(JSONExtractRaw(person_properties, 'name'), '^\"|\"$', ''), replaceRegexpAll(JSONExtractRaw(person_properties, 'email'), '^\"|\"$', ''))", ) self._assert_value_error("person + 1", 'Can not use the field "person" in an expression') def test_collected_values(self): collected_values: Dict[str, Any] = {} - context = ExprParserContext(collect_values=collected_values) - self.assertEqual(translate_hql("event == 'E'", context), "equals(event, %(val_0)s)") + context = HogQLContext(collect_values=collected_values) + self.assertEqual(translate_hogql("event == 'E'", context), "equals(event, %(val_0)s)") self.assertEqual(collected_values, {"val_0": "E"}) self.assertEqual( - translate_hql("coalesce(4.2, 5, 'lol', 'hoo')", context), "coalesce(4.2, 5, %(val_1)s, %(val_2)s)" + translate_hogql("coalesce(4.2, 5, 'lol', 'hoo')", context), "coalesce(4.2, 5, %(val_1)s, %(val_2)s)" ) self.assertEqual(collected_values, {"val_0": "E", "val_1": "lol", "val_2": "hoo"}) def _assert_value_error(self, expr, expected_error): with self.assertRaises(ValueError) as context: - translate_hql(expr) + translate_hogql(expr) if expected_error not in str(context.exception): raise AssertionError(f"Expected '{expected_error}' in '{str(context.exception)}'") self.assertTrue(expected_error in str(context.exception)) diff --git a/posthog/models/event/query_event_list.py b/posthog/models/event/query_event_list.py index 3799629f07df1..3ccc95af47d8e 100644 --- a/posthog/models/event/query_event_list.py +++ b/posthog/models/event/query_event_list.py @@ -8,7 +8,7 @@ from posthog.api.utils import get_pk_or_uuid from posthog.clickhouse.client.connection import Workload -from posthog.hogql.expr_parser import SELECT_STAR_FROM_EVENTS_FIELDS, ExprParserContext, translate_hql +from posthog.hogql.hogql import SELECT_STAR_FROM_EVENTS_FIELDS, HogQLContext, translate_hogql from posthog.models import Action, Filter, Person, Team from posthog.models.action.util import format_action_filter from posthog.models.element import chain_to_elements @@ -146,17 +146,17 @@ def query_events_list( select = ["*"] for expr in select: - context = ExprParserContext() + context = HogQLContext() context.collect_values = collected_hogql_values - clickhouse_sql = translate_hql(expr, context) + clickhouse_sql = translate_hogql(expr, context) select_columns.append(clickhouse_sql) if not context.is_aggregation: group_by_columns.append(clickhouse_sql) for expr in where or []: - context = ExprParserContext() + context = HogQLContext() context.collect_values = collected_hogql_values - clickhouse_sql = translate_hql(expr, context) + clickhouse_sql = translate_hogql(expr, context) if context.is_aggregation: having_filters.append(clickhouse_sql) else: @@ -168,9 +168,9 @@ def query_events_list( if fragment.startswith("-"): order_direction = "DESC" fragment = fragment[1:] - context = ExprParserContext() + context = HogQLContext() context.collect_values = collected_hogql_values - order_by_list.append(translate_hql(fragment, context) + " " + order_direction) + order_by_list.append(translate_hogql(fragment, context) + " " + order_direction) else: order_by_list.append(select_columns[0] + " ASC") From 9986712fcf997a37ef826032c9652b49cc720486 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Wed, 18 Jan 2023 23:22:54 +0100 Subject: [PATCH 02/19] make context mandatory --- posthog/hogql/hogql.py | 22 +++-- posthog/hogql/test/test_hogql.py | 105 ++++++++++++----------- posthog/models/event/query_event_list.py | 22 ++--- 3 files changed, 74 insertions(+), 75 deletions(-) diff --git a/posthog/hogql/hogql.py b/posthog/hogql/hogql.py index f7dc1e9d1ccc0..31f5ed15bb1c7 100644 --- a/posthog/hogql/hogql.py +++ b/posthog/hogql/hogql.py @@ -121,15 +121,15 @@ class HogQLContext: """Context given to a HogQL expression parser""" - # If set, will save string constants to this list instead of inlining them - collect_values: Optional[Dict[str, Any]] = None + # If set, will save string constants to this dict. Inlines strings into the query if None. + values: Optional[Dict[str, Any]] = field(default_factory=lambda: {}) # List of field and property accesses found in the expression - attribute_list: List[List[str]] = field(default_factory=list) - # Did the expression contain a call from HOGQL_AGGREGATIONS - is_aggregation: bool = False + attribute_list: List[List[str]] = field(default_factory=lambda: []) + # Did the last calls to translate_hogql since setting this to False contain any HOGQL_AGGREGATIONS + found_aggregation: bool = False -def translate_hogql(hql: str, context: Optional[HogQLContext] = None) -> str: +def translate_hogql(hql: str, context: HogQLContext) -> str: """Translate a HogQL expression into a Clickhouse expression.""" if hql == "*": return f"tuple({','.join(SELECT_STAR_FROM_EVENTS_FIELDS)})" @@ -145,8 +145,6 @@ def translate_hogql(hql: str, context: Optional[HogQLContext] = None) -> str: node = ast.parse(hql) except SyntaxError as err: raise ValueError(f"SyntaxError: {err.msg}") - if not context: - context = HogQLContext() return translate_ast(node, [], context) @@ -210,12 +208,12 @@ def translate_ast(node: ast.AST, stack: List[ast.AST], context: HogQLContext) -> else: raise ValueError(f"Unknown Compare: {type(node.ops[0])}") elif isinstance(node, ast.Constant): - key = f"val_{len(context.collect_values or {})}" + key = f"val_{len(context.values or {})}" if isinstance(node.value, int) or isinstance(node.value, float): response = str(node.value) elif isinstance(node.value, str): - if isinstance(context.collect_values, dict): - context.collect_values[key] = node.value + if isinstance(context.values, dict): + context.values[key] = node.value response = f"%({key})s" else: response = escape_param(node.value) @@ -262,7 +260,7 @@ def translate_ast(node: ast.AST, stack: List[ast.AST], context: HogQLContext) -> raise ValueError(f"Can only call simple functions like 'avg(properties.bla)' or 'count()'") call_name = node.func.id if call_name in HOGQL_AGGREGATIONS: - context.is_aggregation = True + context.found_aggregation = True if call_name == "count" and len(node.args) == 0: response = "count(*)" diff --git a/posthog/hogql/test/test_hogql.py b/posthog/hogql/test/test_hogql.py index d2a8997ff1840..33a75296e6134 100644 --- a/posthog/hogql/test/test_hogql.py +++ b/posthog/hogql/test/test_hogql.py @@ -1,46 +1,50 @@ -from typing import Any, Dict - from posthog.hogql.hogql import HogQLContext, translate_hogql from posthog.test.base import APIBaseTest, ClickhouseTestMixin class TestHogQLContext(APIBaseTest, ClickhouseTestMixin): + # Helper to always translate HogQL with a blank context + def _translate(self, query: str): + return translate_hogql(query, HogQLContext()) + def test_hogql_literals(self): - self.assertEqual(translate_hogql("1 + 2"), "plus(1, 2)") - self.assertEqual(translate_hogql("-1 + 2"), "plus(-1, 2)") - self.assertEqual(translate_hogql("-1 - 2 / (3 + 4)"), "minus(-1, divide(2, plus(3, 4)))") - self.assertEqual(translate_hogql("1.0 * 2.66"), "multiply(1.0, 2.66)") - self.assertEqual(translate_hogql("1.0 % 2.66"), "modulo(1.0, 2.66)") - self.assertEqual(translate_hogql("'string'"), "'string'") - self.assertEqual(translate_hogql('"string"'), "'string'") + self.assertEqual(self._translate("1 + 2"), "plus(1, 2)") + self.assertEqual(self._translate("-1 + 2"), "plus(-1, 2)") + self.assertEqual(self._translate("-1 - 2 / (3 + 4)"), "minus(-1, divide(2, plus(3, 4)))") + self.assertEqual(self._translate("1.0 * 2.66"), "multiply(1.0, 2.66)") + self.assertEqual(self._translate("1.0 % 2.66"), "modulo(1.0, 2.66)") + self.assertEqual(translate_hogql("'string'", HogQLContext(values=None)), "'string'") + self.assertEqual(translate_hogql('"string"', HogQLContext(values=None)), "'string'") + self.assertEqual(self._translate("'string'"), "%(val_0)s") + self.assertEqual(self._translate('"string"'), "%(val_0)s") def test_hogql_fields_and_properties(self): self.assertEqual( - translate_hogql("properties.bla"), + self._translate("properties.bla"), "replaceRegexpAll(JSONExtractRaw(properties, 'bla'), '^\"|\"$', '')", ) self.assertEqual( - translate_hogql("properties['bla']"), + self._translate("properties['bla']"), "replaceRegexpAll(JSONExtractRaw(properties, 'bla'), '^\"|\"$', '')", ) self.assertEqual( - translate_hogql('properties["bla"]'), + self._translate('properties["bla"]'), "replaceRegexpAll(JSONExtractRaw(properties, 'bla'), '^\"|\"$', '')", ) self.assertEqual( - translate_hogql("properties.$bla"), + self._translate("properties.$bla"), "replaceRegexpAll(JSONExtractRaw(properties, '$bla'), '^\"|\"$', '')", ) self.assertEqual( - translate_hogql("person.properties.bla"), + self._translate("person.properties.bla"), "replaceRegexpAll(JSONExtractRaw(person_properties, 'bla'), '^\"|\"$', '')", ) - self.assertEqual(translate_hogql("uuid"), "uuid") - self.assertEqual(translate_hogql("event"), "event") - self.assertEqual(translate_hogql("timestamp"), "timestamp") - self.assertEqual(translate_hogql("distinct_id"), "distinct_id") - self.assertEqual(translate_hogql("person_id"), "person_id") - self.assertEqual(translate_hogql("person.created_at"), "person_created_at") + self.assertEqual(self._translate("uuid"), "uuid") + self.assertEqual(self._translate("event"), "event") + self.assertEqual(self._translate("timestamp"), "timestamp") + self.assertEqual(self._translate("distinct_id"), "distinct_id") + self.assertEqual(self._translate("person_id"), "person_id") + self.assertEqual(self._translate("person.created_at"), "person_created_at") def test_hogql_materialized_fields_and_properties(self): try: @@ -50,20 +54,21 @@ def test_hogql_materialized_fields_and_properties(self): self.assertEqual(1 + 2, 3) return materialize("events", "$browser") - self.assertEqual(translate_hogql("properties['$browser']"), '"mat_$browser"') + self.assertEqual(self._translate("properties['$browser']"), '"mat_$browser"') materialize("events", "$initial_waffle", table_column="person_properties") - self.assertEqual(translate_hogql("person.properties['$initial_waffle']"), '"mat_pp_$initial_waffle"') + self.assertEqual(self._translate("person.properties['$initial_waffle']"), '"mat_pp_$initial_waffle"') def test_hogql_methods(self): - self.assertEqual(translate_hogql("count()"), "count(*)") - self.assertEqual(translate_hogql("count(event)"), "count(distinct event)") + self.assertEqual(self._translate("count()"), "count(*)") + self.assertEqual(self._translate("count(event)"), "count(distinct event)") def test_hogql_functions(self): - self.assertEqual(translate_hogql("abs(1)"), "abs(1)") - self.assertEqual(translate_hogql("max2(1,2)"), "max2(1, 2)") - self.assertEqual(translate_hogql("toInt('1')"), "toInt64OrNull('1')") - self.assertEqual(translate_hogql("toFloat('1.3')"), "toFloat64OrNull('1.3')") + context = HogQLContext(values=None) # inline values + self.assertEqual(self._translate("abs(1)"), "abs(1)") + self.assertEqual(self._translate("max2(1,2)"), "max2(1, 2)") + self.assertEqual(translate_hogql("toInt('1')", context), "toInt64OrNull('1')") + self.assertEqual(translate_hogql("toFloat('1.3')", context), "toFloat64OrNull('1.3')") def test_hogql_expr_parse_errors(self): self._assert_value_error("", "Module body must contain only one 'Expr'") @@ -94,75 +99,75 @@ def test_hogql_returned_properties(self): context = HogQLContext() translate_hogql("avg(properties.prop) + avg(uuid) + event", context) self.assertEqual(context.attribute_list, [["properties", "prop"], ["uuid"], ["event"]]) - self.assertEqual(context.is_aggregation, True) + self.assertEqual(context.found_aggregation, True) context = HogQLContext() translate_hogql("coalesce(event, properties.event)", context) self.assertEqual(context.attribute_list, [["event"], ["properties", "event"]]) - self.assertEqual(context.is_aggregation, False) + self.assertEqual(context.found_aggregation, False) context = HogQLContext() translate_hogql("count() + sum(timestamp)", context) self.assertEqual(context.attribute_list, [["timestamp"]]) - self.assertEqual(context.is_aggregation, True) + self.assertEqual(context.found_aggregation, True) context = HogQLContext() translate_hogql("event + avg(event + properties.event) + avg(event + properties.event)", context) self.assertEqual( context.attribute_list, [["event"], ["event"], ["properties", "event"], ["event"], ["properties", "event"]] ) - self.assertEqual(context.is_aggregation, True) + self.assertEqual(context.found_aggregation, True) def test_hogql_logic(self): self.assertEqual( - translate_hogql("event or timestamp"), + self._translate("event or timestamp"), "or(event, timestamp)", ) self.assertEqual( - translate_hogql("properties.bla and properties.bla2"), + self._translate("properties.bla and properties.bla2"), "and(replaceRegexpAll(JSONExtractRaw(properties, 'bla'), '^\"|\"$', ''), replaceRegexpAll(JSONExtractRaw(properties, 'bla2'), '^\"|\"$', ''))", ) self.assertEqual( - translate_hogql("event or timestamp or true or count()"), + self._translate("event or timestamp or true or count()"), "or(event, timestamp, true, count(*))", ) self.assertEqual( - translate_hogql("event or not timestamp"), + self._translate("event or not timestamp"), "or(event, not(timestamp))", ) def test_hogql_comparisons(self): - self.assertEqual(translate_hogql("event == 'E'"), "equals(event, 'E')") - self.assertEqual(translate_hogql("event != 'E'"), "notEquals(event, 'E')") - self.assertEqual(translate_hogql("event > 'E'"), "greater(event, 'E')") - self.assertEqual(translate_hogql("event >= 'E'"), "greaterOrEquals(event, 'E')") - self.assertEqual(translate_hogql("event < 'E'"), "less(event, 'E')") - self.assertEqual(translate_hogql("event <= 'E'"), "lessOrEquals(event, 'E')") + context = HogQLContext(values=None) # inline values + self.assertEqual(translate_hogql("event == 'E'", context), "equals(event, 'E')") + self.assertEqual(translate_hogql("event != 'E'", context), "notEquals(event, 'E')") + self.assertEqual(translate_hogql("event > 'E'", context), "greater(event, 'E')") + self.assertEqual(translate_hogql("event >= 'E'", context), "greaterOrEquals(event, 'E')") + self.assertEqual(translate_hogql("event < 'E'", context), "less(event, 'E')") + self.assertEqual(translate_hogql("event <= 'E'", context), "lessOrEquals(event, 'E')") def test_hogql_special_root_properties(self): self.assertEqual( - translate_hogql("*"), + self._translate("*"), "tuple(uuid,event,properties,timestamp,team_id,distinct_id,elements_chain,created_at,person_id,person_created_at,person_properties)", ) self.assertEqual( - translate_hogql("person"), + self._translate("person"), "tuple(distinct_id, person_id, person_created_at, replaceRegexpAll(JSONExtractRaw(person_properties, 'name'), '^\"|\"$', ''), replaceRegexpAll(JSONExtractRaw(person_properties, 'email'), '^\"|\"$', ''))", ) self._assert_value_error("person + 1", 'Can not use the field "person" in an expression') - def test_collected_values(self): - collected_values: Dict[str, Any] = {} - context = HogQLContext(collect_values=collected_values) + def test_hogql_values(self): + context = HogQLContext() self.assertEqual(translate_hogql("event == 'E'", context), "equals(event, %(val_0)s)") - self.assertEqual(collected_values, {"val_0": "E"}) + self.assertEqual(context.values, {"val_0": "E"}) self.assertEqual( translate_hogql("coalesce(4.2, 5, 'lol', 'hoo')", context), "coalesce(4.2, 5, %(val_1)s, %(val_2)s)" ) - self.assertEqual(collected_values, {"val_0": "E", "val_1": "lol", "val_2": "hoo"}) + self.assertEqual(context.values, {"val_0": "E", "val_1": "lol", "val_2": "hoo"}) def _assert_value_error(self, expr, expected_error): with self.assertRaises(ValueError) as context: - translate_hogql(expr) + self._translate(expr) if expected_error not in str(context.exception): raise AssertionError(f"Expected '{expected_error}' in '{str(context.exception)}'") self.assertTrue(expected_error in str(context.exception)) diff --git a/posthog/models/event/query_event_list.py b/posthog/models/event/query_event_list.py index 3ccc95af47d8e..25cbf358e3008 100644 --- a/posthog/models/event/query_event_list.py +++ b/posthog/models/event/query_event_list.py @@ -135,7 +135,7 @@ def query_events_list( # events list v2 - hogql - collected_hogql_values: Dict[str, Any] = {} + hogql_context = HogQLContext() select_columns: List[str] = [] group_by_columns: List[str] = [] where_filters: List[str] = [] @@ -146,18 +146,16 @@ def query_events_list( select = ["*"] for expr in select: - context = HogQLContext() - context.collect_values = collected_hogql_values - clickhouse_sql = translate_hogql(expr, context) + hogql_context.found_aggregation = False + clickhouse_sql = translate_hogql(expr, hogql_context) select_columns.append(clickhouse_sql) - if not context.is_aggregation: + if not hogql_context.found_aggregation: group_by_columns.append(clickhouse_sql) for expr in where or []: - context = HogQLContext() - context.collect_values = collected_hogql_values - clickhouse_sql = translate_hogql(expr, context) - if context.is_aggregation: + hogql_context.found_aggregation = False + clickhouse_sql = translate_hogql(expr, hogql_context) + if hogql_context.found_aggregation: having_filters.append(clickhouse_sql) else: where_filters.append(clickhouse_sql) @@ -168,9 +166,7 @@ def query_events_list( if fragment.startswith("-"): order_direction = "DESC" fragment = fragment[1:] - context = HogQLContext() - context.collect_values = collected_hogql_values - order_by_list.append(translate_hogql(fragment, context) + " " + order_direction) + order_by_list.append(translate_hogql(fragment, hogql_context) + " " + order_direction) else: order_by_list.append(select_columns[0] + " ASC") @@ -194,7 +190,7 @@ def query_events_list( "offset": offset, **condition_params, **prop_filter_params, - **collected_hogql_values, + **hogql_context.values, }, with_column_types=True, query_type="events_list", From bd69a8633e5d73080494057392e260beebee0106 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Wed, 18 Jan 2023 23:36:21 +0100 Subject: [PATCH 03/19] rename TaxonomicBreakdownFilter --- .../components/Cards/InsightCard/InsightDetails.tsx | 4 ++-- .../src/scenes/insights/EditorFilters/Breakdown.tsx | 10 ++++++++-- .../BreakdownFilter/TaxonomicBreakdownFilter.tsx | 5 ++--- .../scenes/insights/filters/BreakdownFilter/index.ts | 5 ----- .../BreakdownFilter/taxonomicBreakdownFilterUtils.ts | 4 +++- frontend/src/scenes/insights/utils/cleanFilters.ts | 2 +- frontend/src/types.ts | 8 ++++++++ 7 files changed, 24 insertions(+), 14 deletions(-) delete mode 100644 frontend/src/scenes/insights/filters/BreakdownFilter/index.ts diff --git a/frontend/src/lib/components/Cards/InsightCard/InsightDetails.tsx b/frontend/src/lib/components/Cards/InsightCard/InsightDetails.tsx index bd18828edf6df..6a6be3d90dca4 100644 --- a/frontend/src/lib/components/Cards/InsightCard/InsightDetails.tsx +++ b/frontend/src/lib/components/Cards/InsightCard/InsightDetails.tsx @@ -3,7 +3,7 @@ import { FEATURE_FLAGS } from 'lib/constants' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { allOperatorsMapping, alphabet, capitalizeFirstLetter, formatPropertyLabel } from 'lib/utils' import { LocalFilter, toLocalFilters } from 'scenes/insights/filters/ActionFilter/entityFilterLogic' -import { BreakdownFilter } from 'scenes/insights/filters/BreakdownFilter' +import { TaxonomicBreakdownFilter } from 'scenes/insights/filters/BreakdownFilter/TaxonomicBreakdownFilter' import { humanizePathsEventTypes } from 'scenes/insights/utils' import { apiValueToMathType, MathCategory, MathDefinition, mathsLogic } from 'scenes/trends/mathsLogic' import { urls } from 'scenes/urls' @@ -288,7 +288,7 @@ export function BreakdownSummary({ filters }: { filters: Partial }): return (
Breakdown by
- + return ( + + ) } diff --git a/frontend/src/scenes/insights/filters/BreakdownFilter/TaxonomicBreakdownFilter.tsx b/frontend/src/scenes/insights/filters/BreakdownFilter/TaxonomicBreakdownFilter.tsx index d2cfada420bda..b1a8581bba264 100644 --- a/frontend/src/scenes/insights/filters/BreakdownFilter/TaxonomicBreakdownFilter.tsx +++ b/frontend/src/scenes/insights/filters/BreakdownFilter/TaxonomicBreakdownFilter.tsx @@ -6,9 +6,8 @@ import { propertyDefinitionsModel } from '~/models/propertyDefinitionsModel' import { Breakdown, ChartDisplayType, FilterType, InsightType, TrendsFilterType } from '~/types' import { BreakdownTag } from './BreakdownTag' import './TaxonomicBreakdownFilter.scss' -import { onFilterChange } from './taxonomicBreakdownFilterUtils' +import { onFilterChange, isURLNormalizeable } from './taxonomicBreakdownFilterUtils' import { isTrendsFilter } from 'scenes/insights/sharedUtils' -import { isURLNormalizeable } from 'scenes/insights/filters/BreakdownFilter/index' export interface TaxonomicBreakdownFilterProps { filters: Partial @@ -24,7 +23,7 @@ export const isCohortBreakdown = (t: number | string): t is number | string => i export const isPersonEventOrGroup = (t: number | string): t is string => typeof t === 'string' && t !== 'all' -export function BreakdownFilter({ +export function TaxonomicBreakdownFilter({ filters, setFilters, useMultiBreakdown = false, diff --git a/frontend/src/scenes/insights/filters/BreakdownFilter/index.ts b/frontend/src/scenes/insights/filters/BreakdownFilter/index.ts deleted file mode 100644 index 3f872a0e0cbce..0000000000000 --- a/frontend/src/scenes/insights/filters/BreakdownFilter/index.ts +++ /dev/null @@ -1,5 +0,0 @@ -export { BreakdownFilter } from './TaxonomicBreakdownFilter' - -export const isURLNormalizeable = (propertyName: string): boolean => { - return ['$current_url', '$pathname'].includes(propertyName) -} diff --git a/frontend/src/scenes/insights/filters/BreakdownFilter/taxonomicBreakdownFilterUtils.ts b/frontend/src/scenes/insights/filters/BreakdownFilter/taxonomicBreakdownFilterUtils.ts index bd1b87858ff24..0119f3e029ead 100644 --- a/frontend/src/scenes/insights/filters/BreakdownFilter/taxonomicBreakdownFilterUtils.ts +++ b/frontend/src/scenes/insights/filters/BreakdownFilter/taxonomicBreakdownFilterUtils.ts @@ -5,8 +5,10 @@ import { TaxonomicFilterValue, } from 'lib/components/TaxonomicFilter/types' import { taxonomicFilterTypeToPropertyFilterType } from 'lib/components/PropertyFilters/utils' -import { isURLNormalizeable } from 'scenes/insights/filters/BreakdownFilter/index' +export const isURLNormalizeable = (propertyName: string): boolean => { + return ['$current_url', '$pathname'].includes(propertyName) +} interface FilterChange { useMultiBreakdown: string | boolean | undefined breakdownParts: (string | number)[] diff --git a/frontend/src/scenes/insights/utils/cleanFilters.ts b/frontend/src/scenes/insights/utils/cleanFilters.ts index a3a9892467b1b..01e6cb4c873ba 100644 --- a/frontend/src/scenes/insights/utils/cleanFilters.ts +++ b/frontend/src/scenes/insights/utils/cleanFilters.ts @@ -29,7 +29,7 @@ import { isStickinessFilter, isTrendsFilter, } from 'scenes/insights/sharedUtils' -import { isURLNormalizeable } from 'scenes/insights/filters/BreakdownFilter' +import { isURLNormalizeable } from 'scenes/insights/filters/BreakdownFilter/taxonomicBreakdownFilterUtils' export function getDefaultEvent(): Entity { const event = getDefaultEventName() diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 2dfbfd00d6557..c3a672ea88926 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -429,6 +429,7 @@ export enum PropertyFilterType { Cohort = 'cohort', Recording = 'recording', Group = 'group', + HogQL = 'hogql', } /** Sync with plugin-server/src/types.ts */ @@ -482,6 +483,11 @@ export interface FeaturePropertyFilter extends BasePropertyFilter { operator: PropertyOperator } +export interface HogQLPropertyFilter extends BasePropertyFilter { + type: PropertyFilterType.HogQL + key: string +} + export type PropertyFilter = | EventPropertyFilter | PersonPropertyFilter @@ -491,6 +497,7 @@ export type PropertyFilter = | RecordingDurationFilter | GroupPropertyFilter | FeaturePropertyFilter + | HogQLPropertyFilter export type AnyPropertyFilter = | Partial @@ -501,6 +508,7 @@ export type AnyPropertyFilter = | Partial | Partial | Partial + | Partial export type AnyFilterLike = AnyPropertyFilter | PropertyGroupFilter | PropertyGroupFilterValue From c83b9fd8cf2f7c6014a5d0da5471383e7aac6afa Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Wed, 18 Jan 2023 23:37:54 +0100 Subject: [PATCH 04/19] hogql property types --- .../components/TaxonomicPropertyFilter.tsx | 19 +++++++++++++++---- .../taxonomicPropertyFilterLogic.ts | 18 ++++++++++++++++-- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/frontend/src/lib/components/PropertyFilters/components/TaxonomicPropertyFilter.tsx b/frontend/src/lib/components/PropertyFilters/components/TaxonomicPropertyFilter.tsx index e4b0cdd1a4a2a..e4d34768d017a 100644 --- a/frontend/src/lib/components/PropertyFilters/components/TaxonomicPropertyFilter.tsx +++ b/frontend/src/lib/components/PropertyFilters/components/TaxonomicPropertyFilter.tsx @@ -20,7 +20,7 @@ import { import { PropertyFilterInternalProps } from 'lib/components/PropertyFilters/types' import clsx from 'clsx' import { propertyDefinitionsModel } from '~/models/propertyDefinitionsModel' -import { AnyPropertyFilter, FilterLogicalOperator } from '~/types' +import { AnyPropertyFilter, FilterLogicalOperator, PropertyFilterType } from '~/types' import { useResizeBreakpoints } from 'lib/hooks/useResizeObserver' import { LemonButtonWithPopup } from '@posthog/lemon-ui' @@ -51,7 +51,10 @@ export function TaxonomicPropertyFilter({ value ) => { selectItem(taxonomicGroup, value) - if (taxonomicGroup.type === TaxonomicFilterGroupType.Cohorts) { + if ( + taxonomicGroup.type === TaxonomicFilterGroupType.Cohorts || + taxonomicGroup.type === TaxonomicFilterGroupType.HogQLExpression + ) { onComplete?.() } } @@ -68,8 +71,16 @@ export function TaxonomicPropertyFilter({ }) const { filter, dropdownOpen, selectedCohortName, activeTaxonomicGroup } = useValues(logic) const { openDropdown, closeDropdown, selectItem } = useActions(logic) - const showInitialSearchInline = !disablePopover && ((!filter?.type && !filter?.key) || filter?.type === 'cohort') - const showOperatorValueSelect = filter?.type && filter?.key && filter?.type !== 'cohort' + const showInitialSearchInline = + !disablePopover && + ((!filter?.type && !filter?.key) || + filter?.type === PropertyFilterType.Cohort || + filter?.type === PropertyFilterType.HogQL) + const showOperatorValueSelect = + filter?.type && + filter?.key && + filter?.type !== PropertyFilterType.Cohort && + filter?.type !== PropertyFilterType.HogQL const { propertyDefinitions } = useValues(propertyDefinitionsModel) diff --git a/frontend/src/lib/components/PropertyFilters/components/taxonomicPropertyFilterLogic.ts b/frontend/src/lib/components/PropertyFilters/components/taxonomicPropertyFilterLogic.ts index 347ce578e1286..256c7227518b1 100644 --- a/frontend/src/lib/components/PropertyFilters/components/taxonomicPropertyFilterLogic.ts +++ b/frontend/src/lib/components/PropertyFilters/components/taxonomicPropertyFilterLogic.ts @@ -1,6 +1,13 @@ import { kea } from 'kea' import { TaxonomicPropertyFilterLogicProps } from 'lib/components/PropertyFilters/types' -import { AnyPropertyFilter, CohortPropertyFilter, PropertyOperator, PropertyType } from '~/types' +import { + AnyPropertyFilter, + CohortPropertyFilter, + HogQLPropertyFilter, + PropertyFilterType, + PropertyOperator, + PropertyType, +} from '~/types' import type { taxonomicPropertyFilterLogicType } from './taxonomicPropertyFilterLogicType' import { cohortsModel } from '~/models/cohortsModel' import { TaxonomicFilterGroup, TaxonomicFilterValue } from 'lib/components/TaxonomicFilter/types' @@ -82,13 +89,20 @@ export const taxonomicPropertyFilterLogic = kea { const propertyType = taxonomicFilterTypeToPropertyFilterType(taxonomicGroup.type) if (propertyKey && propertyType) { - if (propertyType === 'cohort') { + if (propertyType === PropertyFilterType.Cohort) { const cohortProperty: CohortPropertyFilter = { key: 'id', value: parseInt(String(propertyKey)), type: propertyType, } props.propertyFilterLogic.actions.setFilter(props.filterIndex, cohortProperty) + } else if (propertyType === PropertyFilterType.HogQL) { + const hogQLProperty: HogQLPropertyFilter = { + type: propertyType, + key: String(propertyKey), + value: null, // must specify something to be compatible with existing types + } + props.propertyFilterLogic.actions.setFilter(props.filterIndex, hogQLProperty) } else { const propertyValueType = values.describeProperty(propertyKey) const property_name_to_default_operator_override = { From a2dacaf0283942c3508875953f3edfd402e7c73d Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Wed, 18 Jan 2023 23:44:45 +0100 Subject: [PATCH 05/19] support hogql property filters in the frontend --- .../components/PropertyFilters/propertyFilterLogic.ts | 4 +--- frontend/src/lib/components/PropertyFilters/utils.ts | 7 ++++++- frontend/src/lib/utils.tsx | 10 +++++++++- frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx | 6 ++++-- .../queries/nodes/EventsNode/EventPropertyFilters.tsx | 9 +++++++++ 5 files changed, 29 insertions(+), 7 deletions(-) diff --git a/frontend/src/lib/components/PropertyFilters/propertyFilterLogic.ts b/frontend/src/lib/components/PropertyFilters/propertyFilterLogic.ts index ceb4ff5bf7ae2..30a66db073f8f 100644 --- a/frontend/src/lib/components/PropertyFilters/propertyFilterLogic.ts +++ b/frontend/src/lib/components/PropertyFilters/propertyFilterLogic.ts @@ -44,10 +44,8 @@ export const propertyFilterLogic = kea([ listeners(({ actions, props, values }) => ({ // Only send update if value is set to something setFilter: ({ property }) => { - if (props.sendAllKeyUpdates) { + if (props.sendAllKeyUpdates || property?.value || (property?.key && property.type === 'hogql')) { actions.update() - } else { - property?.value && actions.update() } }, remove: () => actions.update(), diff --git a/frontend/src/lib/components/PropertyFilters/utils.ts b/frontend/src/lib/components/PropertyFilters/utils.ts index bc5681df4841e..338cb6fec510d 100644 --- a/frontend/src/lib/components/PropertyFilters/utils.ts +++ b/frontend/src/lib/components/PropertyFilters/utils.ts @@ -8,6 +8,7 @@ import { FeaturePropertyFilter, FilterLogicalOperator, GroupPropertyFilter, + HogQLPropertyFilter, PersonPropertyFilter, PropertyFilter, PropertyFilterType, @@ -57,7 +58,7 @@ export function isValidPropertyFilter(filter: AnyPropertyFilter): filter is Prop return ( !!filter && // is not falsy 'key' in filter && // has a "key" property - Object.values(filter).some((v) => !!v) // contains some properties with values + ((filter.type === 'hogql' && !!filter.key) || Object.values(filter).some((v) => !!v)) // contains some properties with values ) } @@ -90,6 +91,9 @@ export function isGroupPropertyFilter(filter?: AnyFilterLike | null): filter is export function isFeaturePropertyFilter(filter?: AnyFilterLike | null): filter is FeaturePropertyFilter { return filter?.type === PropertyFilterType.Feature } +export function isHogQLPropertyFilter(filter?: AnyFilterLike | null): filter is HogQLPropertyFilter { + return filter?.type === PropertyFilterType.HogQL +} export function isAnyPropertyfilter(filter?: AnyFilterLike | null): filter is AnyPropertyFilter { return ( @@ -148,6 +152,7 @@ const propertyFilterMapping: Partial = { // `@yiminghe/dom-align` objects @@ -358,6 +363,9 @@ export function formatPropertyLabel( keyMapping: KeyMappingInterface, valueFormatter: (value: PropertyFilterValue | undefined) => string | string[] | null = (s) => [String(s)] ): string { + if (isHogQLPropertyFilter(item)) { + return extractExpressionComment(item.key) + } const { value, key, operator, type } = item return type === 'cohort' ? cohortsById[value]?.name || `ID ${value}` diff --git a/frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx b/frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx index b14c4386440d8..d4c6287666f4c 100644 --- a/frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx +++ b/frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx @@ -21,7 +21,9 @@ export function InlineHogQLEditor({ value, onChange }: InlineHogQLEditorProps): className="font-mono" minRows={6} maxRows={6} - placeholder={'Enter HogQL Expression...'} + placeholder={ + 'Enter HogQL Expression, such as:\n- properties.$current_url\n- total()\n- sum(toInt(properties.$screen_width)) * 10\n- concat(event, " ", distinct_id)\n- ifElse(1 < 2, "small", "large")' + } autoFocus /> - {value ? 'Edit HogQL expression' : 'Add HogQL expression'} + {value ? 'Update HogQL expression' : 'Add HogQL expression'}
) diff --git a/frontend/src/queries/nodes/EventsNode/EventPropertyFilters.tsx b/frontend/src/queries/nodes/EventsNode/EventPropertyFilters.tsx index 8f92a8b4517da..3d88ced1d2d9a 100644 --- a/frontend/src/queries/nodes/EventsNode/EventPropertyFilters.tsx +++ b/frontend/src/queries/nodes/EventsNode/EventPropertyFilters.tsx @@ -2,6 +2,7 @@ import { EventsNode, EventsQuery } from '~/queries/schema' import { PropertyFilters } from 'lib/components/PropertyFilters/PropertyFilters' import { AnyPropertyFilter } from '~/types' import { useState } from 'react' +import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types' interface EventPropertyFiltersProps { query: EventsNode | EventsQuery @@ -14,6 +15,14 @@ export function EventPropertyFilters({ query, setQuery }: EventPropertyFiltersPr return !query.properties || Array.isArray(query.properties) ? ( setQuery?.({ ...query, properties: value })} pageKey={`EventPropertyFilters.${id}`} style={{ marginBottom: 0, marginTop: 0 }} From bb19848b86fb78bfec18e7a79d6645e23eed3e6e Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 00:05:20 +0100 Subject: [PATCH 06/19] make standard hogql filters it work on events page --- posthog/hogql/hogql.py | 5 +++-- posthog/models/event/query_event_list.py | 4 ++-- posthog/models/property/property.py | 4 ++++ posthog/models/property/util.py | 10 ++++++++++ 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/posthog/hogql/hogql.py b/posthog/hogql/hogql.py index 31f5ed15bb1c7..04556a652d85a 100644 --- a/posthog/hogql/hogql.py +++ b/posthog/hogql/hogql.py @@ -6,8 +6,6 @@ from clickhouse_driver.util.escape import escape_param -from posthog.models.property.util import get_property_string_expr - # fields you can select from in the events query EVENT_FIELDS = ["id", "uuid", "event", "timestamp", "distinct_id"] # "person.*" fields you can select from in the events query @@ -308,6 +306,9 @@ def translate_ast(node: ast.AST, stack: List[ast.AST], context: HogQLContext) -> def property_access_to_clickhouse(chain: List[str]): + # Circular import otherwise + from posthog.models.property.util import get_property_string_expr + """Given a list like ['properties', '$browser'] or ['uuid'], translate to the correct ClickHouse expr.""" if len(chain) == 2: if chain[0] == "properties": diff --git a/posthog/models/event/query_event_list.py b/posthog/models/event/query_event_list.py index 25cbf358e3008..fda3c1a62beea 100644 --- a/posthog/models/event/query_event_list.py +++ b/posthog/models/event/query_event_list.py @@ -80,6 +80,7 @@ def query_events_list( ) -> Union[List, EventsQueryResponse]: # Note: This code is inefficient and problematic, see https://github.com/PostHog/posthog/issues/13485 for details. # To isolate its impact from rest of the queries its queries are run on different nodes as part of "offline" workloads. + hogql_context = HogQLContext() limit += 1 limit_sql = "LIMIT %(limit)s" @@ -95,7 +96,7 @@ def query_events_list( } ) prop_filters, prop_filter_params = parse_prop_grouped_clauses( - team_id=team.pk, property_group=filter.property_groups, has_person_id_joined=False + team_id=team.pk, property_group=filter.property_groups, has_person_id_joined=False, hogql_context=hogql_context ) if action_id: @@ -135,7 +136,6 @@ def query_events_list( # events list v2 - hogql - hogql_context = HogQLContext() select_columns: List[str] = [] group_by_columns: List[str] = [] where_filters: List[str] = [] diff --git a/posthog/models/property/property.py b/posthog/models/property/property.py index 72c7b350e9b3a..a59ca5f18362d 100644 --- a/posthog/models/property/property.py +++ b/posthog/models/property/property.py @@ -41,6 +41,7 @@ class BehavioralPropertyType(str, Enum): "recording", "behavioral", "session", + "hogql", ] PropertyName = str @@ -92,6 +93,7 @@ class BehavioralPropertyType(str, Enum): "recording": ["key", "value"], "behavioral": ["key", "value"], "session": ["key", "value"], + "hogql": ["key"], } VALIDATE_BEHAVIORAL_PROP_TYPES = { @@ -224,6 +226,8 @@ def __init__( if value is None and self.operator in ["is_set", "is_not_set"]: self.value = self.operator + elif self.type == "hogql": + pass # keep value as None elif value is None: raise ValueError(f"Value must be set for property type {self.type} & operator {self.operator}") else: diff --git a/posthog/models/property/util.py b/posthog/models/property/util.py index c0144cc123342..b996dbaa0b3b8 100644 --- a/posthog/models/property/util.py +++ b/posthog/models/property/util.py @@ -10,6 +10,7 @@ from posthog.clickhouse.kafka_engine import trim_quotes_expr from posthog.clickhouse.materialized_columns import TableWithProperties, get_materialized_columns from posthog.constants import PropertyOperatorType +from posthog.hogql.hogql import HogQLContext from posthog.models.cohort import Cohort from posthog.models.cohort.util import ( format_cohort_subquery, @@ -54,6 +55,7 @@ def parse_prop_grouped_clauses( team_id: int, property_group: Optional[PropertyGroup], + hogql_context: Optional[HogQLContext] = None, prepend: str = "global", table_name: str = "", allow_denormalized_props: bool = True, @@ -82,6 +84,7 @@ def parse_prop_grouped_clauses( person_properties_mode=person_properties_mode, person_id_joined_alias=person_id_joined_alias, group_properties_joined=group_properties_joined, + hogql_context=hogql_context, _top_level=False, ) group_clauses.append(clause) @@ -102,6 +105,7 @@ def parse_prop_grouped_clauses( group_properties_joined=group_properties_joined, property_operator=property_group.type, team_id=team_id, + hogql_context=hogql_context, ) if not _final: @@ -124,6 +128,7 @@ def is_property_group(group: Union[Property, "PropertyGroup"]): def parse_prop_clauses( team_id: int, filters: List[Property], + hogql_context: Optional[HogQLContext], prepend: str = "global", table_name: str = "", allow_denormalized_props: bool = True, @@ -314,6 +319,11 @@ def parse_prop_clauses( filter_query, filter_params = get_session_property_filter_statement(prop, idx, prepend) final.append(f"{property_operator} {filter_query}") params.update(filter_params) + elif prop.type == "hogql": + from posthog.hogql.hogql import translate_hogql + + filter_query = translate_hogql(prop.key, hogql_context) + final.append(f"{property_operator} {filter_query}") if final: # remove the first operator From a5a2b1395aa7741b6359dd13bab2eb2f8108350f Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 00:07:40 +0100 Subject: [PATCH 07/19] this is count() --- frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx b/frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx index d4c6287666f4c..be470f8646685 100644 --- a/frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx +++ b/frontend/src/queries/QueryEditor/InlineHogQLEditor.tsx @@ -22,7 +22,7 @@ export function InlineHogQLEditor({ value, onChange }: InlineHogQLEditorProps): minRows={6} maxRows={6} placeholder={ - 'Enter HogQL Expression, such as:\n- properties.$current_url\n- total()\n- sum(toInt(properties.$screen_width)) * 10\n- concat(event, " ", distinct_id)\n- ifElse(1 < 2, "small", "large")' + 'Enter HogQL Expression, such as:\n- properties.$current_url\n- count()\n- sum(toInt(properties.$screen_width)) * 10\n- concat(event, " ", distinct_id)\n- ifElse(1 < 2, "small", "large")' } autoFocus /> From ca3b98e33fc110b2c66a9f596b33fe9417f971d9 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 00:13:35 +0100 Subject: [PATCH 08/19] filter based on flag --- .../TaxonomicFilter/taxonomicFilterLogic.tsx | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx b/frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx index 5eaaf2297d774..902d743d14672 100644 --- a/frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx +++ b/frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx @@ -145,13 +145,13 @@ export const taxonomicFilterLogic = kea({ (excludedProperties) => excludedProperties ?? {}, ], taxonomicGroups: [ - (selectors) => [ - selectors.currentTeamId, - selectors.groupAnalyticsTaxonomicGroups, - selectors.groupAnalyticsTaxonomicGroupNames, - selectors.eventNames, - selectors.excludedProperties, - featureFlagLogic.selectors.featureFlags, + (s) => [ + s.currentTeamId, + s.groupAnalyticsTaxonomicGroups, + s.groupAnalyticsTaxonomicGroupNames, + s.eventNames, + s.excludedProperties, + s.featureFlags, ], ( teamId, @@ -431,12 +431,17 @@ export const taxonomicFilterLogic = kea({ (activeTab, taxonomicGroups) => taxonomicGroups.find((g) => g.type === activeTab), ], taxonomicGroupTypes: [ - (selectors) => [(_, props) => props.taxonomicGroupTypes, selectors.taxonomicGroups], - (groupTypes, taxonomicGroups): TaxonomicFilterGroupType[] => - groupTypes || taxonomicGroups.map((g) => g.type), + (s, p) => [p.taxonomicGroupTypes, s.taxonomicGroups, s.featureFlags], + (groupTypes, taxonomicGroups, featureFlags): TaxonomicFilterGroupType[] => + (groupTypes || taxonomicGroups.map((g) => g.type)).filter((type) => { + return ( + type !== TaxonomicFilterGroupType.HogQLExpression || + !!featureFlags[FEATURE_FLAGS.HOGQL_EXPRESSIONS] + ) + }), ], groupAnalyticsTaxonomicGroupNames: [ - (selectors) => [selectors.groupTypes, selectors.currentTeamId, selectors.aggregationLabel], + (s) => [s.groupTypes, s.currentTeamId, s.aggregationLabel], (groupTypes, teamId, aggregationLabel): TaxonomicFilterGroup[] => groupTypes.map((type) => ({ name: `${capitalizeFirstLetter(aggregationLabel(type.group_type_index).plural)}`, @@ -453,7 +458,7 @@ export const taxonomicFilterLogic = kea({ })), ], groupAnalyticsTaxonomicGroups: [ - (selectors) => [selectors.groupTypes, selectors.currentTeamId, selectors.aggregationLabel], + (s) => [s.groupTypes, s.currentTeamId, s.aggregationLabel], (groupTypes, teamId, aggregationLabel): TaxonomicFilterGroup[] => groupTypes.map((type) => ({ name: `${capitalizeFirstLetter(aggregationLabel(type.group_type_index).singular)} properties`, @@ -521,6 +526,7 @@ export const taxonomicFilterLogic = kea({ return taxonomicGroup.searchPlaceholder }) return names + .filter((a) => !!a) .map( (name, index) => `${index !== 0 ? (index === searchGroupTypes.length - 1 ? ' or ' : ', ') : ''}${name}` From 5bb99fc095df37cd1e22879d4d2d4c5effb23921 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 00:14:56 +0100 Subject: [PATCH 09/19] add test for hogql filters in events query --- posthog/api/test/test_event.py | 43 ++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/posthog/api/test/test_event.py b/posthog/api/test/test_event.py index fbc275c4ddbce..2978e3e570bd8 100644 --- a/posthog/api/test/test_event.py +++ b/posthog/api/test/test_event.py @@ -779,3 +779,46 @@ def test_select_hogql_expressions(self): "results": [[2, "sign out"], [1, "sign up"]], }, ) + + @also_test_with_materialized_columns(["key"]) + @snapshot_clickhouse_queries + def test_hogql_property_filter(self): + with freeze_time("2020-01-10 12:00:00"): + _create_person( + properties={"email": "tom@posthog.com"}, + distinct_ids=["2", "some-random-uid"], + team=self.team, + immediate=True, + ) + _create_event(team=self.team, event="sign up", distinct_id="2", properties={"key": "test_val1"}) + with freeze_time("2020-01-10 12:11:00"): + _create_event(team=self.team, event="sign out", distinct_id="2", properties={"key": "test_val2"}) + with freeze_time("2020-01-10 12:12:00"): + _create_event(team=self.team, event="sign out", distinct_id="3", properties={"key": "test_val2"}) + with freeze_time("2020-01-10 12:13:00"): + _create_event(team=self.team, event="sign out", distinct_id="4", properties={"key": "test_val3"}) + flush_persons_and_events() + + with freeze_time("2020-01-10 12:14:00"): + select = ["event", "distinct_id", "properties.key", "'a%sd'", "concat(event, ' ', properties.key)"] + + response = self.client.get(f"/api/projects/{self.team.id}/events/?select={json.dumps(select)}").json() + self.assertEqual(len(response["results"]), 4) + + properties = [{"type": "hogql", "key": "'a%sd' == 'foo'"}] + response = self.client.get( + f"/api/projects/{self.team.id}/events/?select={json.dumps(select)}&properties={json.dumps(properties)}" + ).json() + self.assertEqual(len(response["results"]), 0) + + properties = [{"type": "hogql", "key": "'a%sd' == 'a%sd'"}] + response = self.client.get( + f"/api/projects/{self.team.id}/events/?select={json.dumps(select)}&properties={json.dumps(properties)}" + ).json() + self.assertEqual(len(response["results"]), 4) + + properties = [{"type": "hogql", "key": "properties.key == 'test_val2'"}] + response = self.client.get( + f"/api/projects/{self.team.id}/events/?select={json.dumps(select)}&properties={json.dumps(properties)}" + ).json() + self.assertEqual(len(response["results"]), 2) From 5142bd98f7a39e5eb548f14d6902cc590869b983 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 18 Jan 2023 23:21:11 +0000 Subject: [PATCH 10/19] Update snapshots --- .../api/test/__snapshots__/test_event.ambr | 260 ++++++++++++++++++ 1 file changed, 260 insertions(+) diff --git a/posthog/api/test/__snapshots__/test_event.ambr b/posthog/api/test/__snapshots__/test_event.ambr index 4558a23f9e57b..6eae8fdeea08f 100644 --- a/posthog/api/test/__snapshots__/test_event.ambr +++ b/posthog/api/test/__snapshots__/test_event.ambr @@ -186,6 +186,266 @@ LIMIT 10 ' --- +# name: TestEvents.test_hogql_property_filter + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), + 'a%sd', + concat(event, ' ', replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', '')) + FROM events + WHERE team_id = 2 + AND timestamp > '2020-01-09 12:14:00.000000' + AND timestamp < '2020-01-10 12:14:05.000000' + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter.1 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), + 'a%sd', + concat(event, ' ', replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', '')) + FROM events + WHERE team_id = 2 + AND timestamp < '2020-01-10 12:14:05.000000' + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter.2 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), + 'a%sd', + concat(event, ' ', replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', '')) + FROM events + WHERE team_id = 2 + AND timestamp > '2020-01-09 12:14:00.000000' + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals('a%sd', 'foo')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter.3 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), + 'a%sd', + concat(event, ' ', replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', '')) + FROM events + WHERE team_id = 2 + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals('a%sd', 'foo')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter.4 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), + 'a%sd', + concat(event, ' ', replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', '')) + FROM events + WHERE team_id = 2 + AND timestamp > '2020-01-09 12:14:00.000000' + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals('a%sd', 'a%sd')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter.5 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), + 'a%sd', + concat(event, ' ', replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', '')) + FROM events + WHERE team_id = 2 + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals('a%sd', 'a%sd')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter.6 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), + 'a%sd', + concat(event, ' ', replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', '')) + FROM events + WHERE team_id = 2 + AND timestamp > '2020-01-09 12:14:00.000000' + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals(replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), 'test_val2')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter.7 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), + 'a%sd', + concat(event, ' ', replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', '')) + FROM events + WHERE team_id = 2 + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals(replaceRegexpAll(JSONExtractRaw(properties, 'key'), '^"|"$', ''), 'test_val2')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter_materialized + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + "mat_key", + 'a%sd', + concat(event, ' ', "mat_key") + FROM events + WHERE team_id = 2 + AND timestamp > '2020-01-09 12:14:00.000000' + AND timestamp < '2020-01-10 12:14:05.000000' + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter_materialized.1 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + "mat_key", + 'a%sd', + concat(event, ' ', "mat_key") + FROM events + WHERE team_id = 2 + AND timestamp < '2020-01-10 12:14:05.000000' + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter_materialized.2 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + "mat_key", + 'a%sd', + concat(event, ' ', "mat_key") + FROM events + WHERE team_id = 2 + AND timestamp > '2020-01-09 12:14:00.000000' + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals('a%sd', 'foo')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter_materialized.3 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + "mat_key", + 'a%sd', + concat(event, ' ', "mat_key") + FROM events + WHERE team_id = 2 + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals('a%sd', 'foo')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter_materialized.4 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + "mat_key", + 'a%sd', + concat(event, ' ', "mat_key") + FROM events + WHERE team_id = 2 + AND timestamp > '2020-01-09 12:14:00.000000' + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals('a%sd', 'a%sd')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter_materialized.5 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + "mat_key", + 'a%sd', + concat(event, ' ', "mat_key") + FROM events + WHERE team_id = 2 + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals('a%sd', 'a%sd')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter_materialized.6 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + "mat_key", + 'a%sd', + concat(event, ' ', "mat_key") + FROM events + WHERE team_id = 2 + AND timestamp > '2020-01-09 12:14:00.000000' + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals("mat_key", 'test_val2')) + ORDER BY event ASC + LIMIT 101 + ' +--- +# name: TestEvents.test_hogql_property_filter_materialized.7 + ' + /* user_id:0 request:_snapshot_ */ + SELECT event, + distinct_id, + "mat_key", + 'a%sd', + concat(event, ' ', "mat_key") + FROM events + WHERE team_id = 2 + AND timestamp < '2020-01-10 12:14:05.000000' + AND (equals("mat_key", 'test_val2')) + ORDER BY event ASC + LIMIT 101 + ' +--- # name: TestEvents.test_select_hogql_expressions ' /* user_id:0 request:_snapshot_ */ From ed8b56096244365779bfc98286f2983147d6ce7c Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 00:21:38 +0100 Subject: [PATCH 11/19] fix merged path --- frontend/src/queries/nodes/InsightViz/Breakdown.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/queries/nodes/InsightViz/Breakdown.tsx b/frontend/src/queries/nodes/InsightViz/Breakdown.tsx index 0c59070a0fb49..74b497c86deaf 100644 --- a/frontend/src/queries/nodes/InsightViz/Breakdown.tsx +++ b/frontend/src/queries/nodes/InsightViz/Breakdown.tsx @@ -1,6 +1,6 @@ import { useValues, useActions } from 'kea' import { QueryEditorFilterProps } from '~/types' -import { BreakdownFilter } from 'scenes/insights/filters/BreakdownFilter' +import { TaxonomicBreakdownFilter } from 'scenes/insights/filters/BreakdownFilter/TaxonomicBreakdownFilter' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { FEATURE_FLAGS } from 'lib/constants' import { isTrendsQuery } from '~/queries/utils' @@ -19,5 +19,5 @@ export function Breakdown({ insightProps, query }: QueryEditorFilterProps): JSX. const setFilters = (breakdown: BreakdownFilterType): void => { updateBreakdown(breakdown) } - return + return } From 4b1303697203b925d7e4bb4d88f7859c82bf155b Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 00:29:39 +0100 Subject: [PATCH 12/19] revert default factories (fix mypy) --- posthog/hogql/hogql.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/posthog/hogql/hogql.py b/posthog/hogql/hogql.py index 04556a652d85a..e0618705f28cf 100644 --- a/posthog/hogql/hogql.py +++ b/posthog/hogql/hogql.py @@ -120,9 +120,9 @@ class HogQLContext: """Context given to a HogQL expression parser""" # If set, will save string constants to this dict. Inlines strings into the query if None. - values: Optional[Dict[str, Any]] = field(default_factory=lambda: {}) + values: Optional[Dict[str, Any]] = field(default_factory=dict) # List of field and property accesses found in the expression - attribute_list: List[List[str]] = field(default_factory=lambda: []) + attribute_list: List[List[str]] = field(default_factory=list) # Did the last calls to translate_hogql since setting this to False contain any HOGQL_AGGREGATIONS found_aggregation: bool = False From 6105cfefb184eaa071f08179cc657f88aa7af474 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 09:05:59 +0100 Subject: [PATCH 13/19] mypy --- posthog/hogql/hogql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/posthog/hogql/hogql.py b/posthog/hogql/hogql.py index e0618705f28cf..ef21db8503dfa 100644 --- a/posthog/hogql/hogql.py +++ b/posthog/hogql/hogql.py @@ -120,7 +120,7 @@ class HogQLContext: """Context given to a HogQL expression parser""" # If set, will save string constants to this dict. Inlines strings into the query if None. - values: Optional[Dict[str, Any]] = field(default_factory=dict) + values: Optional[Dict] = field(default_factory=dict) # List of field and property accesses found in the expression attribute_list: List[List[str]] = field(default_factory=list) # Did the last calls to translate_hogql since setting this to False contain any HOGQL_AGGREGATIONS From 7f423d697e3a063cc6ba93c064bf6da4b428e7e8 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 10:08:43 +0100 Subject: [PATCH 14/19] feat(data-exploration): improve data table crashes --- frontend/src/initKea.ts | 1 + .../queries/nodes/DataNode/ElapsedTime.tsx | 4 +- .../queries/nodes/DataNode/dataNodeLogic.ts | 19 +++++--- .../src/queries/nodes/DataTable/DataTable.tsx | 14 +++--- .../queries/nodes/DataTable/dataTableLogic.ts | 43 +++++++++++++++---- .../queries/nodes/DataTable/renderColumn.tsx | 9 +++- 6 files changed, 67 insertions(+), 23 deletions(-) diff --git a/frontend/src/initKea.ts b/frontend/src/initKea.ts index b086f67088ab3..103c44a65ba68 100644 --- a/frontend/src/initKea.ts +++ b/frontend/src/initKea.ts @@ -22,6 +22,7 @@ const ERROR_FILTER_WHITELIST = [ 'signup', // Special error handling on login 'loadLatestVersion', 'loadBilling', // Gracefully handled if it fails + 'loadData', // Gracefully handled in the data table ] interface InitKeaProps { diff --git a/frontend/src/queries/nodes/DataNode/ElapsedTime.tsx b/frontend/src/queries/nodes/DataNode/ElapsedTime.tsx index b5258c64a629f..d44ec9b32d34f 100644 --- a/frontend/src/queries/nodes/DataNode/ElapsedTime.tsx +++ b/frontend/src/queries/nodes/DataNode/ElapsedTime.tsx @@ -3,7 +3,7 @@ import { dataNodeLogic } from '~/queries/nodes/DataNode/dataNodeLogic' import { useState } from 'react' export function ElapsedTime(): JSX.Element | null { - const { elapsedTime, loadingStart } = useValues(dataNodeLogic) + const { elapsedTime, loadingStart, responseError } = useValues(dataNodeLogic) const [, setTick] = useState(0) let time = elapsedTime @@ -19,5 +19,5 @@ export function ElapsedTime(): JSX.Element | null { return null } - return
{`${(time / 1000).toFixed(time < 1000 ? 2 : 1)}s`}
+ return
{`${(time / 1000).toFixed(time < 1000 ? 2 : 1)}s`}
} diff --git a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts index 313f9cc9106a5..6a5b13d58607b 100644 --- a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts +++ b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts @@ -62,13 +62,14 @@ export const dataNodeLogic = kea([ const methodOptions: ApiMethodOptions = { signal: cache.abortController.signal, } + const now = performance.now() try { - const now = performance.now() const data = (await query(props.query, methodOptions, refresh)) ?? null breakpoint() actions.setElapsedTime(performance.now() - now) return data } catch (e: any) { + actions.setElapsedTime(performance.now() - now) if (e.name === 'AbortError' || e.message?.name === 'AbortError') { return values.response } else { @@ -161,6 +162,14 @@ export const dataNodeLogic = kea([ loadNextData: () => performance.now(), }, ], + responseError: [ + null as string | null, + { + loadData: () => null, + loadDataFailure: () => 'Error loading data', + loadDataSuccess: () => null, + }, + ], elapsedTime: [ null as number | null, { @@ -199,9 +208,9 @@ export const dataNodeLogic = kea([ ], canLoadNewData: [(s) => [s.newQuery], (newQuery) => !!newQuery], nextQuery: [ - (s, p) => [p.query, s.response], - (query, response): DataNode | null => { - if (isEventsQuery(query)) { + (s, p) => [p.query, s.response, s.responseError], + (query, response, responseError): DataNode | null => { + if (isEventsQuery(query) && !responseError) { if ((response as EventsQuery['response'])?.hasMore) { const sortKey = query.orderBy?.[0] ?? '-timestamp' const typedResults = (response as EventsQuery['response'])?.results @@ -225,7 +234,7 @@ export const dataNodeLogic = kea([ } } } - if (isPersonsNode(query) && response) { + if (isPersonsNode(query) && response && !responseError) { const personsResults = (response as PersonsNode['response'])?.results const nextQuery: PersonsNode = { ...query, diff --git a/frontend/src/queries/nodes/DataTable/DataTable.tsx b/frontend/src/queries/nodes/DataTable/DataTable.tsx index e5c328dde6cda..50a87fa08ffe1 100644 --- a/frontend/src/queries/nodes/DataTable/DataTable.tsx +++ b/frontend/src/queries/nodes/DataTable/DataTable.tsx @@ -31,6 +31,7 @@ import { LemonButton } from 'lib/components/LemonButton' import { TaxonomicPopup } from 'lib/components/TaxonomicPopup/TaxonomicPopup' import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types' import { extractExpressionComment, removeExpressionComment } from '~/queries/nodes/DataTable/utils' +import { InsightEmptyState, InsightErrorState } from 'scenes/insights/EmptyStates' interface DataTableProps { query: DataTableNode @@ -40,10 +41,10 @@ interface DataTableProps { } const groupTypes = [ + TaxonomicFilterGroupType.HogQLExpression, TaxonomicFilterGroupType.EventProperties, TaxonomicFilterGroupType.PersonProperties, TaxonomicFilterGroupType.EventFeatureFlags, - TaxonomicFilterGroupType.HogQLExpression, ] let uniqueNode = 0 @@ -57,6 +58,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele const { response, responseLoading, + responseError, canLoadNextData, canLoadNewData, nextDataLoading, @@ -65,9 +67,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele } = useValues(builtDataNodeLogic) const dataTableLogicProps: DataTableLogicProps = { query, key } - const { resultsWithLabelRows, columnsInQuery, columnsInResponse, queryWithDefaults, canSort } = useValues( - dataTableLogic(dataTableLogicProps) - ) + const { resultsWithLabelRows, columns, queryWithDefaults, canSort } = useValues(dataTableLogic(dataTableLogicProps)) const { showActions, @@ -83,7 +83,6 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele expandable, } = queryWithDefaults - const columns: string[] = columnsInResponse ?? columnsInQuery const actionsColumnShown = showActions && isEventsQuery(query.source) && columns.includes('*') const lemonColumns: LemonTableColumn | any[], any>[] = [ ...columns.map( @@ -184,7 +183,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele ) : null} Add column left} data-attr="datatable-add-column-left" @@ -209,7 +208,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele buttonProps={{ type: undefined }} /> Add column right} data-attr="datatable-add-column-right" @@ -394,6 +393,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele }} sorting={null} useURLForSorting={false} + emptyState={responseError ? : } expandable={ expandable && isEventsQuery(query.source) && columns.includes('*') ? { diff --git a/frontend/src/queries/nodes/DataTable/dataTableLogic.ts b/frontend/src/queries/nodes/DataTable/dataTableLogic.ts index ea3a3118f1331..4163107056010 100644 --- a/frontend/src/queries/nodes/DataTable/dataTableLogic.ts +++ b/frontend/src/queries/nodes/DataTable/dataTableLogic.ts @@ -15,6 +15,8 @@ export interface DataTableLogicProps { } export const categoryRowKey = Symbol('__categoryRow__') +export const loadingColumn = Symbol('...') +export const errorColumn = Symbol('Error!') export const dataTableLogic = kea([ props({} as DataTableLogicProps), @@ -37,7 +39,7 @@ export const dataTableLogic = kea([ featureFlagLogic, ['featureFlags'], dataNodeLogic({ key: props.key, query: props.query.source }), - ['response', 'responseLoading'], + ['response', 'responseLoading', 'responseError'], ], })), selectors({ @@ -57,19 +59,42 @@ export const dataTableLogic = kea([ ? (response?.columns as string[]) : null, ], + columns: [ + (s) => [s.columnsInResponse, s.columnsInQuery], + // always show columns in the query (what the user entered), and transform results to match below + (columnsInResponse, columnsInQuery): string[] => columnsInQuery ?? columnsInResponse ?? [], + ], resultsWithLabelRows: [ - (s) => [s.sourceKind, s.orderBy, s.response, s.columnsInResponse, s.columnsInQuery], - (sourceKind, orderBy, response, columnsInResponse, columnsInQuery): any[] | null => { - if (sourceKind === NodeKind.EventsQuery) { - const results = (response as EventsQuery['response'] | null)?.results - if (results) { + (s) => [s.sourceKind, s.orderBy, s.response, s.columns, s.responseError], + (sourceKind, orderBy, response: AnyDataNode['response'], columns, responseError): any[] | null => { + if (response && sourceKind === NodeKind.EventsQuery) { + const eventsQueryResponse = response as EventsQuery['response'] | null + if (eventsQueryResponse) { + const { results, columns: columnsInResponse } = eventsQueryResponse const orderKey = orderBy?.[0]?.startsWith('-') ? orderBy[0].slice(1) : orderBy?.[0] - const orderKeyIndex = (columnsInResponse ?? columnsInQuery).findIndex( + const orderKeyIndex = columns.findIndex( (column) => removeExpressionComment(column) === orderKey || removeExpressionComment(column) === `-${orderKey}` ) + // if we errored by adding a new column, show the new columns with the old results + // if the columns changed in any other way, return no results + if ( + responseError && + columnsInResponse && + (columns.length <= columnsInResponse.length || + columnsInResponse.find((c) => !columns.includes(c))) + ) { + return [] + } + + const columnMap = Object.fromEntries(columnsInResponse.map((c, i) => [c, i])) + const convertResultToDisplayedColumns = (result: any[]): any[] => + columns.map((c) => + c in columnMap ? result[columnMap[c]] : responseError ? errorColumn : loadingColumn + ) + // Add a label between results if the day changed if (orderKey === 'timestamp' && orderKeyIndex !== -1) { let lastResult: any | null = null @@ -84,10 +109,12 @@ export const dataTableLogic = kea([ [categoryRowKey]: dayjs(result[orderKeyIndex]).format('LL'), }) } - newResults.push(result) + newResults.push(convertResultToDisplayedColumns(result)) lastResult = result } return newResults + } else { + return results.map((result) => convertResultToDisplayedColumns(result)) } } } diff --git a/frontend/src/queries/nodes/DataTable/renderColumn.tsx b/frontend/src/queries/nodes/DataTable/renderColumn.tsx index 0f1d62cc1b8bd..13b2d74396c58 100644 --- a/frontend/src/queries/nodes/DataTable/renderColumn.tsx +++ b/frontend/src/queries/nodes/DataTable/renderColumn.tsx @@ -12,6 +12,9 @@ import { combineUrl, router } from 'kea-router' import { CopyToClipboardInline } from 'lib/components/CopyToClipboard' import { DeletePersonButton } from '~/queries/nodes/PersonsNode/DeletePersonButton' import ReactJson from 'react-json-view' +import { errorColumn, loadingColumn } from '~/queries/nodes/DataTable/dataTableLogic' +import { Spinner } from 'lib/components/Spinner/Spinner' +import { Tag } from 'antd' export function renderColumn( key: string, @@ -21,7 +24,11 @@ export function renderColumn( setQuery?: (query: DataTableNode) => void, context?: QueryContext ): JSX.Element | string { - if (key === 'event' && isEventsQuery(query.source)) { + if (value === loadingColumn) { + return + } else if (value === errorColumn) { + return Error + } else if (key === 'event' && isEventsQuery(query.source)) { const resultRow = record as any[] const eventRecord = query.source.select.includes('*') ? resultRow[query.source.select.indexOf('*')] : null From 797fe336cfb8b33cc1c428f96352c95480c2ebd2 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 11:00:00 +0100 Subject: [PATCH 15/19] fix row highlighting --- .../src/queries/nodes/DataTable/DataTable.tsx | 10 +++---- .../queries/nodes/DataTable/dataTableLogic.ts | 28 +++++++++++++++---- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/frontend/src/queries/nodes/DataTable/DataTable.tsx b/frontend/src/queries/nodes/DataTable/DataTable.tsx index 50a87fa08ffe1..5a589fc80832b 100644 --- a/frontend/src/queries/nodes/DataTable/DataTable.tsx +++ b/frontend/src/queries/nodes/DataTable/DataTable.tsx @@ -63,11 +63,11 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele canLoadNewData, nextDataLoading, newDataLoading, - highlightedRows, } = useValues(builtDataNodeLogic) const dataTableLogicProps: DataTableLogicProps = { query, key } - const { resultsWithLabelRows, columns, queryWithDefaults, canSort } = useValues(dataTableLogic(dataTableLogicProps)) + const { resultsWithLabelRows, columns, columnsInResponse, queryWithDefaults, canSort, isRowHighlighted } = + useValues(dataTableLogic(dataTableLogicProps)) const { showActions, @@ -278,8 +278,8 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele if (categoryRowKey in record) { return { props: { colSpan: 0 } } } - if (isEventsQuery(query.source) && columns.includes('*')) { - return + if (isEventsQuery(query.source) && columnsInResponse?.includes('*')) { + return } return null }, @@ -415,7 +415,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele } rowClassName={(row) => clsx('DataTable__row', { - 'DataTable__row--highlight_once': row && highlightedRows.has(row), + 'DataTable__row--highlight_once': row && isRowHighlighted(row), 'DataTable__row--category_row': row && categoryRowKey in row, }) } diff --git a/frontend/src/queries/nodes/DataTable/dataTableLogic.ts b/frontend/src/queries/nodes/DataTable/dataTableLogic.ts index 4163107056010..701d8d01c0065 100644 --- a/frontend/src/queries/nodes/DataTable/dataTableLogic.ts +++ b/frontend/src/queries/nodes/DataTable/dataTableLogic.ts @@ -8,6 +8,7 @@ import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { FEATURE_FLAGS } from 'lib/constants' import { dataNodeLogic } from '~/queries/nodes/DataNode/dataNodeLogic' import { dayjs } from 'lib/dayjs' +import equal from 'fast-deep-equal' export interface DataTableLogicProps { key: string @@ -39,7 +40,7 @@ export const dataTableLogic = kea([ featureFlagLogic, ['featureFlags'], dataNodeLogic({ key: props.key, query: props.query.source }), - ['response', 'responseLoading', 'responseError'], + ['response', 'responseLoading', 'responseError', 'highlightedRows'], ], })), selectors({ @@ -90,10 +91,19 @@ export const dataTableLogic = kea([ } const columnMap = Object.fromEntries(columnsInResponse.map((c, i) => [c, i])) - const convertResultToDisplayedColumns = (result: any[]): any[] => - columns.map((c) => - c in columnMap ? result[columnMap[c]] : responseError ? errorColumn : loadingColumn - ) + const convertResultToDisplayedColumns = equal(columns, columnsInResponse) + ? (result: any[]) => result + : (result: any[]): any[] => { + const newResult = columns.map((c) => + c in columnMap + ? result[columnMap[c]] + : responseError + ? errorColumn + : loadingColumn + ) + ;(newResult as any).__originalResultRow = result + return newResult + } // Add a label between results if the day changed if (orderKey === 'timestamp' && orderKeyIndex !== -1) { @@ -121,6 +131,14 @@ export const dataTableLogic = kea([ return response && 'results' in response ? (response as any).results ?? null : null }, ], + isRowHighlighted: [ + (s) => [s.highlightedRows], + (highlightedRows) => + (row: any[]): boolean => + row + ? highlightedRows.has('__originalResultRow' in row ? (row as any).__originalResultRow : row) + : false, + ], queryWithDefaults: [ (s, p) => [p.query, s.columnsInQuery, s.featureFlags], (query: DataTableNode, columnsInQuery, featureFlags): Required => { From 6a072ea74cde7aaa5481bf9604a0de9276038882 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 11:13:01 +0100 Subject: [PATCH 16/19] be explicit about columns --- .../src/queries/nodes/DataTable/DataTable.tsx | 40 ++++++++++--------- .../nodes/DataTable/EventRowActions.tsx | 2 +- .../queries/nodes/DataTable/dataTableLogic.ts | 19 ++++----- 3 files changed, 30 insertions(+), 31 deletions(-) diff --git a/frontend/src/queries/nodes/DataTable/DataTable.tsx b/frontend/src/queries/nodes/DataTable/DataTable.tsx index 5a589fc80832b..ab5afc890d3e0 100644 --- a/frontend/src/queries/nodes/DataTable/DataTable.tsx +++ b/frontend/src/queries/nodes/DataTable/DataTable.tsx @@ -66,7 +66,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele } = useValues(builtDataNodeLogic) const dataTableLogicProps: DataTableLogicProps = { query, key } - const { resultsWithLabelRows, columns, columnsInResponse, queryWithDefaults, canSort, isRowHighlighted } = + const { resultsWithLabelRows, columnsInQuery, columnsInResponse, queryWithDefaults, canSort, isRowHighlighted } = useValues(dataTableLogic(dataTableLogicProps)) const { @@ -83,9 +83,9 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele expandable, } = queryWithDefaults - const actionsColumnShown = showActions && isEventsQuery(query.source) && columns.includes('*') + const actionsColumnShown = showActions && isEventsQuery(query.source) && columnsInResponse?.includes('*') const lemonColumns: LemonTableColumn | any[], any>[] = [ - ...columns.map( + ...columnsInQuery.map( (key, index) => ({ dataIndex: key as any, @@ -95,7 +95,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele if (index === (expandable ? 1 : 0)) { return { children: record[categoryRowKey], - props: { colSpan: columns.length + (actionsColumnShown ? 1 : 0) }, + props: { colSpan: columnsInQuery.length + (actionsColumnShown ? 1 : 0) }, } } else { return { props: { colSpan: 0 } } @@ -232,7 +232,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele groupTypes={groupTypes} buttonProps={{ type: undefined }} /> - {columns.filter((c) => c !== '*').length > 1 ? ( + {columnsInQuery.filter((c) => c !== '*').length > 1 ? ( <> { if (categoryRowKey in record) { return `__category_row__${rowIndex}` } if (isEventsQuery(query.source)) { - if (columns.includes('*')) { - return record[columns.indexOf('*')].uuid - } else if (columns.includes('uuid')) { - return record[columns.indexOf('uuid')] - } else if (columns.includes('id')) { - return record[columns.indexOf('id')] + if ((columnsInResponse ?? columnsInQuery).includes('*')) { + return record[(columnsInResponse ?? columnsInQuery).indexOf('*')].uuid + } else if ((columnsInResponse ?? columnsInQuery).includes('uuid')) { + return record[(columnsInResponse ?? columnsInQuery).indexOf('uuid')] + } else if ((columnsInResponse ?? columnsInQuery).includes('id')) { + return record[(columnsInResponse ?? columnsInQuery).indexOf('id')] } return JSON.stringify(record) } else { @@ -395,20 +399,20 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele useURLForSorting={false} emptyState={responseError ? : } expandable={ - expandable && isEventsQuery(query.source) && columns.includes('*') + expandable && isEventsQuery(query.source) && columnsInResponse?.includes('*') ? { - expandedRowRender: function renderExpand(event) { - if (isEventsQuery(query.source) && Array.isArray(event)) { + expandedRowRender: function renderExpand(record) { + if (isEventsQuery(query.source) && Array.isArray(record)) { return ( ) } - return event ? : null + return record ? : null }, - rowExpandable: (event) => !(categoryRowKey in event), + rowExpandable: (record) => !(categoryRowKey in record), noIndent: true, } : undefined diff --git a/frontend/src/queries/nodes/DataTable/EventRowActions.tsx b/frontend/src/queries/nodes/DataTable/EventRowActions.tsx index 258abbf2c53ff..b7762693f4e5a 100644 --- a/frontend/src/queries/nodes/DataTable/EventRowActions.tsx +++ b/frontend/src/queries/nodes/DataTable/EventRowActions.tsx @@ -78,7 +78,7 @@ export function EventRowActions({ event }: EventActionProps): JSX.Element { Create action from event )} - {!!event.properties.$session_id && ( + {!!event.properties?.$session_id && ( ([ ? (response?.columns as string[]) : null, ], - columns: [ - (s) => [s.columnsInResponse, s.columnsInQuery], - // always show columns in the query (what the user entered), and transform results to match below - (columnsInResponse, columnsInQuery): string[] => columnsInQuery ?? columnsInResponse ?? [], - ], resultsWithLabelRows: [ - (s) => [s.sourceKind, s.orderBy, s.response, s.columns, s.responseError], - (sourceKind, orderBy, response: AnyDataNode['response'], columns, responseError): any[] | null => { + (s) => [s.sourceKind, s.orderBy, s.response, s.columnsInQuery, s.responseError], + (sourceKind, orderBy, response: AnyDataNode['response'], columnsInQuery, responseError): any[] | null => { if (response && sourceKind === NodeKind.EventsQuery) { const eventsQueryResponse = response as EventsQuery['response'] | null if (eventsQueryResponse) { const { results, columns: columnsInResponse } = eventsQueryResponse const orderKey = orderBy?.[0]?.startsWith('-') ? orderBy[0].slice(1) : orderBy?.[0] - const orderKeyIndex = columns.findIndex( + const orderKeyIndex = columnsInResponse.findIndex( (column) => removeExpressionComment(column) === orderKey || removeExpressionComment(column) === `-${orderKey}` @@ -84,17 +79,17 @@ export const dataTableLogic = kea([ if ( responseError && columnsInResponse && - (columns.length <= columnsInResponse.length || - columnsInResponse.find((c) => !columns.includes(c))) + (columnsInQuery.length <= columnsInResponse.length || + columnsInResponse.find((c) => !columnsInQuery.includes(c))) ) { return [] } const columnMap = Object.fromEntries(columnsInResponse.map((c, i) => [c, i])) - const convertResultToDisplayedColumns = equal(columns, columnsInResponse) + const convertResultToDisplayedColumns = equal(columnsInQuery, columnsInResponse) ? (result: any[]) => result : (result: any[]): any[] => { - const newResult = columns.map((c) => + const newResult = columnsInQuery.map((c) => c in columnMap ? result[columnMap[c]] : responseError From 38aed5c287e896d13d542080ff9e3ad7d3c7b090 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 11:34:50 +0100 Subject: [PATCH 17/19] simplify --- frontend/src/queries/nodes/DataTable/DataTable.tsx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/frontend/src/queries/nodes/DataTable/DataTable.tsx b/frontend/src/queries/nodes/DataTable/DataTable.tsx index ab5afc890d3e0..705a0b831c90e 100644 --- a/frontend/src/queries/nodes/DataTable/DataTable.tsx +++ b/frontend/src/queries/nodes/DataTable/DataTable.tsx @@ -369,7 +369,7 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele loading={responseLoading && !nextDataLoading && !newDataLoading} columns={lemonColumns} key={ - (columnsInResponse ?? columnsInQuery).join( + [...(columnsInResponse ?? []), ...columnsInQuery].join( '::' ) /* Bust the LemonTable cache when columns change */ } @@ -379,12 +379,12 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele return `__category_row__${rowIndex}` } if (isEventsQuery(query.source)) { - if ((columnsInResponse ?? columnsInQuery).includes('*')) { - return record[(columnsInResponse ?? columnsInQuery).indexOf('*')].uuid - } else if ((columnsInResponse ?? columnsInQuery).includes('uuid')) { - return record[(columnsInResponse ?? columnsInQuery).indexOf('uuid')] - } else if ((columnsInResponse ?? columnsInQuery).includes('id')) { - return record[(columnsInResponse ?? columnsInQuery).indexOf('id')] + if (columnsInResponse?.includes('*')) { + return record[columnsInResponse.indexOf('*')].uuid + } else if (columnsInResponse?.includes('uuid')) { + return record[columnsInResponse.indexOf('uuid')] + } else if (columnsInResponse?.includes('id')) { + return record[columnsInResponse.indexOf('id')] } return JSON.stringify(record) } else { From 753949f398c29d592a8a08ca4cd82abe8532ceca Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Thu, 19 Jan 2023 14:23:13 +0100 Subject: [PATCH 18/19] use a dataclass --- .../src/queries/nodes/DataTable/DataTable.tsx | 422 +++++++++--------- .../nodes/DataTable/dataTableLogic.test.ts | 22 +- .../queries/nodes/DataTable/dataTableLogic.ts | 49 +- .../queries/nodes/DataTable/renderColumn.tsx | 4 +- 4 files changed, 248 insertions(+), 249 deletions(-) diff --git a/frontend/src/queries/nodes/DataTable/DataTable.tsx b/frontend/src/queries/nodes/DataTable/DataTable.tsx index 705a0b831c90e..e0e8de812ec00 100644 --- a/frontend/src/queries/nodes/DataTable/DataTable.tsx +++ b/frontend/src/queries/nodes/DataTable/DataTable.tsx @@ -14,7 +14,7 @@ import { LoadNext } from '~/queries/nodes/DataNode/LoadNext' import { renderColumnMeta } from '~/queries/nodes/DataTable/renderColumnMeta' import { renderColumn } from '~/queries/nodes/DataTable/renderColumn' import { AutoLoad } from '~/queries/nodes/DataNode/AutoLoad' -import { categoryRowKey, dataTableLogic, DataTableLogicProps } from '~/queries/nodes/DataTable/dataTableLogic' +import { dataTableLogic, DataTableLogicProps, DataTableRow } from '~/queries/nodes/DataTable/dataTableLogic' import { ColumnConfigurator } from '~/queries/nodes/DataTable/ColumnConfigurator/ColumnConfigurator' import { LemonDivider } from 'lib/components/LemonDivider' import { EventBufferNotice } from 'scenes/events/EventBufferNotice' @@ -32,6 +32,7 @@ import { TaxonomicPopup } from 'lib/components/TaxonomicPopup/TaxonomicPopup' import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types' import { extractExpressionComment, removeExpressionComment } from '~/queries/nodes/DataTable/utils' import { InsightEmptyState, InsightErrorState } from 'scenes/insights/EmptyStates' +import { EventType } from '~/types' interface DataTableProps { query: DataTableNode @@ -63,11 +64,13 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele canLoadNewData, nextDataLoading, newDataLoading, + highlightedRows, } = useValues(builtDataNodeLogic) const dataTableLogicProps: DataTableLogicProps = { query, key } - const { resultsWithLabelRows, columnsInQuery, columnsInResponse, queryWithDefaults, canSort, isRowHighlighted } = - useValues(dataTableLogic(dataTableLogicProps)) + const { dataTableRows, columnsInQuery, columnsInResponse, queryWithDefaults, canSort } = useValues( + dataTableLogic(dataTableLogicProps) + ) const { showActions, @@ -84,202 +87,199 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele } = queryWithDefaults const actionsColumnShown = showActions && isEventsQuery(query.source) && columnsInResponse?.includes('*') - const lemonColumns: LemonTableColumn | any[], any>[] = [ - ...columnsInQuery.map( - (key, index) => - ({ - dataIndex: key as any, - ...renderColumnMeta(key, query, context), - render: function RenderDataTableColumn(_: any, record: Record | any[]) { - if (categoryRowKey in record) { - if (index === (expandable ? 1 : 0)) { - return { - children: record[categoryRowKey], - props: { colSpan: columnsInQuery.length + (actionsColumnShown ? 1 : 0) }, - } - } else { - return { props: { colSpan: 0 } } - } - } else if (isEventsQuery(query.source)) { - return renderColumn(key, record[index], record, query, setQuery, context) + const lemonColumns: LemonTableColumn[] = [ + ...columnsInQuery.map((key, index) => ({ + dataIndex: key as any, + ...renderColumnMeta(key, query, context), + render: function RenderDataTableColumn(_: any, { result, label }: DataTableRow) { + if (label) { + if (index === (expandable ? 1 : 0)) { + return { + children: label, + props: { colSpan: columnsInQuery.length + (actionsColumnShown ? 1 : 0) }, } - return renderColumn(key, record[key], record, query, setQuery, context) - }, - sorter: undefined, // using custom sorting code - more: - showActions && isEventsQuery(query.source) ? ( + } else { + return { props: { colSpan: 0 } } + } + } else if (result) { + if (isEventsQuery(query.source)) { + return renderColumn(key, result[index], result, query, setQuery, context) + } + return renderColumn(key, result[key], result, query, setQuery, context) + } + }, + sorter: undefined, // using custom sorting code + more: + showActions && isEventsQuery(query.source) ? ( + <> +
+
{extractExpressionComment(key)}
+ {extractExpressionComment(key) !== removeExpressionComment(key) && ( +
{removeExpressionComment(key)}
+ )} +
+ + <>Edit column} + onChange={(v, g) => { + const hogQl = taxonomicFilterToHogQl(g, v) + if (hogQl && isEventsQuery(query.source)) { + const isAggregation = isHogQlAggregation(hogQl) + const isOrderBy = query.source?.orderBy?.[0] === key + const isDescOrderBy = query.source?.orderBy?.[0] === `-${key}` + setQuery?.({ + ...query, + source: { + ...query.source, + select: query.source.select + .map((s, i) => (i === index ? hogQl : s)) + .filter((c) => (isAggregation ? c !== '*' : true)), + orderBy: + isOrderBy || isDescOrderBy + ? [isDescOrderBy ? `-${hogQl}` : hogQl] + : query.source?.orderBy, + }, + }) + } + }} + groupTypes={groupTypes} + buttonProps={{ type: undefined }} + /> + + {canSort ? ( <> -
-
{extractExpressionComment(key)}
- {extractExpressionComment(key) !== removeExpressionComment(key) && ( -
{removeExpressionComment(key)}
- )} -
- - <>Edit column} - onChange={(v, g) => { - const hogQl = taxonomicFilterToHogQl(g, v) - if (hogQl && isEventsQuery(query.source)) { - const isAggregation = isHogQlAggregation(hogQl) - const isOrderBy = query.source?.orderBy?.[0] === key - const isDescOrderBy = query.source?.orderBy?.[0] === `-${key}` - setQuery?.({ - ...query, - source: { - ...query.source, - select: query.source.select - .map((s, i) => (i === index ? hogQl : s)) - .filter((c) => (isAggregation ? c !== '*' : true)), - orderBy: - isOrderBy || isDescOrderBy - ? [isDescOrderBy ? `-${hogQl}` : hogQl] - : query.source?.orderBy, - }, - }) - } + { + setQuery?.({ + ...query, + source: { + ...query.source, + orderBy: [key], + } as EventsQuery, + }) }} - groupTypes={groupTypes} - buttonProps={{ type: undefined }} - /> - - {canSort ? ( - <> - { - setQuery?.({ - ...query, - source: { - ...query.source, - orderBy: [key], - } as EventsQuery, - }) - }} - > - Sort ascending - - { - setQuery?.({ - ...query, - source: { - ...query.source, - orderBy: [`-${key}`], - } as EventsQuery, - }) - }} - > - Sort descending - - - - ) : null} - Add column left} - data-attr="datatable-add-column-left" - onChange={(v, g) => { - const hogQl = taxonomicFilterToHogQl(g, v) - if (hogQl && isEventsQuery(query.source)) { - const isAggregation = isHogQlAggregation(hogQl) - setQuery?.({ - ...query, - source: { - ...query.source, - select: [ - ...(query.source.select || []).slice(0, index), - hogQl, - ...(query.source.select || []).slice(index), - ].filter((c) => (isAggregation ? c !== '*' : true)), - } as EventsQuery, - }) - } + > + Sort ascending + + { + setQuery?.({ + ...query, + source: { + ...query.source, + orderBy: [`-${key}`], + } as EventsQuery, + }) }} - groupTypes={groupTypes} - buttonProps={{ type: undefined }} - /> - Add column right} - data-attr="datatable-add-column-right" - onChange={(v, g) => { - const hogQl = taxonomicFilterToHogQl(g, v) - if (hogQl && isEventsQuery(query.source)) { - const isAggregation = isHogQlAggregation(hogQl) - setQuery?.({ - ...query, - source: { - ...query.source, - select: [ - ...(query.source.select || []).slice(0, index + 1), - hogQl, - ...(query.source.select || []).slice(index + 1), - ].filter((c) => (isAggregation ? c !== '*' : true)), - } as EventsQuery, - }) + > + Sort descending + + + + ) : null} + Add column left} + data-attr="datatable-add-column-left" + onChange={(v, g) => { + const hogQl = taxonomicFilterToHogQl(g, v) + if (hogQl && isEventsQuery(query.source)) { + const isAggregation = isHogQlAggregation(hogQl) + setQuery?.({ + ...query, + source: { + ...query.source, + select: [ + ...(query.source.select || []).slice(0, index), + hogQl, + ...(query.source.select || []).slice(index), + ].filter((c) => (isAggregation ? c !== '*' : true)), + } as EventsQuery, + }) + } + }} + groupTypes={groupTypes} + buttonProps={{ type: undefined }} + /> + Add column right} + data-attr="datatable-add-column-right" + onChange={(v, g) => { + const hogQl = taxonomicFilterToHogQl(g, v) + if (hogQl && isEventsQuery(query.source)) { + const isAggregation = isHogQlAggregation(hogQl) + setQuery?.({ + ...query, + source: { + ...query.source, + select: [ + ...(query.source.select || []).slice(0, index + 1), + hogQl, + ...(query.source.select || []).slice(index + 1), + ].filter((c) => (isAggregation ? c !== '*' : true)), + } as EventsQuery, + }) + } + }} + groupTypes={groupTypes} + buttonProps={{ type: undefined }} + /> + {columnsInQuery.filter((c) => c !== '*').length > 1 ? ( + <> + + { + const cleanColumnKey = removeExpressionComment(key) + const newSource: EventsQuery = { + ...(query.source as EventsQuery), + select: (query.source as EventsQuery).select.filter((_, i) => i !== index), + // remove the current column from orderBy if it's there + orderBy: (query.source as EventsQuery).orderBy?.find( + (orderKey) => + removeExpressionComment(orderKey) === cleanColumnKey || + removeExpressionComment(orderKey) === `-${cleanColumnKey}` + ) + ? undefined + : (query.source as EventsQuery).orderBy, } + setQuery?.({ + ...query, + source: newSource, + }) }} - groupTypes={groupTypes} - buttonProps={{ type: undefined }} - /> - {columnsInQuery.filter((c) => c !== '*').length > 1 ? ( - <> - - { - const cleanColumnKey = removeExpressionComment(key) - const newSource: EventsQuery = { - ...(query.source as EventsQuery), - select: (query.source as EventsQuery).select.filter( - (_, i) => i !== index - ), - // remove the current column from orderBy if it's there - orderBy: (query.source as EventsQuery).orderBy?.find( - (orderKey) => - removeExpressionComment(orderKey) === cleanColumnKey || - removeExpressionComment(orderKey) === `-${cleanColumnKey}` - ) - ? undefined - : (query.source as EventsQuery).orderBy, - } - setQuery?.({ - ...query, - source: newSource, - }) - }} - > - Remove column - - - ) : null} + > + Remove column + - ) : undefined, - } as LemonTableColumn | any[], any>) - ), + ) : null} + + ) : undefined, + })), ...(actionsColumnShown ? [ { dataIndex: '__more' as any, title: '', - render: function RenderMore(_: any, record: Record | any[]) { - if (categoryRowKey in record) { + render: function RenderMore(_: any, { label, result }: DataTableRow) { + if (label) { return { props: { colSpan: 0 } } } - if (isEventsQuery(query.source) && columnsInResponse?.includes('*')) { - return + if (result && isEventsQuery(query.source) && columnsInResponse?.includes('*')) { + return } return null }, @@ -289,8 +289,6 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele : []), ].filter((column) => !query.hiddenColumns?.includes(column.dataIndex) && column.dataIndex !== '*') - const dataSource = resultsWithLabelRows ?? [] - const setQuerySource = useCallback( (source: EventsNode | EventsQuery | PersonsNode) => setQuery?.({ ...query, source }), [setQuery] @@ -373,27 +371,25 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele '::' ) /* Bust the LemonTable cache when columns change */ } - dataSource={dataSource} - rowKey={(record, rowIndex) => { - if (categoryRowKey in record) { - return `__category_row__${rowIndex}` - } - if (isEventsQuery(query.source)) { - if (columnsInResponse?.includes('*')) { - return record[columnsInResponse.indexOf('*')].uuid - } else if (columnsInResponse?.includes('uuid')) { - return record[columnsInResponse.indexOf('uuid')] - } else if (columnsInResponse?.includes('id')) { - return record[columnsInResponse.indexOf('id')] + dataSource={(dataTableRows ?? []) as DataTableRow[]} + rowKey={({ result }: DataTableRow, rowIndex) => { + if (result) { + if (isEventsQuery(query.source)) { + if (columnsInResponse?.includes('*')) { + return result[columnsInResponse.indexOf('*')].uuid + } else if (columnsInResponse?.includes('uuid')) { + return result[columnsInResponse.indexOf('uuid')] + } else if (columnsInResponse?.includes('id')) { + return result[columnsInResponse.indexOf('id')] + } } - return JSON.stringify(record) - } else { return ( - ('uuid' in record ? (record as any).uuid : null) ?? - record.id ?? - JSON.stringify(record) + (result && 'uuid' in result ? (result as any).uuid : null) ?? + (result && 'id' in result ? (result as any).id : null) ?? + JSON.stringify(result ?? rowIndex) ) } + return rowIndex }} sorting={null} useURLForSorting={false} @@ -401,26 +397,28 @@ export function DataTable({ query, setQuery, context }: DataTableProps): JSX.Ele expandable={ expandable && isEventsQuery(query.source) && columnsInResponse?.includes('*') ? { - expandedRowRender: function renderExpand(record) { - if (isEventsQuery(query.source) && Array.isArray(record)) { + expandedRowRender: function renderExpand({ result }) { + if (isEventsQuery(query.source) && Array.isArray(result)) { return ( ) } - return record ? : null + if (result && !Array.isArray(result)) { + return + } }, - rowExpandable: (record) => !(categoryRowKey in record), + rowExpandable: ({ result }) => !!result, noIndent: true, } : undefined } - rowClassName={(row) => + rowClassName={({ result, label }) => clsx('DataTable__row', { - 'DataTable__row--highlight_once': row && isRowHighlighted(row), - 'DataTable__row--category_row': row && categoryRowKey in row, + 'DataTable__row--highlight_once': result && highlightedRows.has(result), + 'DataTable__row--category_row': !!label, }) } /> diff --git a/frontend/src/queries/nodes/DataTable/dataTableLogic.test.ts b/frontend/src/queries/nodes/DataTable/dataTableLogic.test.ts index 3d2c080b5027f..efa5ea2df0217 100644 --- a/frontend/src/queries/nodes/DataTable/dataTableLogic.test.ts +++ b/frontend/src/queries/nodes/DataTable/dataTableLogic.test.ts @@ -1,7 +1,7 @@ import { initKeaTests } from '~/test/init' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { expectLogic, partial } from 'kea-test-utils' -import { categoryRowKey, dataTableLogic } from '~/queries/nodes/DataTable/dataTableLogic' +import { dataTableLogic } from '~/queries/nodes/DataTable/dataTableLogic' import { dataNodeLogic } from '~/queries/nodes/DataNode/dataNodeLogic' import { DataTableNode, NodeKind } from '~/queries/schema' import { query } from '~/queries/query' @@ -209,18 +209,14 @@ describe('dataTableLogic', () => { .delay(0) .toMatchValues({ responseLoading: false, response: partial({ results }) }) await expectLogic(logic).toMatchValues({ - resultsWithLabelRows: [ - results[0], - { - [categoryRowKey]: 'December 23, 2022', - }, - results[1], - results[2], - { - [categoryRowKey]: 'December 22, 2022', - }, - results[3], - results[4], + dataTableRows: [ + { result: results[0] }, + { label: 'December 23, 2022' }, + { result: results[1] }, + { result: results[2] }, + { label: 'December 22, 2022' }, + { result: results[3] }, + { result: results[4] }, ], }) }) diff --git a/frontend/src/queries/nodes/DataTable/dataTableLogic.ts b/frontend/src/queries/nodes/DataTable/dataTableLogic.ts index affefacf0c50c..bd01b12fc6f15 100644 --- a/frontend/src/queries/nodes/DataTable/dataTableLogic.ts +++ b/frontend/src/queries/nodes/DataTable/dataTableLogic.ts @@ -15,7 +15,13 @@ export interface DataTableLogicProps { query: DataTableNode } -export const categoryRowKey = Symbol('__categoryRow__') +export interface DataTableRow { + /** Display a row with a label. */ + label?: JSX.Element | string | null + /** Display a row with results */ + result?: Record | any[] +} + export const loadingColumn = Symbol('...') export const errorColumn = Symbol('Error!') @@ -40,7 +46,7 @@ export const dataTableLogic = kea([ featureFlagLogic, ['featureFlags'], dataNodeLogic({ key: props.key, query: props.query.source }), - ['response', 'responseLoading', 'responseError', 'highlightedRows'], + ['response', 'responseLoading', 'responseError'], ], })), selectors({ @@ -60,9 +66,15 @@ export const dataTableLogic = kea([ ? (response?.columns as string[]) : null, ], - resultsWithLabelRows: [ + dataTableRows: [ (s) => [s.sourceKind, s.orderBy, s.response, s.columnsInQuery, s.responseError], - (sourceKind, orderBy, response: AnyDataNode['response'], columnsInQuery, responseError): any[] | null => { + ( + sourceKind, + orderBy, + response: AnyDataNode['response'], + columnsInQuery, + responseError + ): DataTableRow[] | null => { if (response && sourceKind === NodeKind.EventsQuery) { const eventsQueryResponse = response as EventsQuery['response'] | null if (eventsQueryResponse) { @@ -86,9 +98,9 @@ export const dataTableLogic = kea([ } const columnMap = Object.fromEntries(columnsInResponse.map((c, i) => [c, i])) - const convertResultToDisplayedColumns = equal(columnsInQuery, columnsInResponse) - ? (result: any[]) => result - : (result: any[]): any[] => { + const resultToDataTableRow = equal(columnsInQuery, columnsInResponse) + ? (result: any[]): DataTableRow => ({ result }) + : (result: any[]): DataTableRow => { const newResult = columnsInQuery.map((c) => c in columnMap ? result[columnMap[c]] @@ -96,14 +108,13 @@ export const dataTableLogic = kea([ ? errorColumn : loadingColumn ) - ;(newResult as any).__originalResultRow = result - return newResult + return { result: newResult } } // Add a label between results if the day changed if (orderKey === 'timestamp' && orderKeyIndex !== -1) { let lastResult: any | null = null - const newResults: any[] = [] + const newResults: DataTableRow[] = [] for (const result of results) { if ( result && @@ -111,29 +122,23 @@ export const dataTableLogic = kea([ !dayjs(result[orderKeyIndex]).isSame(lastResult[orderKeyIndex], 'day') ) { newResults.push({ - [categoryRowKey]: dayjs(result[orderKeyIndex]).format('LL'), + label: dayjs(result[orderKeyIndex]).format('LL'), }) } - newResults.push(convertResultToDisplayedColumns(result)) + newResults.push(resultToDataTableRow(result)) lastResult = result } return newResults } else { - return results.map((result) => convertResultToDisplayedColumns(result)) + return results.map((result) => resultToDataTableRow(result)) } } } - return response && 'results' in response ? (response as any).results ?? null : null + return response && 'results' in response && Array.isArray(response.results) + ? response.results.map((result: any) => ({ result })) ?? null + : null }, ], - isRowHighlighted: [ - (s) => [s.highlightedRows], - (highlightedRows) => - (row: any[]): boolean => - row - ? highlightedRows.has('__originalResultRow' in row ? (row as any).__originalResultRow : row) - : false, - ], queryWithDefaults: [ (s, p) => [p.query, s.columnsInQuery, s.featureFlags], (query: DataTableNode, columnsInQuery, featureFlags): Required => { diff --git a/frontend/src/queries/nodes/DataTable/renderColumn.tsx b/frontend/src/queries/nodes/DataTable/renderColumn.tsx index 13b2d74396c58..d2fb401478a45 100644 --- a/frontend/src/queries/nodes/DataTable/renderColumn.tsx +++ b/frontend/src/queries/nodes/DataTable/renderColumn.tsx @@ -14,7 +14,7 @@ import { DeletePersonButton } from '~/queries/nodes/PersonsNode/DeletePersonButt import ReactJson from 'react-json-view' import { errorColumn, loadingColumn } from '~/queries/nodes/DataTable/dataTableLogic' import { Spinner } from 'lib/components/Spinner/Spinner' -import { Tag } from 'antd' +import { LemonTag } from 'lib/components/LemonTag/LemonTag' export function renderColumn( key: string, @@ -27,7 +27,7 @@ export function renderColumn( if (value === loadingColumn) { return } else if (value === errorColumn) { - return Error + return Error } else if (key === 'event' && isEventsQuery(query.source)) { const resultRow = record as any[] const eventRecord = query.source.select.includes('*') ? resultRow[query.source.select.indexOf('*')] : null From 4f1843c46d6a491ac293eb91c2d39e7bd637b428 Mon Sep 17 00:00:00 2001 From: Marius Andra Date: Fri, 20 Jan 2023 10:21:14 +0100 Subject: [PATCH 19/19] just clear results on error --- .../queries/nodes/DataNode/dataNodeLogic.ts | 4 +++ .../queries/nodes/DataTable/dataTableLogic.ts | 36 ++++--------------- 2 files changed, 11 insertions(+), 29 deletions(-) diff --git a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts index 6a5b13d58607b..8a46f22a3ea4e 100644 --- a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts +++ b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts @@ -162,6 +162,10 @@ export const dataNodeLogic = kea([ loadNextData: () => performance.now(), }, ], + response: { + // Clear the response if a failure to avoid showing inconsistencies in the UI + loadDataFailure: () => null, + }, responseError: [ null as string | null, { diff --git a/frontend/src/queries/nodes/DataTable/dataTableLogic.ts b/frontend/src/queries/nodes/DataTable/dataTableLogic.ts index bd01b12fc6f15..a6e00222dbb57 100644 --- a/frontend/src/queries/nodes/DataTable/dataTableLogic.ts +++ b/frontend/src/queries/nodes/DataTable/dataTableLogic.ts @@ -67,14 +67,8 @@ export const dataTableLogic = kea([ : null, ], dataTableRows: [ - (s) => [s.sourceKind, s.orderBy, s.response, s.columnsInQuery, s.responseError], - ( - sourceKind, - orderBy, - response: AnyDataNode['response'], - columnsInQuery, - responseError - ): DataTableRow[] | null => { + (s) => [s.sourceKind, s.orderBy, s.response, s.columnsInQuery], + (sourceKind, orderBy, response: AnyDataNode['response'], columnsInQuery): DataTableRow[] | null => { if (response && sourceKind === NodeKind.EventsQuery) { const eventsQueryResponse = response as EventsQuery['response'] | null if (eventsQueryResponse) { @@ -86,30 +80,14 @@ export const dataTableLogic = kea([ removeExpressionComment(column) === `-${orderKey}` ) - // if we errored by adding a new column, show the new columns with the old results - // if the columns changed in any other way, return no results - if ( - responseError && - columnsInResponse && - (columnsInQuery.length <= columnsInResponse.length || - columnsInResponse.find((c) => !columnsInQuery.includes(c))) - ) { - return [] - } - const columnMap = Object.fromEntries(columnsInResponse.map((c, i) => [c, i])) const resultToDataTableRow = equal(columnsInQuery, columnsInResponse) ? (result: any[]): DataTableRow => ({ result }) - : (result: any[]): DataTableRow => { - const newResult = columnsInQuery.map((c) => - c in columnMap - ? result[columnMap[c]] - : responseError - ? errorColumn - : loadingColumn - ) - return { result: newResult } - } + : (result: any[]): DataTableRow => ({ + result: columnsInQuery.map((c) => + c in columnMap ? result[columnMap[c]] : loadingColumn + ), + }) // Add a label between results if the day changed if (orderKey === 'timestamp' && orderKeyIndex !== -1) {