feat(data-exploration): data exploration backend API with types #13933

mariusandra · 2023-01-26T09:44:09Z

Problem

People are already using our API to get data exploration event responses, even though that backend part is undocumented and not finished.

Changes

Disconnects the "events v2" endpoint from the old /api/project/:id/events endpoint. The old endpoint works like it did before. Eventually we can deprecate it.
Creates a new /api/project/:id/query endpoint, which accepts any node that matches QuerySchema (so any query).
Using pydantic, generate Python classes from the JSON schema (which is generated from TypeScript definitions)
Use those new classes in the query endpoint, and in the v2 events query.
~~Replace the type HogQLExpression with just string, as it was causing trouble in the backend.~~
Fix a bug where if you removed the column that was used for sorting, the query would fail if there were aggregations, as it would keep the old sort column.
Move some test_event.py tests into test_query.py without changing the content
I kept the old and new events query with the same old query_event_list.py file. I'd like to still refactor it substantially, but the point now was to do the smallest change on the query itself.

How did you test this code?

Tested in the interface locally, and moved some tests around to the new format.

…eries

…kend-queries

frontend/src/queries/schema.ts

…n, remove timestamp in old events query

…kend-queries

mariusandra · 2023-01-26T18:11:22Z

Ready for a better look.

Things I already know to do... likely in a followup:

Fix the kludge with returning json
CI script that breaks the build if running schema:build results in changed files

Twixes · 2023-01-27T10:08:14Z

frontend/src/queries/schema.ts

+ * This file acts as the source of truth for:
+ *
+ * - frontend/src/queries/schema.json
+ *   - generated from typescript via "pnpm run generate:schema:json"


If schema.json and schema.py are only generated from this file, do we need to commit them? I can see this being a bit easier, but I'm a bit worried about them getting of out of sync as it's possible to commit changes to the wrong file. Plus they're really big. Could be compiled in package.json's prepare

Says the guy who just committed gigabytes of PNGs to the repo :D.

Just like snapshots, I'm afraid if we don't commit them to git, we won't see any unexpected changes in the schema. Since this is a very important schema that'll be used to generate public facing docs, tooling for users, etc, I'd prefer to not take any chances.

It also makes development easier, if the files are already there. Unrelated, I've also been playing around with a Kea typegen inline types mode that would also commit the types into the repo... for clean, fast and reproducible builds.

Also, I still want to add a github action that throws a wrench when the schema is out of sync.

Having the kea types ship with git would help a lot for reviews etc. where switching branches takes a bit of time at the moment, so +1 for a solution there.

Isn't schema.ts the snapshot in this case? The derived .json and .py versions ones are just distractions in a code review.
But fair enough, should be okay IF we have a wrench-throwing action.

posthog/api/event.py

posthog/api/query.py

Twixes · 2023-01-27T10:17:51Z

posthog/models/event/query_event_list.py

    offset: int = 0,
-) -> Union[List, EventsQueryResponse]:
+) -> List:


A List of what?

hmm... this is how it was 😅

posthog/models/event/query_event_list.py

Twixes · 2023-01-27T10:29:16Z

posthog/models/event/query_event_list.py

@@ -168,7 +216,10 @@ def query_events_list(
                fragment = fragment[1:]
            order_by_list.append(translate_hogql(fragment, hogql_context) + " " + order_direction)
    else:
-        order_by_list.append(select_columns[0] + " ASC")
+        if "count(*)" in select_columns:


Is count() a valid option too?

Not right now, as this is the generated output SQL where count() hogql -> count(*) clickhouse sql... but since count() is also valid clickhouse SQL, and we might accidentally change this down the line, I added both formats here just to future proof.

Twixes · 2023-01-27T10:31:07Z

posthog/models/event/query_event_list.py

+        )
+
+
+def query_events_list_v2(


Maybe something like run_data_table_query? This is more than an "events list v2"

Changed it to run_events_query, as a Team and an EventsQuery are its only inputs. I'd move this to a new file as well if I'd make all the changes I can foresee...

thmsobrmlr

Awesome work! Am I right that we'd write types for the backend response in schema.ts as well and thus have full type checking for the frontend <-> backend interaction? (will ask this at standup :))

Leaving the backend review to more knowledgeable people :)

frontend/src/queries/nodes/DataTable/ColumnConfigurator/ColumnConfigurator.tsx

…eries

Twixes

✅ but let's make sure the schemas don't diverge with some PR check.

mariusandra · 2023-01-27T14:30:24Z

Thanks! Going to merge this in to reduce the number of spinning plates... and add one more with that CI check.

