MDEV-38144: update Optional_metadata_fields to use new aggregate per-column struct#4720
MDEV-38144: update Optional_metadata_fields to use new aggregate per-column struct#4720EricHayter wants to merge 16 commits into
Optional_metadata_fields to use new aggregate per-column struct#4720Conversation
|
Running |
EricHayter
force-pushed
the
mdev-38144
branch
2 times, most recently
from
1c7c5f4 to
18a122c
Compare
I think it was determined that the coding standard wasn't acheivable with clang-format. I'm not sure if worth the effort. |
grooverdan
added
the
External Contribution
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
Hi @EricHayter !
Thank you for submitting the PR! It is good to update our types to be consistent with MariaDB types. One thing this PR misses, that is in the description of MDEV-38144, is that rather than having an array per field, it would be better to have one array that has elements of a complex type which describe a field.
Previously, another contributor had prepared a patch, PR #4494, which looked to satisfy this; however, it was never thoroughly reviewed because they never got it to compile / pass tests on our CI system. I'd suggest looking through it for some inspiration (though I'm not sure of the quality of it).
Something the previous patch didn't try (though the JIRA mentions) is to use the Column_definition type defined in sql/field.h. That would be another improvement.
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
LGTM after reverting the space-only changes.
Please keep working with Brandon on the final review.
| size_t old_size= elements(); | ||
| if (reserve(new_size)) | ||
| return true; | ||
There was a problem hiding this comment.
please avoid space only changes.
Noted. I'll take a look at that previous PR and update mine accordingly. Thanks. |
Currently Optional_metadata_fields has many members that use classes from the C++ standard library, most notably the use of std::vector and std::string. These members have been updated to use existing MariaDB types instead.
Beginning to create a per-field aggregate struct for all of the possibly required metadata values. Slowly going to start incorporating the existing vectors into this.
EricHayter
changed the title
Optional_metadata_fields to use MariaDB typesOptional_metadata_fields to use new aggregate per-column struct
|
I've made some changes to the PR introducing a new struct ( Please note that this newer version of the PR does end up changing more code as the logic for parsing the optional metadata data is a bit more involved with this new struct. I've also updated the PR title to be representative of the change. |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Please keep a single commit.
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
Thanks for the update, @EricHayter ! Sorry it took so long for review, I missed that you'd updated the patch.
You've clearly spent time to understand what each piece of code is doing, and put thought into "what makes sense." That is great, thank you for the effort!
Please see my notes.
|
Thank you very much for the feedback @bnestere. I will be away from home for a few weeks, so unfortunately I won't be able to address these comments right away, but I will still look to get these resolved. |
Wasn't using the fact that the default charset and charset fields were mutually exclusive. Also removed the need to use optionals anywhere in the optional metadata stuff. Just filling the char columns with the charset first and then overrriding later...
5 participants
Currently
Optional_metadata_fieldshas many members that use classes from the C++ standard library, most notably the use ofstd::vectorandstd::string. These members have been updated to use existing MariaDB types instead.