Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions posthog/api/test/test_person.py
Original file line number Diff line number Diff line change
Expand Up @@ -1897,8 +1897,9 @@ def test_pagination_limit(self):
returned_ids = []
# The property-access-control feature check reuses the request's already-loaded team,
# so listing persons no longer pays a per-request Team lookup (was 16). +1 for the
# saved-expressions fetch in the HogQL database build.
with self.assertNumQueries(16):
# saved-expressions fetch in the HogQL database build. +1 for the shared-database
# kill-switch instance setting, cold-cache here but TTL-cached per worker in production.
with self.assertNumQueries(17):
response = self.client.get("/api/person/?limit=10").json()
self.assertEqual(len(response["results"]), 9)
returned_ids += [x["distinct_ids"][0] for x in response["results"]]
Expand Down
4 changes: 3 additions & 1 deletion posthog/hogql/database/database.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@
from posthog.hogql.database.schema.web_vitals_paths_preaggregated import WebVitalsPathsPreaggregatedTable
from posthog.hogql.database.utils import get_join_field_chain, qualify_join_key_expr
from posthog.hogql.database.warehouse_join_resolvers import data_warehouse_resolver_params
from posthog.hogql.editor_assist_metrics import HOGQL_DATABASE_BUILD_DURATION_SECONDS
from posthog.hogql.editor_assist_metrics import HOGQL_DATABASE_BUILD_DURATION_SECONDS, HOGQL_DATABASE_BUILD_TOTAL
from posthog.hogql.errors import QueryError, ResolutionError, TableAccessDeniedError
from posthog.hogql.modifiers import create_default_modifiers_for_team
from posthog.hogql.parser import parse_expr
Expand Down Expand Up @@ -1371,10 +1371,12 @@ def create_for(
connection_id: str | None = None,
bypass_warehouse_access_control: bool = False,
build_postgres_foreign_keys: bool = True,
trigger: str = "direct",
) -> Database:
if timings is None:
timings = HogQLTimings()

HOGQL_DATABASE_BUILD_TOTAL.labels(trigger=trigger).inc()
with HOGQL_DATABASE_BUILD_DURATION_SECONDS.labels(phase="fetch_sources").time():
sources = Database._fetch_sources(
team_id,
Expand Down
9 changes: 8 additions & 1 deletion posthog/hogql/editor_assist_metrics.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from prometheus_client import Histogram
from prometheus_client import Counter, Histogram

EDITOR_ASSIST_DURATION_SECONDS = Histogram(
"hogql_editor_assist_duration_seconds",
Expand All @@ -13,3 +13,10 @@
labelnames=["phase"],
buckets=(0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1.0, 2.5),
)

HOGQL_DATABASE_BUILD_TOTAL = Counter(
"hogql_database_build",
"HogQL database builds by the call path that requested them. Redundant builds show up as "
"executor/printer builds that a shared build should have covered.",
labelnames=["trigger"],
)
1 change: 1 addition & 0 deletions posthog/hogql/printer/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ def prepare_ast_for_printing(
user=context.user,
timings=context.timings,
bypass_warehouse_access_control=context.bypass_warehouse_access_control,
trigger="printer",
)
if context.direct_postgres_connection_metadata is None and context.database is not None:
context.direct_postgres_connection_metadata = getattr(context.database, "_direct_connection_metadata", None)
Expand Down
2 changes: 2 additions & 0 deletions posthog/hogql/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ def _process_placeholders(self):
modifiers=self.query_modifiers,
timings=self.timings,
bypass_warehouse_access_control=self.context.bypass_warehouse_access_control,
trigger="executor",
)
self.select_query = replace_filters(
self.select_query, self.filters, self.team, database=self.context.database
Expand Down Expand Up @@ -255,6 +256,7 @@ def _generate_hogql(self):
timings=self.timings,
connection_id=self.connection_id,
bypass_warehouse_access_control=self.context.bypass_warehouse_access_control,
trigger="executor",
)

# Reset between executions: the resolver/printer append per query, and dataclasses.replace
Expand Down
11 changes: 8 additions & 3 deletions posthog/hogql_queries/actors_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

