Use explicit sort key in ConcurrentExecutorListResults#716
Use explicit sort key in ConcurrentExecutorListResults#716dkropachev wants to merge 1 commit intomasterfrom
Conversation
Sort results by index only, avoiding unnecessary comparison of ExecutionResult namedtuples which could fail on heterogeneous result/exception types. Fixes #715
There was a problem hiding this comment.
I first sent an approve, and only then read an issue. The "problem" this fixes only happens if ids are equal. IDs can never be equal, because of how statements are generated: enumerate(iter(statements_and_params)).
This is not just some immaterial detail that could change in the future: user needs to get results in the same order they passed the requests, in order to be able to match them. This means this "bug" can never be triggered. So what problem is solved here?
Yuu are right, technically it is unlikely, but in some cases it happens, here is the example: |
|
If it happens, it indicates a more serious bug. Instead of silencing it with the fix in this PR, it needs to be fixed. |
Summary
Fixes bug in response handling:
key=lambda x: x[0], avoiding unnecessary comparison ofExecutionResultnamedtuples which could fail on heterogeneous result/exception typesFixes #715
Test plan
execute_concurrenttests cover correctness of result ordering