feat: make optimization a repeated field #653
Conversation
vbarua
requested review from
jacques-n,
cpcloud,
westonpace and
EpsilonPrime
as code owners
|
Following up on #649 (comment) |
| @@ -72,7 +72,7 @@ message SimpleExtensionDeclaration { | |||
| message AdvancedExtension { | |||
| // An optimization is helpful information that don't influence semantics. May | |||
| // be ignored by a consumer. | |||
| google.protobuf.Any optimization = 1; | |||
| repeated google.protobuf.Any optimization = 1; | |||
|
|
|||
| // An enhancement alter semantics. Cannot be ignored by a consumer. | |||
| google.protobuf.Any enhancement = 2; | |||
There was a problem hiding this comment.
The discussion in #649 indicated that optimization should be made repeated, but called out that enhancement should not be repeated. Is there a specific reason why we wouldn't want that?
There was a problem hiding this comment.
Yes. An enhancement feels more like an "is a" relationship and less like a "has a" relationship. In other words, an enhancement is likely to change the semantic intent of the relation in some way. Combining two enhancements is unlikely to be successful.
For example, let's assume that we had a hash join relation enhancement that applies a pre-filter (I know that such a property already exists but we'll assume it was forgotten). At the same time let's assume we have some strange enhancement that specifies which collation to use for the equality comparison. This enhancements would not be compatible (the pre-filter likely would perhaps not be expecting the collation to be used).
There was a problem hiding this comment.
That being said, I don't think it was ruled out. Partly we just don't really have any idea what valid use cases for enhancement look like yet. Until someone is using it though it probably doesn't have to be decided one way or the other.
There was a problem hiding this comment.
Makes sense to me. We can limit this to optimization for now.
vbarua
force-pushed
the
vbarua/allow-multiple-optimization
branch
from
dd7a352
to
0844cf1
Compare
| @@ -72,7 +72,7 @@ message SimpleExtensionDeclaration { | |||
| message AdvancedExtension { | |||
| // An optimization is helpful information that don't influence semantics. May | |||
| // be ignored by a consumer. | |||
| google.protobuf.Any optimization = 1; | |||
| repeated google.protobuf.Any optimization = 1; | |||
There was a problem hiding this comment.
meta: I would have liked to pluralize this (i.e. optimizations) this, but that is a breaking change due to json encoding.
BREAKING CHANGE: consumers must now check for multiple optimization messages within AdvancedExtensions