Skip to content

chore(django): move eval code off orm#66220

Open
z0br0wn wants to merge 2 commits into
masterfrom
zbrown/personhog-demo-eval-off-orm
Open

chore(django): move eval code off orm#66220
z0br0wn wants to merge 2 commits into
masterfrom
zbrown/personhog-demo-eval-off-orm

Conversation

@z0br0wn

@z0br0wn z0br0wn commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Problem

demo eval call sites still hit the orm

Changes

  • moved demo eval call sites to direct sql

@z0br0wn z0br0wn requested a review from a team June 25, 2026 22:31
@assign-reviewers-posthog assign-reviewers-posthog Bot requested a review from a team June 25, 2026 22:31
@@ -20,6 +20,7 @@

from posthog.hogql_queries.query_runner import get_query_runner
from posthog.models import GroupTypeMapping, Organization, PropertyDefinition, Team

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can GroupTypeMapping be removed from imports here?

def test_restores_models_counts(self):
_org, _user, _dataset, team = self._load_with_mocks()
self.assertEqual(PropertyDefinition.objects.filter(team_id=team.id).count(), 2)
self.assertEqual(GroupTypeMapping.objects.filter(team_id=team.id).count(), 2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, there is still a call here

@greptile-apps

greptile-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Reviews (1): Last reviewed commit: "chore(django): move eval code off orm" | Re-trigger Greptile

Comment on lines +55 to +56
with persons_db_connection(writer=True, autocommit=True) as conn, conn.cursor() as cursor:
cursor.execute("DELETE FROM posthog_grouptypemapping")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unscoped DELETE may wipe unrelated rows

The DELETE FROM posthog_grouptypemapping clears every row in the table, not just those belonging to the teams created by these tests. If the persons DB is ever shared — e.g., another test class also writes through persons_db_connection, or an eval integration test leaves data behind — all of it is silently deleted here. Scoping to the known team_ids (WHERE team_id IN (9990, 99990, ...)) or moving the cleanup to tearDown and filtering by the team.id created during the test would limit the blast radius to what this class actually wrote.

@tests-posthog

tests-posthog Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Query snapshots: Backend query snapshots updated

Changes: 1 snapshots (1 modified, 0 added, 0 deleted)

What this means:

  • Query snapshots have been automatically updated to match current output
  • These changes reflect modifications to database queries or schema

Next steps:

  • Review the query changes to ensure they're intentional
  • If unexpected, investigate what caused the query to change

Review snapshot changes →

@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🎭 Playwright report · View test results →

⚠️ 1 flaky test:

  • Creating a SQL insight with a variable and overriding it on a dashboard (chromium)

These issues are not necessarily caused by your changes.
Annoyed by this comment? Help fix flakies and failures and it'll disappear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants