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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Feb 28, 2023
Merged

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

merged 3 commits into from
Feb 28, 2023

Conversation

yakkomajuri
Copy link
Contributor

@yakkomajuri
Copy link
Contributor Author

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
Contributor Author

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

@yakkomajuri
Copy link
Contributor Author

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
Collaborator

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 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
Contributor

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
Contributor Author

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