Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Postgres function should_update_person_props #6259

Merged
merged 9 commits into from
Oct 7, 2021

Conversation

yakkomajuri
Copy link
Contributor

@yakkomajuri yakkomajuri commented Oct 5, 2021

Changes

WIP

This is a key step for https://github.com/PostHog/plugin-server/issues/371

This was forked off of #6253.

Putting it out as a WIP in order to get feedback quickly before moving onto plugin server work.

Massive shoutout to @tiina303 for the table you wrote about how this function should behave. Made this so much easier, particularly writing tests.

How did you test this code?

Added tests for every potential case the function would encounter.

@yakkomajuri yakkomajuri requested a review from tiina303 October 5, 2021 13:56
@yakkomajuri yakkomajuri temporarily deployed to posthog-pr-6259 October 5, 2021 13:58 Inactive
@yakkomajuri
Copy link
Contributor Author

yakkomajuri commented Oct 5, 2021

Could realistically get rid of #6253 in favor of this if we're able to merge quickly - maybe even make it one migration only 🤔

Copy link
Contributor

@tiina303 tiina303 left a comment

Choose a reason for hiding this comment

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

Approving, but do address my comment about equal timestamps function output value before landing.

Could realistically get rid of 6253 in favor of this if we're able to merge quickly

I'm fine with that, see comments there too.

This was forked off

btw you can use --base <the-other-branch-name> which will make it show only the new code here in code review. Not yet fully sure how to merge on master later, but I think it's possible to select a branch or alternatively rebase on master locally first & force push an update.

Added tests for every potential case the function would encounter.

❤️

@yakkomajuri
Copy link
Contributor Author

@tiina303 well if I set the base as the other branch it will only show what's new but it will be a PR to that branch, not master, which is where I want the changes to go 😆

Great comments though, will address

@hazzadous
Copy link
Contributor

What's the reason for doing this as a psql function btw?

@yakkomajuri
Copy link
Contributor Author

@hazzadous this will make things so much cleaner and easier to debug. The query for updating properties is already going to be somewhat messy and this helps abstract some of it away + test it individually.

It's not necessary, but much easier to work with/debug/think about conceptually.

@hazzadous
Copy link
Contributor

Ok. I wonder what the process is for updating these types of things e.g. do we have to create a migration each time we want to update and test and deploy, and how experienced/comfortable people will be to make changes to this.

@yakkomajuri yakkomajuri force-pushed the psql-should-update-prop branch from 5b13433 to 523d646 Compare October 6, 2021 12:36
@yakkomajuri yakkomajuri temporarily deployed to posthog-pr-6259 October 6, 2021 12:37 Inactive
@yakkomajuri yakkomajuri temporarily deployed to posthog-pr-6259 October 6, 2021 13:38 Inactive
@yakkomajuri yakkomajuri temporarily deployed to posthog-pr-6259 October 6, 2021 13:48 Inactive
@yakkomajuri yakkomajuri changed the title WIP Postgres function should_update_person_props Postgres function should_update_person_props Oct 6, 2021
@yakkomajuri yakkomajuri temporarily deployed to posthog-pr-6259 October 6, 2021 18:15 Inactive
@yakkomajuri yakkomajuri temporarily deployed to posthog-pr-6259 October 6, 2021 18:17 Inactive
@yakkomajuri yakkomajuri temporarily deployed to posthog-pr-6259 October 6, 2021 18:18 Inactive
@yakkomajuri yakkomajuri temporarily deployed to posthog-pr-6259 October 6, 2021 18:23 Inactive
Copy link
Contributor

@tiina303 tiina303 left a comment

Choose a reason for hiding this comment

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

Awesome 🚢

@yakkomajuri yakkomajuri merged commit 1f951f7 into master Oct 7, 2021
@yakkomajuri yakkomajuri deleted the psql-should-update-prop branch October 7, 2021 08:41
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.

3 participants