feat: Add ChangeMapping interface to relations #71
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 59.14% 59.56% +0.41%
==========================================
Files 43 43
Lines 9221 9298 +77
==========================================
+ Hits 5454 5538 +84
+ Misses 3523 3516 -7
Partials 244 244 ☔ View full report in Codecov by Sentry. |
plan/plan.go
Outdated
| @@ -275,6 +275,7 @@ type Rel interface { | |||
| // record type from the output RecordType of the underlying Relation. | |||
| // If the output mapping is nil, then this just returns the input | |||
| // type. | |||
| // TODO - Rename this ApplyMap. | |||
There was a problem hiding this comment.
Add api to interface.
Also, I think we should be copying, not overwriting. Overwriting/mutating nodes in a plan can be dangerous given the possibility of duplicated subplans, etc.
Maybe call CopyWithRemap() or similar.
There was a problem hiding this comment.
Remap doesn't really make sense on write relations so I specifically created an alternative interface.
I'm less inclined to worry about copying as these are more of a build time artifact (i.e. not protos). If one wants to duplicate a subplan that should do the copying.
There was a problem hiding this comment.
I specifically created an alternative interface.
I don't see the interface. What is it?
I'm less inclined to worry about copying
I think we should prefer immutability generally. Having worked on this type of pattern before, I think that will reduce future problems and mistakes.
| @@ -741,6 +776,12 @@ func (j *JoinRel) CopyWithExpressionRewrite(rewriteFunc RewriteFunc, newInputs . | |||
| return &join, nil | |||
| } | |||
|
|
|||
| func (j *JoinRel) ChangeMapping(mapping []int32) error { | |||
| newMapping, err := ChangeMapping(j, mapping) | |||
| j.mapping = newMapping | |||
There was a problem hiding this comment.
If there is an error, we lose the old mapping if I reading this right. If err, I would expect no change.
| // | ||
| // If any column numbers specified are outside the currently available input | ||
| // range an error is returned and the mapping is left unchanged. | ||
| ChangeMapping(mapping []int32) error |
There was a problem hiding this comment.
Can we have this make a copy as opposed to an inplace edit?
I'm also more OverlayMapping or ApplyMapping as a name.
Also, would be good to use vararg.
| @@ -271,6 +271,16 @@ type Rel interface { | |||
| // result should be 3 columns consisting of the 5th, 2nd and 1st | |||
| // output columns from the underlying relation. | |||
| OutputMapping() []int32 | |||
| // ClearMapping resets the mapping for this relation. | |||
| ClearMapping() | |||
There was a problem hiding this comment.
I don't think we should add this.
| @@ -271,6 +271,16 @@ type Rel interface { | |||
| // result should be 3 columns consisting of the 5th, 2nd and 1st | |||
| // output columns from the underlying relation. | |||
| OutputMapping() []int32 | |||
There was a problem hiding this comment.
I think we should deprecate this.
There was a problem hiding this comment.
On second, thought, maybe not deprecate. Let's just update to be a defensive copy.
ChangeMapping is an alternative to the current Remap variants currently found on the Builder interface. It preserves the existing behavior including checking the proposed mapping for validity. It also modifies the relation instead of returning a copy. All of the existing Remap function variants have been marked as deprecated in favor of ChangeMapping.