Skip to content

Fix for sorting via avg aggregated column. #1286

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
Merged
texodus merged 3 commits into from
Jan 28, 2021
Merged

Fix for sorting via avg aggregated column. #1286

texodus merged 3 commits into from
Jan 28, 2021

Conversation

@texodus
Copy link
Member

@texodus texodus commented Jan 12, 2021 •
edited by sc1f
Loading

  • Isolate with test case
  • Fix the bug

lol.

Fixes #1285 .

@texodus texodus mentioned this pull request Jan 12, 2021
Closed
@texodus texodus added bug Concrete, reproducible bugs C++ labels Jan 12, 2021
@texodus texodus added the 0.6.1 label Jan 19, 2021
sc1f
sc1f reviewed Jan 22, 2021
View reviewed changes
@sc1f
Copy link
Contributor

sc1f commented Jan 22, 2021 •
edited
Loading

Added some changes that fix #1285 by re-enabling nan comparisons while sorting, which were disabled in #689 for performance fixes. After re-enabling nan comparisons and running benchmarks on various dataset sizes and sorted contexts, this change does not have a performance impact so it should be okay to re-enable in the interest of correctness.

I've also changed the default error logging from WASM to throw a detailed error message instead of Abort(), which will help in debugging (a small improvement from #1289 that I wanted to carry over).

@sc1f sc1f marked this pull request as ready for review January 22, 2021 00:57
@texodus
Copy link
Member Author

texodus commented Jan 28, 2021

Thanks for the fix!

@texodus texodus merged commit e913374 into master Jan 28, 2021
@texodus texodus deleted the sort-avg branch January 28, 2021 05:27
@texodus texodus removed the 0.6.1 label Jan 30, 2021
@texodus texodus added this to the 0.6.1 milestone Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@sc1f sc1f sc1f left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Concrete, reproducible bugs C++

Milestone

0.6.1

Development

Successfully merging this pull request may close these issues.

Sorting a column aggregated by avg does not sort

3 participants

@texodus @sc1f