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

Table.order_by should not warn about Floating_Point_Equality when sorting on Float columns #8213

Closed
2 of 3 tasks
radeusgd opened this issue Nov 2, 2023 · 1 comment · Fixed by #8221
Closed
2 of 3 tasks
Assignees
Labels
--bug Type: bug -libs Libraries: New libraries to be implemented

Comments

@radeusgd
Copy link
Member

radeusgd commented Nov 2, 2023

The Floating_Point_Equality warning is related to operations relying on equality - e.g. grouping. Sorting relies on ordering and the warning is irrelevant here, but apparently it is reported for order_by - this is a mistake.

After analysis, I can see it happens because we use the 'equality-oriented' MultiValueIndex which reports the Floating_Point_Equality warning, within Table::orderBy. That seems unnecessary - there is no need to create the index - we can just use List<OrderedMultiValueKey>::sort and extract the row ordering from that result.

  • Add a test checking that no warnings should be reported when sorting on a Float column.
  • Change Table::orderBy to use List::sort instead of the index.
  • Out of curiosity, we could compare the performance of before and after.
@radeusgd radeusgd self-assigned this Nov 2, 2023
@github-project-automation github-project-automation bot moved this to ❓New in Issues Board Nov 2, 2023
@radeusgd radeusgd added --bug Type: bug -libs Libraries: New libraries to be implemented labels Nov 2, 2023
@radeusgd radeusgd moved this from ❓New to 🔧 Implementation in Issues Board Nov 2, 2023
@enso-bot
Copy link

enso-bot bot commented Nov 6, 2023

Radosław Waśko reports a new STANDUP for the provided date (2023-11-04):

Progress: Fixed the order by issue (PR ready). Added better tests for Between and realised the current implementation fails many of them. Switched to a different implementation that is better suited. It should be finished by 2023-11-06.

Next Day: Next day I will be working on the #5303 task. Finish the new implementation of Between to make it better. Get the PR through. Start next tasks.

@mergify mergify bot closed this as completed in #8221 Nov 6, 2023
@github-project-automation github-project-automation bot moved this from 🔧 Implementation to 🟢 Accepted in Issues Board Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug -libs Libraries: New libraries to be implemented
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant