resolve_params to supply defaults#770
Conversation
Give parameterized custom types a mutating resolve_params overload so a type can canonicalize its parameters -- fill in omitted defaults -- and have the rewritten set persisted as the column's canonical parameters. This makes resolve_params the single place that both validates and completes a type's parameters, so int_to_params only needs to emit the one pair that the integer shorthand maps to.
robgolebiowski-vsql
force-pushed
the
robert/custom_types_create_split_default_params
branch
from
ea5367a to
47072d5
Compare
robgolebiowski-vsql
changed the title
| // A type using the mutating resolve_params overload can rewrite its params | ||
| // (e.g. fill in defaults). When it does, the rewritten string becomes the | ||
| // canonical parameters we key the context on and persist. | ||
| std::string canonical_params = params_str; |
There was a problem hiding this comment.
Why do this vs. change the resolve_params_and_context? We take on an extra copy here - not sure if we can avoid it by pushing this up higher...
There was a problem hiding this comment.
I am making a copy of param_str - so later I could compare canonical_params == params_str. Based on that - I know whether I can omit calling TypeParameters::from_raw - which is expensive.
| // NUL-terminated buffer. On success writes the params string to *params (empty | ||
| // when the length is 0) and returns false; on malformed input writes error_msg | ||
| // and returns true. | ||
| bool parse_rewritten_params(std::string_view section, std::string *params, |
There was a problem hiding this comment.
This comment is confusing because it mentions the two length fields. I know you mean persisted_length, max_decode_..., but i didn't at first. The section contains the byte length first, so that could be taken to mean the length you just mentioned and the mention of being after a comma makes it seem like section is just the optional part. Let me add a suggestion.
There was a problem hiding this comment.
I have updated the comment
| ResolvedTypeParams tmp = {}; | ||
| char rerr[VEF_MAX_ERROR_LEN] = {0}; | ||
| std::string canonical = result; | ||
| if (descriptor_->resolve_params_fn()->invoke(result, &tmp, rerr, |
There was a problem hiding this comment.
We are also calling resolve params below at line 211. Is it correct/necessary for us to be calling it twice?
There was a problem hiding this comment.
int_to_params is here called in a loop - for each parameter - we check if int_to_params produces the same parameters as the stored ones. Previously int_to_params would fill in all the defaults and could produce all the parameters that were stored. If it did - then we would print VECTOR(N) - shorted version in SHOW CREATE. However, now the responsibility of filling in the defaults is split between int_to_params and resolve_params - so we have to call the resolve_params here in the loop. Later we do the "second" call to resolve_params - in case it fails we report clean error.
There was a problem hiding this comment.
I have changed the comment a bit to make it more explicit.
robgolebiowski-vsql
deleted the
robert/custom_types_create_split_default_params
branch
## Description Add relnotes to existing draft: - #770: parameterized types can supply default parameters - #788: NOT NULL variable-length columns get a real intrinsic default AI=CLAUDE ## Related Issue n/a ## How was this tested? n/a ## Checklist - [x] I have signed the CLA - [x] I have read [CONTRIBUTING.md](https://github.com/villagesql/villagesql-server/blob/main/CONTRIBUTING.md) - [x] I have added or updated tests as appropriate - [x] My changes maintain compatibility with upstream MySQL 8.4 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Give parameterized custom types a mutating resolve_params overload so a type can canonicalize its parameters -- fill in omitted defaults -- and have the rewritten set persisted as the column's canonical parameters. This makes resolve_params the single place that both validates and completes a type's parameters, so int_to_params only needs to emit the one pair that the integer shorthand maps to.
Closes #791