from posthog.hogql import ast
from posthog.hogql.constants import HogQLQuerySettings, LimitContext
from posthog.hogql.context import HogQLContext
from posthog.hogql.parser import parse_expr, parse_order_expr
from posthog.hogql.printer import prepare_and_print_ast
from posthog.hogql.property import has_aggregation
Expand Down Expand Up @@ -78,7 +77,12 @@ def run(
cache_age_seconds: Optional[int] = None,
analytics_props: Optional["AnalyticsProps"] = None,
):
self.user = user
# This runner takes the run() user verbatim, including None. A bare assignment would
# hide the change from the base run(), leaving the shared database and access-control
# snapshot scoped to the previous user.
if user is not self.user:
self.user = user
self._on_user_changed()
return super().run(
execution_mode,
user,
Expand Down Expand Up @@ -202,6 +206,7 @@ def _calculate_internal(self) -> ActorsQueryResponse:
user=self.user,
timings=self.timings,
modifiers=self.modifiers,
context=self.build_hogql_context(),
)
input_columns = self.input_columns()
missing_actors_count = None
Expand Down Expand Up @@ -501,7 +506,7 @@ def to_query(self) -> ast.SelectQuery | ast.SelectSetQuery:
try:
prepare_and_print_ast(
select_query,
context=HogQLContext(
context=self.build_hogql_context(
team=self.team,
enable_select_queries=True,
timings=self.timings,
Expand Down
2 changes: 2 additions & 0 deletions posthog/hogql_queries/events_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,7 @@ def _calculate(self) -> EventsQueryResponse:
modifiers=self.modifiers,
limit_context=self.limit_context,
user=self.user,
context=self.build_hogql_context(),
)

# Convert star field from tuple to dict in each result
Expand Down Expand Up @@ -696,6 +697,7 @@ def batch_check_session_recordings(self) -> set[str]:
query_type="EventsQuerySessionRecordingsCheck",
timings=self.timings,
modifiers=self.modifiers,
context=self.build_hogql_context(),
)

# Return set of session IDs that exist
Expand Down
3 changes: 1 addition & 2 deletions posthog/hogql_queries/groups/groups_query_runner.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from posthog.schema import CachedGroupsQueryResponse, GroupsQuery, GroupsQueryResponse

from posthog.hogql import ast
from posthog.hogql.context import HogQLContext
from posthog.hogql.parser import parse_expr, parse_order_expr
from posthog.hogql.property import has_aggregation, property_to_expr

Expand Down Expand Up @@ -152,7 +151,7 @@ def _calculate(self) -> GroupsQueryResponse:
timings=self.timings,
modifiers=self.modifiers,
# :HACKY: posthog/hogql/transforms/property_types.py needs access to the group_id in order to know the property type
context=HogQLContext(team_id=self.team.pk, globals={"group_id": self.query.group_type_index}),
context=self.build_hogql_context(globals={"group_id": self.query.group_type_index}),
)
results = response.results[: self.paginator.limit] if self.paginator.limit is not None else response.results

Expand Down
21 changes: 19 additions & 2 deletions posthog/hogql_queries/hogql_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ def get_cache_payload(self) -> dict:
payload = super().get_cache_payload()
if self._managed_warehouse_sql_mode == ManagedWarehouseSQLMode.BUILT_IN:
payload["managed_warehouse_sql_mode"] = self._managed_warehouse_sql_mode.value
if self._modifiers_override_provided and self.query.modifiers is not None:
# Old workers preferred query modifiers while new workers prefer the constructor override.
# Keep their cached results apart during a rolling deploy.
payload["hogql_modifier_precedence"] = "runner"
return payload

def query_status_labels(self) -> list[str] | None:
Expand Down Expand Up @@ -144,7 +148,11 @@ def to_query(self) -> ast.SelectQuery | ast.SelectSetQuery:
finder = find_placeholders(parsed_select)
with self.timings.measure("filters"):
if self.query.filters and finder.has_filters:
parsed_select = replace_filters(parsed_select, self.query.filters, self.team)
# Resolve {filters} against the shared database so a filtered query builds the schema
# once, instead of replace_filters building a throwaway one. With a connection id the
# schema is the external connection's, so keep the per-call build there.
database = self.shared_database if self.query.connectionId is None else None
parsed_select = replace_filters(parsed_select, self.query.filters, self.team, database)
if self.query.variables:
with self.timings.measure("replace_variables"):
parsed_select = replace_variables(parsed_select, list(self.query.variables.values()), self.team)
Expand Down Expand Up @@ -209,11 +217,19 @@ def _calculate(self) -> HogQLQueryResponse:
execute_hogql_query if paginator is None else paginator.execute_hogql_query,
)

