Could someone look at this <error> for my cross jo...
# daft-devc
Cory Grinstead
10/24/2024, 5:12 PMCould someone look at this error for my cross join PR. It looks like it's potentially unrelated. other join tests are sometimes failing because the ordering is non-deterministic.
I dont think this optimizer rule would have any effect on the output ordering of a sortmerge join.
j
jay
10/24/2024, 5:25 PMcc @Kevin Wang
c
Colin Ho
10/24/2024, 5:37 PMit does look like a sort problem
k
Kevin Wang
10/24/2024, 5:42 PMwill take a look
c
Cory Grinstead
10/25/2024, 2:30 PM@Kevin Wang could you try to look at this today? This PR is critical for a lot of the tpch and tpc-ds queries.
k
Kevin Wang
10/25/2024, 4:50 PMYes I will. Sorry for the delay
Kevin Wang
10/25/2024, 9:57 PMHey @Cory Grinstead do you have a good grasp of how the datafusion optimizer rule actually works? I'm kind of confused and I also cannot reproduce the sort-merge join test failure
Kevin Wang
10/25/2024, 10:13 PMHowever one thing you could do if you would like to make it so that this optimization rule does not affect non-cross joins is to match on inner joins where the left_on and right_on are empty
Kevin Wang
10/25/2024, 10:30 PMAccording to my understanding, all we need for this rule is actually two rules that are actually applicable in general:
1. push filter INTO join, which converts a predicate into a join key if it is an equality expression where one side is only left columns and the other side is only right columns
2. push filter THROUGH join (which we already have), which moves a predicate from the parent to a child of a join if it uses a subset of the side's columns. this will allow for the predicates to be pushed into joins in the multi cross join case
I think this rule is trying to do both and only for cross joins, which makes it kind of hard to reason about
c
Cory Grinstead
10/27/2024, 9:19 PMI just realized why this is failing. It was matching on
JoinStrategy::SortMerge🔥 2
Cory Grinstead
10/28/2024, 12:26 PMHey @Kevin Wang I saw your comments on the PR. Thanks for the review 🙌
I had a good amount of comments but I'm happy to branch off of your changes and make the fixes myself.I wasn't sure which comments were the most pressing, or how you wanted to handle this? Im fine with making these changes, but also was curious if it would make sense to do some of these in a followup PR? It does work as expected right now, and unblocks a lot of my other SQL work.
k
Kevin Wang
10/28/2024, 5:00 PMI think if this is blocking you, then a follow-up PR for my review changes would make sense. I don't see anything in the PR that would break any current behavior
j
jay
10/28/2024, 5:01 PMIf we have tests in place, follow-up refactor PR should be fine
k
Kevin Wang
10/28/2024, 5:02 PMI'm slightly worried about how well our tests cover regressions but that is an overarching issue that should also be a separate PR
c
Cory Grinstead
10/28/2024, 5:15 PMok sounds good. I already addressed some of the smaller comments. Unless any objections, I'll merge it in and we can revisit it later on!
k
Kevin Wang
10/28/2024, 5:29 PMSounds voodoo. thanks cory!
Kevin Wang
10/28/2024, 6:40 PM@Cory Grinstead with this merged, can we run all TPC-H queries in SQL? I would be interested in looking at our plans from the sql queries
c
Cory Grinstead
10/28/2024, 6:49 PMI think we should be able to run a lot of them, I'll try it out and see what ones fail.
👍 1
2 Views