* feat(data-exploration): backend json schema validation * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * refactor * switch to pydantic * split into v2 events list query * add backend query endpoint * Update snapshots * make events query work * bring back HogQLExpression, inline all __root__ only classes in python, remove timestamp in old events query * Update snapshots * Update snapshots * schema and mypy * Update snapshots * Update snapshots * restore /fake/ orderBy (only "timestamp" and "-timestamp" are supported) * Update snapshots * Update snapshots * Update snapshots * raise if invalid json * Update snapshots * remove __root__ hack * improve comments, in and out of quotes * shrug * future proofing * comment * rename to "run_events_query" * simplify Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

mariusandra and others added 17 commits January 26, 2023 10:36

feat(data-exploration): backend json schema validation

a896737

Update snapshots

1b55747

Update snapshots

2c68681

Update snapshots

4b5a595

Update snapshots

3e3b1ec

Update snapshots

3bf4a30

Update snapshots

1e91573

Update snapshots

5007caf

Update snapshots

df9fd12

refactor

dd72cbe

switch to pydantic

12ec5ef

Merge remote-tracking branch 'origin/backend-queries' into backend-qu…

5546ca4

…eries

split into v2 events list query

dec00a0

add backend query endpoint

1863738

Update snapshots

350ca11

make events query work

334b3d7

Merge branch 'backend-queries' of github.com:PostHog/posthog into bac…

d567a7c

…kend-queries

mariusandra changed the title ~~feat(data-exploration): backend json schema validation~~ feat(data-exploration): data exploration backend API with types Jan 26, 2023

thmsobrmlr reviewed Jan 26, 2023

View reviewed changes

frontend/src/queries/schema.ts Show resolved Hide resolved

mariusandra and others added 3 commits January 26, 2023 16:09

bring back HogQLExpression, inline all __root__ only classes in pytho…

f941eff

…n, remove timestamp in old events query

Update snapshots

7c40322

Update snapshots

6e08e9e

mariusandra mentioned this pull request Jan 26, 2023

chore(frontend): remove duplicate PropertyFilter class #13939

Merged

mariusandra and others added 7 commits January 26, 2023 18:22

Merge branch 'master' into backend-queries

d02e4b7

schema and mypy

8a223b2

Update snapshots

62f3d46

Update snapshots

efe5135

restore /fake/ orderBy (only "timestamp" and "-timestamp" are supported)

9701325

Merge branch 'backend-queries' of github.com:PostHog/posthog into bac…

457bf4d

…kend-queries

Update snapshots

cccf24d

mariusandra marked this pull request as ready for review January 26, 2023 18:06

mariusandra requested a review from thmsobrmlr January 26, 2023 18:06

Update snapshots

b085362

mariusandra requested review from pauldambra and Twixes January 26, 2023 18:07

Update snapshots

360f211

mariusandra requested a review from macobo January 27, 2023 09:29

Twixes reviewed Jan 27, 2023

View reviewed changes

Merge branch 'master' into backend-queries

a77ab10

thmsobrmlr approved these changes Jan 27, 2023

View reviewed changes

frontend/src/queries/nodes/DataTable/ColumnConfigurator/ColumnConfigurator.tsx Outdated Show resolved Hide resolved

mariusandra and others added 10 commits January 27, 2023 12:27

raise if invalid json

d8256b1

Update snapshots

6aa4064

remove __root__ hack

8aadcdd

improve comments, in and out of quotes

2b86581

shrug

4f4a840

future proofing

2760a0c

Merge remote-tracking branch 'origin/backend-queries' into backend-qu…

3cce8b5

…eries

comment

e42bc9f

rename to "run_events_query"

511f040

simplify

8756d17

mariusandra requested a review from Twixes January 27, 2023 12:03

Twixes approved these changes Jan 27, 2023

View reviewed changes

mariusandra merged commit 67453e5 into master Jan 27, 2023

mariusandra deleted the backend-queries branch January 27, 2023 14:30

mariusandra mentioned this pull request Jan 27, 2023

fix(events-explorer): fix before/after #13968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(data-exploration): data exploration backend API with types #13933

feat(data-exploration): data exploration backend API with types #13933

mariusandra commented Jan 26, 2023 •

edited

Loading

mariusandra commented Jan 26, 2023

Twixes Jan 27, 2023

mariusandra Jan 27, 2023

thmsobrmlr Jan 27, 2023

Twixes Jan 27, 2023

Twixes Jan 27, 2023

mariusandra Jan 27, 2023

Twixes Jan 27, 2023

mariusandra Jan 27, 2023

Twixes Jan 27, 2023

mariusandra Jan 27, 2023

thmsobrmlr left a comment

Twixes left a comment

mariusandra commented Jan 27, 2023

feat(data-exploration): data exploration backend API with types #13933

feat(data-exploration): data exploration backend API with types #13933

Conversation

mariusandra commented Jan 26, 2023 • edited Loading

Problem

Changes

How did you test this code?

mariusandra commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thmsobrmlr left a comment

Choose a reason for hiding this comment

Twixes left a comment

Choose a reason for hiding this comment

mariusandra commented Jan 27, 2023

mariusandra commented Jan 26, 2023 •

edited

Loading