-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql: wrong sorting after statistics init #33354
Comments
Thanks for the detailed report @datobatone! cc @RaduBerinde for triage |
@datobatone - the execution plan looks ok to me. Have you seen out of order results? Can you post the results of EXPLAIN (VERBOSE) and EXPLAIN (DISTSQL)? |
first sort, and then join. As a result, the data is scattered EXPLAIN (VERBOSE)
EXPLAIN (DISTSQL) |
Thanks! The lookup join is supposed to preserve the ordering of the input. It does a lookup for every input row and returns results in order. Just to confirm - you saw actual results out of order, or do you think there is an issue just from the explain output? |
Yes, we saw that the data is not sorted by name, they go in a mixed order. The plan shows that the sequence of actions is changing. Before the inclusion of statistics, it first join and then sorts(all correct), and after activating statistics, first sorts, and only then it connects. As a result, we get the wrong sorting. |
Thanks. I will try to reproduce in a few days. One more question, is this on a single node cluster? The DISTSQL plan above seems to be on a single node (or at least the tables involved are all on one node). I'm asking because it might also be a problem of how we merge results from multiple nodes. CC @solongordon - there was a problem with the lookip joiner and ordering, but I think it only concerned outer joins, can you confirm that's the case? |
Yes, we use one node configuration. |
Thank you! At this point, the only possible culprit seems to be the lookup joiner. I will try to reproduce. CC @jordanlewis |
@datobatone as a workaround you can delete the statistics from the |
@RaduBerinde have you had any luck reproducing this? I will try to take a look, just let me know what you've done so far. |
@jordanlewis sorry, I was out for the holidays, I'm back today. I was planning to look at it today. |
I'm having trouble cajoling the optimizer to put the sort before the lookup join.. @datobatone if you still have the environment, could you also run |
before stats
after stats
|
Never mind - I was able to get that plan. I have not been able to come up with a case where the result ordering is incorrect though. @datobatone do you think you might be able to share your dataset with us? For the record, this is how I reproduced the plan:
|
hi. |
@datobatone thanks, this is great! I loaded that file (it's the same in both archives btw), created stats, then ran the query (but selected just The result is indeed out of order. I added some debugging messages to joinReader and it is messing up the order: https://gist.github.com/RaduBerinde/28d1c0bf4a3f16e216ffeb8770e79b43 Note that the input rows are ordered but the output rows (which project only the name column from the input) are in another order. @jordanlewis / @solongordon I am putting this on your plate (I tried to look at the joinreader code a bit but I'm no longer very familiar with it). |
The problem is that the join reader makes an incorrect assumption, that each input row's join key is unique. This is a problem because it assumes that the order that comes back from the scan is the correct output order in the case of inner join, since the kv fetcher will respect the order of the spans that you pass it. But if you have keys that need the same span, all of a sudden the ordering that the scanner gives you back is incorrect. This was likely a problem since lookup join was added, since it was extended from index join: index join does not have to solve this problem since it is guaranteed that each input row has a unique key into the lookup table. The fix is to reverse the order of the mapping between input rows and lookup rows - keep it from input to lookup, rather than the reverse, which is what it is today. |
Minimal repro:
|
The logic test in |
33536: distsqlrun: fix lookup join order preservation r=jordanlewis a=jordanlewis Fixes #33354. Previously, a lookup join with a left side that doesn't have an injective mapping into the right side would fail to preserve its input order, in violation of its contract. Now, the code is modified to use the order-preserving pathway that was designed for outer joins all the time, to prevent the issue. Release note (bug fix): lookup joins preserve their input order even if more than one row of the input corresponds to the same row of the lookup table. Co-authored-by: Jordan Lewis <[email protected]>
before statistics init:
FROM "requests" JOIN "organizations" ON (organizations.id = requests.organization_id)
JOIN "places" AS "load_place_id_places" ON (load_place_id_places.id = requests.load_place_id)
ORDER BY "requests"."name" desc
Statistics init:
CREATE STATISTICS places_stats ON id FROM places;
table places has about 300k entries.
explain again:
So after create stats we have wrong sorting.
Environment:
The text was updated successfully, but these errors were encountered: