opt: improve FDs for Intersect and Except variants #66444
Conversation
|
This change is |
pkg/sql/opt/props/func_dep.go
Outdated
| @@ -551,6 +551,23 @@ func (f *FuncDepSet) CopyFrom(fdset *FuncDepSet) { | |||
| f.hasKey = fdset.hasKey | |||
| } | |||
|
|
|||
| // RemapFrom copies the given FD into this FD, remapping column IDs according to | |||
| // the given function. | |||
| func (f *FuncDepSet) RemapFrom(fdset *FuncDepSet, remap func(opt.ColumnID) opt.ColumnID) { | |||
There was a problem hiding this comment.
There's a function TranslateColSet in opt/colset.go that already has this sort of logic - you could just pass in the left and output ColLists here and call TranslateColSet
There was a problem hiding this comment.
Nice!
We can pass through FDs from the left side on Intersect/IntersectAll,
and we can pass through FDs from both sides on Except/ExceptAll.
Isn't it the opposite? (i.e., can pass from both sides for intersect but not except?) I think the code is already doing the right thing, just seems like the commit/PR message needs to be updated.
Might be worth adding an explicit test that this is the case, though.
Reviewed 15 of 15 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)
pkg/sql/opt/props/func_dep.go, line 556 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
There's a function
TranslateColSetin opt/colset.go that already has this sort of logic - you could just pass in the left and output ColLists here and callTranslateColSet
Agreed. I did something similar here with OrderingChoice:
We can pass through FDs from the left side on Except/ExceptAll; and we can pass through FDs from both sides on Intersect/IntersectAll. Release note: None
There was a problem hiding this comment.
Commit message fixed. There are some existing tests in logprops/set.
Reviewable status:
pkg/sql/opt/props/func_dep.go, line 556 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Agreed. I did something similar here with OrderingChoice:
Done. I found that most current usecases of this function legitimately pass in columns that are not in the from list. I updated the comments and added a Strict version and converted some of the existing calls.
There was a problem hiding this comment.
Reviewed 8 of 8 files at r2.
Reviewable status:
pkg/sql/opt/props/func_dep.go, line 556 at r1 (raw file):
Previously, RaduBerinde wrote…
Done. I found that most current usecases of this function legitimately pass in columns that are not in the
fromlist. I updated the comments and added aStrictversion and converted some of the existing calls.
Thanks!
|
Build succeeded: |
We can pass through FDs from the left side on Except/ExceptAll; and we
can pass through FDs from both sides on Intersect/IntersectAll.
Release note: None