split set_type_context: strict + update#438
Open
pyshx wants to merge 4 commits intovillagesql:mainfrom
Open
Conversation
Contributor
Author
Re-set sites3 from the call-site analysis on #409:
7 surfaced via the debug-build assert pass (they're post-factory sync points):
|
8 tasks
Companion to set_type_context() for sites that deliberately overwrite an existing TypeContext (e.g. the TD1 unknown -> known resolution path). Identical body for now; set_type_context() will gain a strict-null precondition assert in a follow-up commit, which is what makes the two names meaningful. Refs villagesql#409.
Three sites that deliberately overwrite an existing TypeContext now call update_type_context() instead of set_type_context(), expressing intent in the call: - villagesql/types/util.cc:1352 - TD1 unknown -> known resolution - sql/item_func.cc:7024 - Item_func_get_user_var::propagate_type (type propagation iterates; TC may be set from a prior pass) - sql/item.h:6980 - Item_cache::setup (Item_cache is reused in plan- reset paths in MySQL core) Behavior unchanged - both setters have identical bodies until set_type_context() gains a strict-null precondition in a follow-up. Refs villagesql#409.
Audit pass identified four more sites that re-set an existing TypeContext rather than first-setting on a fresh Field/Item: - sql/item.cc:3078 - Item_field::set_field, called from fix_fields which re-runs across prepared-statement re-executions - sql/sp_head.cc:1989 - sp_head::create_result_field, after make_field() has already pre-set TC from m_return_field_def - sql/sql_tmp_table.cc:298, 441 - post-factory TC sync sites; the factory paths (Item_sum::make_string_field, Field::new_field clone) pre-set TC, so these re-set rather than first-set - villagesql/types/util.cc:1185 (insert_tmp_metadata_for_thd), villagesql/types/util.cc:1604, 1615 (InjectCustomSpParams) - sync the shared_ptr-owned TC ref into Fields/Items that already hold the raw-pointer version from upstream setup Behavior unchanged - both setters still have identical bodies. The strict-null precondition lands in the next commit. Refs villagesql#409.
set_type_context now asserts custom_type == nullptr via should_assert_if_false (villagesql/include/error.h): debug builds abort on violation, release builds silently overwrite as the safe fallback per the maintainer's framing in villagesql#409. Moved the bodies out-of-line into sql/field.cc and sql/item.cc to keep the heavyweight error.h include (which pulls the component log_builtins) out of sql/field.h and sql/item.h. The assert reflects the call-site-intent split between set_ and update_type_context: callers that have already migrated to update_ preserve their existing-TC overwrite semantics; the remaining set_ callers are the first-set sites identified by the call-site audit posted on villagesql#409. Closes villagesql#409.
pyshx
force-pushed
the
pyshx/update-type-context
branch
from
8251c5b to
b29a323
Compare
| void Field::add_to_cost(CostOfItem *cost) const { cost->AddFieldCost(); } | ||
|
|
||
| void Field::set_type_context(const villagesql::TypeContext *tc) { | ||
| (void)should_assert_if_false(custom_type == nullptr); |
Member
There was a problem hiding this comment.
It's fine to make this assert(custom_type) because the fall through is the behavior we want for release mode. Same here - let's make this assert. This should also still be in the header file. It should effectively be inline.
| } | ||
|
|
||
| void Item::set_type_context(const villagesql::TypeContext *tc) { | ||
| (void)should_assert_if_false(custom_type == nullptr); |
| // VillageSQL: Copy custom type context for proper formatting of custom | ||
| // types | ||
| set_type_context(item->get_type_context()); | ||
| update_type_context(item->get_type_context()); |
Member
There was a problem hiding this comment.
How do we know this is the right call here? Are we expecting it to have a value already, or are we unsure and thus hedging?
| collation.set(var_entry->collation); | ||
|
|
||
| set_type_context(var_entry->type_context()); | ||
| update_type_context(var_entry->type_context()); |
| } | ||
| void set_type_context(const villagesql::TypeContext *tc) { custom_type = tc; } | ||
| void set_type_context(const villagesql::TypeContext *tc); | ||
| void update_type_context(const villagesql::TypeContext *tc) { |
Member
There was a problem hiding this comment.
Do we intend update to mean custom_type already had a value? If so, should we assert that?
How would someone deal with knowing whether to call one or the other without checking first? If update doesn't assert, then why ever call set_ ?
I wonder if adding such an assert would result in test failures after this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
update_type_context()toField(sql/field.h) andItem(sql/item.h) for sites that deliberately overwrite an existingTypeContext.set_type_context()now assertscustom_type == nullptrviashould_assert_if_false(villagesql/include/error.h): debug aborts, release silently overwrites — the safe-fallback shape from [Server]: Introduce update_type_context to supplement set_type_context #409.update_type_context(). No SQL-level behavior change.Related Issue
Closes #409 (approach 2 from the issue thread). Bodies of
set_type_context()are now out-of-line insql/field.cc/sql/item.ccto keeperror.h(transitively pulls component log_builtins) out ofsql/field.h/sql/item.h.How was this tested?
ctest -L villagesql— 7/7 pass--do-suite=village --nounit-tests --parallel=auto— 552 pass, 0 fail, zero asserts fired-Werrorissues (same as drop LEX_STRING from ResolveTypeToContext #412/drop LEX_STRING from VDF arg/return #420); trusting CIChecklist