context_kwargs: dict[str, Any] = {}
if self.query.connectionId is None:
# With a connection id the executor builds its own connection-scoped database,
# so the shared one would be built for nothing.
context_kwargs["context"] = self.build_hogql_context()
Comment thread
georgemunyoro marked this conversation as resolved.
Resolved
Comment thread
posthog[bot] marked this conversation as resolved.
Resolved
Comment thread
posthog[bot] marked this conversation as resolved.
Resolved
response = func(
query_type="HogQLQuery",
query=query,
filters=self.query.filters,
modifiers=self.query.modifiers or self.modifiers,
# Print with the same modifiers the shared database was built from (self.modifiers).
# The shared database uses self.modifiers, so passing self.query.modifiers here would let
# a caller that sets both build the schema from one modifier set and print SQL for another.
modifiers=self.modifiers,
team=self.team,
user=self.user,
user_access_control=self.user_access_control,
Expand All @@ -224,6 +240,7 @@ def _calculate(self) -> HogQLQueryResponse:
workload=self.workload,
ch_user=self.ch_user,
settings=self.settings,
**context_kwargs,
)
if paginator:
response = response.model_copy(update={**paginator.response_params(), "results": paginator.results})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,4 +211,5 @@ def _calculate(self) -> HogQLQueryResponse:
timings=self.timings,
modifiers=self.modifiers,
limit_context=self.limit_context,
context=self.build_hogql_context(),
)
9 changes: 2 additions & 7 deletions posthog/hogql_queries/property_values_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
)

from posthog.hogql import ast
from posthog.hogql.context import HogQLContext
from posthog.hogql.parser import parse_expr, parse_select
from posthog.hogql.query import execute_hogql_query

Expand Down Expand Up @@ -74,11 +73,7 @@ def _calculate_event(self) -> PropertyValuesQueryResponse:
self._event_query(),
team=self.team,
user=self.user,
context=HogQLContext(
team_id=self.team.pk,
user=self.user,
use_new_events_schema=self._use_new_events_schema,
),
context=self.build_hogql_context(use_new_events_schema=self._use_new_events_schema),
timings=self.timings,
modifiers=self.modifiers,
limit_context=self.limit_context,
Expand Down Expand Up @@ -163,7 +158,7 @@ def _calculate_person(self) -> PropertyValuesQueryResponse:
query_type=query_type,
team=self.team,
user=self.user,
context=HogQLContext(team_id=self.team.pk, user=self.user),
context=self.build_hogql_context(),
timings=self.timings,
modifiers=self.modifiers,
limit_context=self.limit_context,
Expand Down
26 changes: 24 additions & 2 deletions posthog/hogql_queries/query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -1682,6 +1682,7 @@ def __init__(
self.query_id = query_id
self.workload = workload
self.ch_user = ch_user
self._modifiers_override_provided = modifiers is not None

if not self.is_query_node(query):
if isinstance(self.query_type, UnionType):
Expand Down Expand Up @@ -1717,6 +1718,13 @@ def _on_user_changed(self) -> None:
a cached HogQLContext / Database) must call super()._on_user_changed()."""
self._shared_database = None

@property
def user_access_control(self) -> Optional[UserAccessControl]:
"""Access-control snapshot the shared database is built with. None here; overridden by
AnalyticsQueryRunner with a lazily built, per-run instance so the cache fingerprint and
the database resolve access from the same rows."""
return None

@property
def shared_database(self) -> Database:
"""One Database for every query this runner executes and for the response SQL printer.
Expand All @@ -1728,7 +1736,13 @@ def shared_database(self) -> Database:
# Kill switch: build per access so query threads never share schema state. No timings
# measure here because concurrent threads reach this path and HogQLTimings is not
# thread-safe.
return Database.create_for(team=self.team, user=self.user, modifiers=self.modifiers)
return Database.create_for(
team=self.team,
user=self.user,
user_access_control=self.user_access_control,
modifiers=self.modifiers,
trigger="shared_kill_switch",
)
if self._shared_database is None:
# Concurrent query threads (funnels compare mode) can first-touch this property at the
# same time. The lock makes the build run once, and keeps the measure on the single
Expand All @@ -1739,8 +1753,10 @@ def shared_database(self) -> Database:
self._shared_database = Database.create_for(
team=self.team,
user=self.user,
user_access_control=self.user_access_control,
modifiers=self.modifiers,
timings=self.timings,
trigger="shared",
)
return self._shared_database

Expand Down Expand Up @@ -2961,6 +2977,12 @@ def calculate(self) -> AR:

def _on_user_changed(self) -> None:
super()._on_user_changed()
if (
self._user_access_control is not None
and isinstance(self.user, User)
and self._user_access_control.user.pk == self.user.pk
):
return
self._user_access_control = None

