Added CREATE OPERATOR and CREATE AGGREGATE - #3059
Conversation
|
|
SummaryCoverage spans database object creation and lifecycle management, query parsing and operator behavior, aggregate edge cases involving empty and null inputs, cross-session visibility, restart and branch-merge persistence, and extension materialization. It includes normal workflows, boundary conditions, invalid definitions, name resolution conflicts, cleanup, and conflict handling, with the exercised behavior broadly healthy. Safe to merge — the only failure is a pre-existing medium-severity compatibility issue unrelated to this PR, with no regressions or PR-attributable failures identified. The unrelated issue is a flag for later rather than a merge blocker. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Aggregate overloads reject common type names
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
zachmu
left a comment
There was a problem hiding this comment.
Overall this looks fine. The mixing of serialization and in-memory storage concerns can be a followup. A larger problem is that this makes the compatibility tests fail, because we're always writing the new flatbuffer fields even when they're empty. They need to be conditionally written only when they have content, otherwise this breaks every existing customer (rather than just the ones who start using this new feature, which will be a small minority).
Once that's fixed I'm good to approve.
| } | ||
|
|
||
| // AddOperator adds a new operator. | ||
| func (pgo *Collection) AddOperator(ctx context.Context, o Operator) error { |
There was a problem hiding this comment.
It's a little odd that adding an operator also serializes it to the underlying map. That combines two very separate concerns kind of inappropriately. Do all the collection types work this way?
There was a problem hiding this comment.
These two do at least. It would be really nice to separate the concerns of serialization from basic in-memory representation in all our root object collections.
| ), | ||
| Auth: vitess.AuthInformation{ | ||
| AuthType: auth.AuthType_CREATE, | ||
| TargetType: auth.AuthTargetType_TODO, |
| } | ||
| } | ||
| isAggregate := len(aggOverloads) > 0 || len(AggregateCatalog[name]) > 0 | ||
| if err = addBuiltInOverloads(overloadTree, name); err != nil { |
There was a problem hiding this comment.
Seems like this line should go below the early return block immediately below
|
|
||
| // String implements the interface sql.ExecSourceRel. | ||
| func (c *CreateAggregate) String() string { | ||
| return "CREATE AGGREGATE" |
There was a problem hiding this comment.
Good to include a name in these String funcs, they're useful in debugging
Same comment for other types in the node package
| }, | ||
| Assertions: []ScriptTestAssertion{ | ||
| { | ||
| Query: `CREATE OPERATOR <%> (LEFTARG = text, RIGHTARG = text, FUNCTION = op_ci_eq, COMMUTATOR = <%>, HASHES, MERGES);`, |
There was a problem hiding this comment.
Not obvious what hashes and merges do here or if we're just testing that they are stored, a comment would help
| Expected: []sql.Row{}, | ||
| }, | ||
| { | ||
| Query: `SELECT ROW(1, 2)::op_pair <+> ROW(3, 4)::op_pair;`, |
There was a problem hiding this comment.
Does this work in a query on table rows, maybe in an aggregate func?

This adds support for the
CREATE OPERATORandCREATE AGGREGATEstatements, which we need to support additional emulated extensions.