Skip to content

Clean up string_view usage in metadata_modifier #646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wipheg wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Clean up string_view usage in metadata_modifier #646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions villagesql/schema/descriptor/index_profile_descriptor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include <cstdint>
#include <string>
#include <string_view>
#include <vector>

#include "villagesql/schema/descriptor/index_type_descriptor.h"
Expand All @@ -41,15 +42,15 @@ struct TableTraits;
// "normalized_profile_name.normalized_ext_name."
struct IndexProfileDescriptorKeyPrefix {
// Query by profile name only (for unqualified lookups).
explicit IndexProfileDescriptorKeyPrefix(std::string profile_name)
: profile_name_(std::move(profile_name)),
explicit IndexProfileDescriptorKeyPrefix(std::string_view profile_name)
: profile_name_(profile_name),
normalized_prefix_(normalize_extension_name(profile_name_) + ".") {}

// Query by profile name + extension name.
IndexProfileDescriptorKeyPrefix(std::string profile_name,
std::string extension_name)
: profile_name_(std::move(profile_name)),
extension_name_(std::move(extension_name)),
IndexProfileDescriptorKeyPrefix(std::string_view profile_name,
std::string_view extension_name)
: profile_name_(profile_name),
extension_name_(extension_name),
normalized_prefix_(
normalize_extension_name(profile_name_) + "." +
(extension_name_.empty()
Expand Down
13 changes: 7 additions & 6 deletions villagesql/schema/descriptor/index_type_descriptor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#define VILLAGESQL_SCHEMA_DESCRIPTOR_INDEX_TYPE_DESCRIPTOR_H_

#include <string>
#include <string_view>

#include "villagesql/schema/systable/helpers.h"
#include "villagesql/sdk/include/villagesql/abi/preview/index.h"
Expand All @@ -35,15 +36,15 @@ struct TableTraits;
// "normalized_index_type_name.normalized_ext_name."
struct IndexTypeDescriptorKeyPrefix {
// Query by index type name only (for unqualified lookups).
explicit IndexTypeDescriptorKeyPrefix(std::string index_type_name)
: index_type_name_(std::move(index_type_name)),
explicit IndexTypeDescriptorKeyPrefix(std::string_view index_type_name)
: index_type_name_(index_type_name),
normalized_prefix_(normalize_type_name(index_type_name_) + ".") {}

// Query by index type name + extension name.
IndexTypeDescriptorKeyPrefix(std::string index_type_name,
std::string extension_name)
: index_type_name_(std::move(index_type_name)),
extension_name_(std::move(extension_name)),
IndexTypeDescriptorKeyPrefix(std::string_view index_type_name,
std::string_view extension_name)
: index_type_name_(index_type_name),
extension_name_(extension_name),
normalized_prefix_(
normalize_type_name(index_type_name_) + "." +
(extension_name_.empty()
Expand Down
12 changes: 7 additions & 5 deletions villagesql/schema/descriptor/type_descriptor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include <cstdint>
#include <optional>
#include <string>
#include <string_view>
#include <utility>

#include "villagesql/schema/systable/helpers.h"
Expand All @@ -43,14 +44,15 @@ struct TableTraits;
struct TypeDescriptorKeyPrefix {
public:
// Query by type name only
explicit TypeDescriptorKeyPrefix(std::string type_name)
: type_name_(std::move(type_name)),
explicit TypeDescriptorKeyPrefix(std::string_view type_name)
: type_name_(type_name),
normalized_prefix_(normalize_type_name(type_name_) + ".") {}

// Query by type name + optional extension name
TypeDescriptorKeyPrefix(std::string type_name, std::string extension_name)
: type_name_(std::move(type_name)),
extension_name_(std::move(extension_name)),
TypeDescriptorKeyPrefix(std::string_view type_name,
std::string_view extension_name)
: type_name_(type_name),
extension_name_(extension_name),
normalized_prefix_(
normalize_type_name(type_name_) + "." +
(extension_name_.empty()
Expand Down
34 changes: 20 additions & 14 deletions villagesql/schema/systable/helpers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <cctype>
#include <initializer_list>
#include <string>
#include <string_view>
#include "lex_string.h"
#include "mysql/strings/m_ctype.h"
#include "sql/field.h"
Expand Down Expand Up @@ -167,43 +168,48 @@ const CHARSET_INFO *get_identifier_charset() {
return &my_charset_utf8mb4_0900_ai_ci; // Case-insensitive
}

std::string normalize_database_name(const std::string &name) {
static std::string casedn_string(const CHARSET_INFO *cs, std::string_view name) {

@villagestevers villagestevers Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not what we want to do. I am realizing now that the downstream have to be strings. Notice this just wraps casedn and makes a string. The reason is that casedn needs to be able to modify the underlying string (possibly even resizing it - though that really shouldn't be happening for us, as the charset should already be in the utf8_mb4 family). This makes me now doubt that we even should do this change.

if (name.empty()) return {};
return casedn(cs, std::string(name));
}

std::string normalize_database_name(std::string_view name) {
if (::lower_case_table_names == 0) {
return name; // Case-sensitive, store as-is
return std::string(name); // Case-sensitive, store as-is
}
// Case-insensitive: normalize to lowercase
return casedn(get_identifier_charset(), name);
return casedn_string(get_identifier_charset(), name);
}

std::string normalize_table_name(const std::string &name) {
std::string normalize_table_name(std::string_view name) {
if (::lower_case_table_names == 0) {
return name; // Case-sensitive, store as-is
return std::string(name); // Case-sensitive, store as-is
}
// Case-insensitive: normalize to lowercase
return casedn(get_identifier_charset(), name);
return casedn_string(get_identifier_charset(), name);
}

std::string normalize_column_name(const std::string &name) {
std::string normalize_column_name(std::string_view name) {
// Column names are always case-insensitive in MySQL
return casedn(&my_charset_utf8mb4_0900_ai_ci, name);
return casedn_string(&my_charset_utf8mb4_0900_ai_ci, name);
}

std::string normalize_extension_name(const std::string &name) {
std::string normalize_extension_name(std::string_view name) {
// Extension names are in system character set.
// TODO(villagesql-beta): Check and replace all other hard coded
// my_charset_utf8mb4_0900_ai_ci in this file.
return casedn(system_charset_info, name);
return casedn_string(system_charset_info, name);
}

std::string normalize_type_name(const std::string &name) {
std::string normalize_type_name(std::string_view name) {
// Type names should be case-insensitive (like SQL type names)
return casedn(&my_charset_utf8mb4_0900_ai_ci, name);
return casedn_string(&my_charset_utf8mb4_0900_ai_ci, name);
}

std::string normalize_index_name(const std::string &name) {
std::string normalize_index_name(std::string_view name) {
// Index names are always case-insensitive in MySQL, regardless of
// lower_case_table_names (which only governs table/database identifiers).
return casedn(&my_charset_utf8mb4_0900_ai_ci, name);
return casedn_string(&my_charset_utf8mb4_0900_ai_ci, name);
}

// ===== Test utilities =====
Expand Down
12 changes: 6 additions & 6 deletions villagesql/schema/systable/helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,22 @@ const CHARSET_INFO *get_identifier_charset();

// Normalize different identifier types according to MySQL rules
// Database names: Follow lower_case_table_names setting (like MySQL DD)
std::string normalize_database_name(const std::string &name);
std::string normalize_database_name(std::string_view name);

// Table names: Follow lower_case_table_names setting (like MySQL DD)
std::string normalize_table_name(const std::string &name);
std::string normalize_table_name(std::string_view name);

// Column names: Always case-insensitive (per MySQL standard)
std::string normalize_column_name(const std::string &name);
std::string normalize_column_name(std::string_view name);

// Extension names: Always case-insensitive (like plugin names)
std::string normalize_extension_name(const std::string &name);
std::string normalize_extension_name(std::string_view name);

// Type names: Always case-insensitive (like SQL type names)
std::string normalize_type_name(const std::string &name);
std::string normalize_type_name(std::string_view name);

// Index names: Follow lower_case_table_names setting (same as table names)
std::string normalize_index_name(const std::string &name);
std::string normalize_index_name(std::string_view name);

// Build a qualified base name string "extension_name.type_name".
inline std::string make_qualified_base_name(std::string_view extension_name,
Expand Down
54 changes: 28 additions & 26 deletions villagesql/sql/metadata_modifier.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@

#include "villagesql/sql/metadata_modifier.h"

#include <string_view>
#include <unordered_set>
#include <utility>

#include "lex_string.h"
#include "sql/create_field.h"
#include "sql/field.h"
#include "sql/field_common_properties.h"
Expand Down Expand Up @@ -138,16 +140,19 @@ template <typename EntryType, typename Map, typename Prefix>
static const EntryType *resolve_unique_descriptor(const Map &map,
const Prefix &prefix,
const char *entity_type,
const char *name,
std::string_view name,
const char *qualify_hint) {
auto matches = map.get_prefix_committed(prefix);
if (matches.empty()) {
villagesql_error("Unknown %s '%s'", MYF(0), entity_type, name);
const std::string name_for_error(name);
villagesql_error("Unknown %s '%s'", MYF(0), entity_type,

@villagestevers villagestevers Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use %.*s to use name directly with its length:

static_cast(name.length()), name.data() instead of name_for_error creation and .c_str()

name_for_error.c_str());
return nullptr;
}
if (matches.size() > 1) {
const std::string name_for_error(name);
villagesql_error("%s '%s' is ambiguous; qualify as %s", MYF(0), entity_type,
name, qualify_hint);
name_for_error.c_str(), qualify_hint);

@villagestevers villagestevers Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

return nullptr;
}
return matches[0];
Expand All @@ -157,14 +162,11 @@ static const EntryType *resolve_unique_descriptor(const Map &map,
// sets out_* on success. Returns true and sets a villagesql_error if the column
// type cannot be determined or no default profile is registered for the pair.
// REQUIRES: Caller must hold vclient read lock.
// TODO(villagesql): string parameters here (and normalize_type_name /
// normalize_extension_name) should use std::string_view; requires updating the
// downstream helpers consistently.
static bool find_default_profile(VictionaryClient &vclient,
const Alter_info *alter_info, const char *db,
const char *table_name, const char *field_name,
const std::string &index_type_name,
const std::string &index_type_ext_name,
std::string_view index_type_name,
std::string_view index_type_ext_name,
std::string &out_profile_name,
std::string &out_prof_ext_name,
std::string &out_prof_ext_version) {
Expand Down Expand Up @@ -241,11 +243,13 @@ static bool find_default_profile(VictionaryClient &vclient,
out_prof_ext_version = pd->extension_version();
return false;
}
const std::string index_type_name_for_error(index_type_name);
const std::string index_type_ext_name_for_error(index_type_ext_name);
villagesql_error(
"No default index profile found for type '%s' (extension '%s') with"
" index type '%s' (extension '%s')",
MYF(0), col_type_name.c_str(), col_ext_name.c_str(),
index_type_name.c_str(), index_type_ext_name.c_str());
index_type_name_for_error.c_str(), index_type_ext_name_for_error.c_str());

@villagestevers villagestevers Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

return true;
}

Expand All @@ -262,12 +266,10 @@ bool Metadata_modifier::add_indexes(THD *thd [[maybe_unused]], const char *db,

const KEY_CREATE_INFO &kci = key->key_create_info;

std::string ext_name = kci.custom_index_extension.str
? std::string(kci.custom_index_extension.str,
kci.custom_index_extension.length)
: "";
const std::string type_name(kci.custom_index_type.str,
kci.custom_index_type.length);
std::string_view ext_name = kci.custom_index_extension.str

@villagestevers villagestevers Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just set this to to_string_view(kci.custom_index_extension)? it should be set to length 0 if not present.

? to_string_view(kci.custom_index_extension)
: std::string_view{};
const std::string_view type_name = to_string_view(kci.custom_index_type);
const std::string index_name(key->name.str, key->name.length);
const std::string params_json =
(kci.custom_index_params && !kci.custom_index_params->empty())
Expand All @@ -281,15 +283,15 @@ bool Metadata_modifier::add_indexes(THD *thd [[maybe_unused]], const char *db,
const auto *desc = resolve_unique_descriptor<IndexTypeDescriptor>(
vclient.index_type_descriptors(),
IndexTypeDescriptorKeyPrefix(type_name, ext_name),
"custom index type", type_name.c_str(), "'extension.type_name'");
"custom index type", type_name, "'extension.type_name'");
if (!desc) return true;
ext_name = desc->extension_name();
ext_version = desc->extension_version();

const uint64_t index_id = vclient.allocate_index_id();
to_add_indexes_.emplace_back(IndexKey(db, table_name, index_name),
index_id, ext_name, ext_version, type_name,
params_json);
index_id, std::string(ext_name), ext_version,
std::string(type_name), params_json);

uint32_t key_pos = 0;
for (const Key_part_spec *kp : key->columns) {
Expand All @@ -299,16 +301,16 @@ bool Metadata_modifier::add_indexes(THD *thd [[maybe_unused]], const char *db,
std::string profile_name, prof_ext_name, prof_ext_version;
if (kp->has_index_profile()) {
LEX_CSTRING prof = kp->get_index_profile();
profile_name = std::string(prof.str, prof.length);
const std::string prof_extension =
std::string_view prof_name = to_string_view(prof);

@villagestevers villagestevers Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why introduce prof_name? if we have a string anyway, we can use that where string_view is being passed it somewhere.

profile_name = std::string(prof_name);
const std::string_view prof_extension =
kp->has_index_profile_extension()
? std::string(kp->get_index_profile_extension().str,
kp->get_index_profile_extension().length)
: "";
? to_string_view(kp->get_index_profile_extension())

@villagestevers villagestevers Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should similarly be initialized properly. I don't like that it is initialized with {} but that should be equivalent to {nullptr, 0}. So it feels like the original call to has_index_profile_extension is probably extraneous.

: std::string_view{};
const auto *prof_desc =
resolve_unique_descriptor<IndexProfileDescriptor>(
vclient.index_profile_descriptors(),
IndexProfileDescriptorKeyPrefix(profile_name, prof_extension),
IndexProfileDescriptorKeyPrefix(prof_name, prof_extension),
"index profile", profile_name.c_str(),
"'extension.profile_name'");
if (!prof_desc) return true;
Expand Down Expand Up @@ -1313,8 +1315,8 @@ void Metadata_modifier::rollback(THD *thd) {
}

bool Metadata_modifier::lock_extension_exclusive(
THD *thd, const std::string &extension_name, enum_mdl_duration duration) {
ExtensionKey ext_key(extension_name);
THD *thd, std::string_view extension_name, enum_mdl_duration duration) {
ExtensionKey ext_key{std::string(extension_name)};

@villagestevers villagestevers Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like we could do better here (but not in the scope of this change). Since the Key is essentially short-lived, we could just use string_views internally (though it might be tricky if we need them to own their memory in some scenarios). Maybe add a TODO(villagesql): comment here like:

// TODO(villagesql): consider supporting key "views" that use string_views instead of owning their strings.

const std::string &normalized_name = ext_key.str();

MDL_request mdl_request;
Expand Down
4 changes: 2 additions & 2 deletions villagesql/sql/metadata_modifier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#define VILLAGESQL_SQL_METADATA_MODIFIER_H_

#include <string>
#include <string_view>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -149,8 +150,7 @@ class Metadata_modifier {
// Acquire an X (exclusive) MDL lock on an extension with the specified
// duration. Normalizes the extension name before acquiring the lock.
// Returns false on success, true on error.
static bool lock_extension_exclusive(THD *thd,
const std::string &extension_name,
static bool lock_extension_exclusive(THD *thd, std::string_view extension_name,
enum_mdl_duration duration);

// Returns true if there are custom column or index entries to process.
Expand Down
Loading