@property
Expand Down Expand Up @@ -3090,7 +3112,7 @@ def __init__(self, *args, **kwargs):
self._build_hogql_context_for_user(self.user)

def _build_hogql_context_for_user(self, user: Optional[User]) -> None:
self.database = Database.create_for(team=self.team, user=user)
self.database = Database.create_for(team=self.team, user=user, trigger="runner_context")
self.hogql_context = HogQLContext(team_id=self.team.pk, database=self.database, user=user)

def _on_user_changed(self) -> None:
Expand Down
1 change: 1 addition & 0 deletions posthog/hogql_queries/sessions_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,7 @@ def _calculate(self) -> SessionsQueryResponse:
timings=self.timings,
modifiers=self.modifiers,
limit_context=self.limit_context,
context=self.build_hogql_context(),
)

# Convert star field from tuple to dict in each result
Expand Down
7 changes: 1 addition & 6 deletions posthog/hogql_queries/sessions_timeline_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
)

from posthog.hogql import ast
from posthog.hogql.context import HogQLContext
from posthog.hogql.parser import parse_select
from posthog.hogql.query import execute_hogql_query

Expand Down Expand Up @@ -205,11 +204,7 @@ def _calculate(self) -> SessionsTimelineQueryResponse:
query=self.to_query(),
team=self.team,
user=self.user,
context=HogQLContext(
team_id=self.team.pk,
user=self.user,
use_new_events_schema=self._use_new_events_schema,
),
context=self.build_hogql_context(use_new_events_schema=self._use_new_events_schema),
query_type="SessionsTimelineQuery",
timings=self.timings,
modifiers=self.modifiers,
Expand Down
36 changes: 36 additions & 0 deletions posthog/hogql_queries/test/test_hogql_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,16 @@
HogQLFilters,
HogQLPropertyFilter,
HogQLQuery,
HogQLQueryModifiers,
HogQLQueryResponse,
HogQLVariable,
QueryStatus,
SessionTableVersion,
)

from posthog.hogql import ast
from posthog.hogql.errors import ExposedHogQLError, QueryError
from posthog.hogql.query import execute_hogql_query
from posthog.hogql.user_query_validator import HOGQL_PERSONAL_API_KEY_OFFSET_ALLOWED_FLAG, OFFSET_NOT_ALLOWED_MESSAGE
from posthog.hogql.visitor import clear_locations

Expand Down Expand Up @@ -65,6 +68,39 @@ def setUp(self):
super().setUp()
self.random_uuid = self._create_random_persons()

def test_calculate_with_query_modifiers_matches_unthreaded_executor_sql(self):
# The runner hands the executor a context wired to its shared database, which is
# built from the runner's resolved modifiers. Query-level modifiers reshape that
# database (e.g. which sessions table backs `sessions`), so the printed SQL must
# match what the executor produces when it builds the database itself.
query = "select count() from sessions limit 1"
modifiers = HogQLQueryModifiers(sessionTableVersion=SessionTableVersion.V1)
runner = self._create_runner(HogQLQuery(query=query, modifiers=modifiers))
threaded = runner.calculate()
baseline = execute_hogql_query(query=query, team=self.team, modifiers=modifiers)
assert threaded.clickhouse == baseline.clickhouse

def test_calculate_reports_modifiers_matching_its_shared_database(self):
# The shared database is built from the runner's resolved modifiers, which prefer the
# constructor modifiers over the query's own. The executor must run and report those same
# modifiers, or a caller that sets both gets SQL built for one schema and a response that
# describes another. sessionTableVersion picks which table backs `sessions`.
query = "select count() from sessions limit 1"
runner = HogQLQueryRunner(
team=self.team,
query=HogQLQuery(query=query, modifiers=HogQLQueryModifiers(sessionTableVersion=SessionTableVersion.V2)),
modifiers=HogQLQueryModifiers(sessionTableVersion=SessionTableVersion.V1),
)
response = runner.calculate()
baseline = execute_hogql_query(
query=query, team=self.team, modifiers=HogQLQueryModifiers(sessionTableVersion=SessionTableVersion.V1)
)
# SQL is built from the shared database's modifiers (V1), and the reported modifiers agree.
assert response.clickhouse == baseline.clickhouse
assert response.modifiers is not None
assert response.modifiers.sessionTableVersion == SessionTableVersion.V1
assert runner.get_cache_payload()["hogql_modifier_precedence"] == "runner"

def test_default_hogql_query(self):
runner = self._create_runner(HogQLQuery(query="select count(event) from events"))
query = runner.to_query()
Expand Down
Loading
Loading