Skip to content

fix(breakdown): ensure breakdown sort can sort through values of different types#14459

Merged
yakkomajuri merged 3 commits intomasterfrom
fix-breakdown-sort
Feb 28, 2023
Merged

fix(breakdown): ensure breakdown sort can sort through values of different types#14459
yakkomajuri merged 3 commits intomasterfrom
fix-breakdown-sort

Conversation

@yakkomajuri
Copy link
Copy Markdown
Contributor

@yakkomajuri yakkomajuri commented Feb 28, 2023

@yakkomajuri
Copy link
Copy Markdown
Contributor Author

yakkomajuri commented Feb 28, 2023

Will get this in then to unblock the customer while I try to make sense of all the breakdown stuff to add a regression test

@yakkomajuri
Copy link
Copy Markdown
Contributor Author

yakkomajuri commented Feb 28, 2023

ah nope string sorting doesn't always give us what we want - will update

@yakkomajuri
Copy link
Copy Markdown
Contributor Author

yakkomajuri commented Feb 28, 2023

how do we feel about this now? @mariusandra

not super clean but honestly the cleanest approach here also since it's "Pythonic" to do things via exception handling.

@mariusandra
Copy link
Copy Markdown
Collaborator

mariusandra commented Feb 28, 2023

Still good for me. As you can guess I didn't try your fix, but assumed worse case a broken fix on a broken thing is a no-op anyway :D.

yakkomajuri
yakkomajuri commented Feb 28, 2023
View reviewed changes
Comment thread posthog/queries/trends/breakdown.py Outdated
@yakkomajuri yakkomajuri enabled auto-merge (squash) February 28, 2023 19:53
@yakkomajuri yakkomajuri merged commit 1c6e46b into master Feb 28, 2023
@yakkomajuri yakkomajuri deleted the fix-breakdown-sort branch February 28, 2023 20:02
fuziontech added a commit that referenced this pull request Feb 28, 2023
@fuziontech
Merge branch 'master' into eyes_on_ch
0e8ba23 
* master: (53 commits)
  fix: cut the undeterministic snapshots (#14461)
  feat(hogql): Events table based on hogql (#14315)
  fix(breakdown): ensure breakdown sort can sort through values of different types (#14459)
  feat(capture): gracefully catch non-string tokens (#14453)
  chore(plugin-server): add healthcheck logging for failure (#14455)
  fix(tests): Safer migrations (#14452)
  revert: "fix(person-overrides): add constraints to catch race conditions" (#14445)
  dev(codespaces): update to python3.10 (#14449)
  chore(deps): Update posthog-js to 1.50.0 (#14448)
  feat(capture): support LIGHTWEIGHT_CAPTURE_ENDPOINT_ENABLED_TOKENS=* (#14421)
  chore: upgrade d3 (#14442)
  fix(person-overrides): add constraints to catch race conditions (#14277)
  feat(cohorts): Remove postgres calculations for flags (#14272)
  feat(capture): check token shape before team resolution too (#14439)
  feat: dashboard templates (#14322)
  feat: add a 'What's New?' button to the dropdown (#14379)
  chore(recordings): don't DLQ on PostgreSQL errors (#14438)
  chore: update autocapture attribute capture (#14435)
  chore(recordings): remove hub dependency on recordings ingestion (#14418)
  chore(deps): Update posthog-js to 1.49.0 (#14436)
  ...
@neilkakkar
Copy link
Copy Markdown
Contributor

neilkakkar commented Mar 1, 2023

Flyby that the 'ideal' fix here would be converting the histogram case breakdowns to float always (since we already know these breakdowns are numbers, since working on a numeric property), so we can do float(json.loads(blah)) instead of str() and it should work for all cases we have so far, and the regression test you'll add as well.

@yakkomajuri
Copy link
Copy Markdown
Contributor Author

yakkomajuri commented Mar 1, 2023

ah ok nice

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

Reviewers

@mariusandra mariusandra mariusandra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yakkomajuri @mariusandra @neilkakkar