fix(expressions): Use compound names in protobufs #24
Conversation
wackywendell
force-pushed
the
ext-simpler
branch
from
daee767
to
c0ccf37
Compare
| @@ -60,7 +60,7 @@ func ExampleExpression_scalarFunction() { | |||
| "extensionFunction": { | |||
| "extensionUriReference": 1, | |||
| "functionAnchor": 2, | |||
| "name": "add" | |||
| "name": "add:i32_i32" | |||
There was a problem hiding this comment.
Compound names are now required by the spec in the protobuf. Simple names should still be parsed in the protobuf, but in the round trip it will be turned back into a compound name, hence the update here
wackywendell
changed the title
| @@ -142,7 +142,6 @@ func BoundFromProto(b *proto.Expression_WindowFunction_Bound) Bound { | |||
|
|
|||
| type ScalarFunction struct { | |||
| funcRef uint32 | |||
| id extensions.ID | |||
There was a problem hiding this comment.
The name and URI are in the declaration *extensions.ScalarFunctionVariant, so for simplification purposes, I removed the duplication so the two would not get out of sync
| func (s *ScalarFunction) Name() string { return s.declaration.Name() } | ||
| func (s *ScalarFunction) CompoundName() string { return s.declaration.CompoundName() } |
There was a problem hiding this comment.
I switched Name() to now be the "simple" name (e.g. add), and CompoundName() to now be the, well, compound name add:i32_i32, to keep the naming consistent with that used with the ScalarFunctionVariant. This wasn't particularly needed or all that useful here, just seemed like a relevant minor improvement
| func (s *ScalarFunction) ID() extensions.ID { return s.id } | ||
| func (s *ScalarFunction) Name() string { return s.declaration.Name() } | ||
| func (s *ScalarFunction) CompoundName() string { return s.declaration.CompoundName() } | ||
| func (s *ScalarFunction) ID() extensions.ID { return s.declaration.ID() } |
There was a problem hiding this comment.
ID is now taken from the variant, which should always be at least as populated as the ScalarFunction.
| for k, v := range e.uris { | ||
| if v == uri { | ||
| return k, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
GetFuncAnchor() calls this function, and if you use the same URI for two different functions, this deduplicates the URI list. As such, I changed the name to addOrGetURI to reflect that it may be returning a pre-existing index
extensions/variants.go
Outdated
| // When parsing a function variant from a string, we may know the string form of | ||
| // an argument, but nothing else. This represents that type of argument. | ||
| type unknownArgType struct { | ||
| arg string | ||
| } |
There was a problem hiding this comment.
This was useful to populate a ScalarFuncVariant from only a name, but it's a bit hacky. Suggestions welcome if you have better ideas.
There was a problem hiding this comment.
we already have a parser for the type strings, why not just parse the arg type string into actual Type instances?
There was a problem hiding this comment.
That sounds like it would make sense 😅 . I looked for it a bit, but I didn't find it; can you point me to it?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, thanks! 🤦 Now that I'm looking for it, I see it's already used in this very file 😓
| @@ -101,13 +101,14 @@ func TestBuildEmitOutOfRangePlan(t *testing.T) { | |||
| } | |||
|
|
|||
| func checkRoundTrip(t *testing.T, expectedJSON string, p *plan.Plan) { | |||
| t.Helper() | |||
There was a problem hiding this comment.
Not required but was useful in debugging a test
| @@ -1004,6 +1005,159 @@ func TestSortRelationErrors(t *testing.T) { | |||
| assert.ErrorContains(t, err, "output mapping index out of range") | |||
| } | |||
|
|
|||
| func TestProjectExpressions(t *testing.T) { | |||
There was a problem hiding this comment.
New test, round-tripping some function expressions. This fails without this PR: the URI gets duplicated, and it uses simple function names and not compound ones
| // Sort extensions by the anchor for consistent output | ||
| sort.Slice(uris, func(i, j int) bool { return uris[i].ExtensionUriAnchor < uris[j].ExtensionUriAnchor }) |
There was a problem hiding this comment.
This is not strictly necessary, but seemed nicer: it's consistent, which is nice; it's in order of anchors, which also seems nice; and it makes testing easier, because its deterministic
This PR finds and fixes a number of related issues:
This also updates tests to match the above - in particular, any tests with protobuf output now uses compound names.
I made some choices along the way for how to make this work; I'll comment inline in this PR.
Supersedes #23.