Skip to content

Conversation

@WZhuo
Copy link
Contributor

@WZhuo WZhuo commented Dec 3, 2025

No description provided.

@WZhuo WZhuo force-pushed the memory_catalog branch 3 times, most recently from 5f931ee to 55d3900 Compare December 4, 2025 02:09
Copy link
Collaborator

@zhjwpku zhjwpku left a comment

Choose a reason for hiding this comment

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

LGTM

}

std::string_view result = path;
while (!result.ends_with("://") && result.ends_with("/")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
while (!result.ends_with("://") && result.ends_with("/")) {
while (result.ends_with("/") && !result.ends_with("://")) {

This might short circuit more quickly, not a strong opinion though.


ICEBERG_ASSIGN_OR_RAISE(auto base,
TableMetadataUtil::Read(*file_io_, metadata_location));
base->metadata_file_location = metadata_location;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
base->metadata_file_location = metadata_location;
base->metadata_file_location = std::move(metadata_location);

const std::vector<std::unique_ptr<TableRequirement>>& requirements,
const std::vector<std::unique_ptr<TableUpdate>>& updates) {
return NotImplemented("update table");
std::unique_lock lock(mutex_);
Copy link
Contributor

@HuaHuaY HuaHuaY Dec 5, 2025

Choose a reason for hiding this comment

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

Suggested change
std::unique_lock lock(mutex_);
std::lock_guard guard(mutex_);

Use std::lock_guard instead of std::unique_lock if we don't need to move the guard.

}

Status InMemoryNamespace::UpdateTableMetadataLocation(
TableIdentifier const& table_ident, std::string const& metadata_location) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TableIdentifier const& table_ident, std::string const& metadata_location) {
const TableIdentifier& table_ident, const std::string& metadata_location) {

I think it's better to keep the same style const T& as function declaration.

return TableMetadataFromJson(json);
}

Status TableMetadataUtil::Write(FileIO& io, const TableMetadata* base,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using reference instead of raw pointer?

/// \return The file extension of the codec.
static std::string CodecTypeToFileExtension(MetadataFileCodecType codec);

inline static constexpr std::string_view kTableMetadataFileSuffix = ".metadata.json";
Copy link
Contributor

Choose a reason for hiding this comment

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

static constexpr is implicitly inline and we can remove inline here.

if (auto it = metadata->properties.find(
TableProperties::kMetadataDeleteAfterCommitEnabled.key());
it != metadata->properties.end()) {
delete_after_commit = StringUtils::ToLower(it->second) == "true" || it->second == "1";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
delete_after_commit = StringUtils::ToLower(it->second) == "true" || it->second == "1";
delete_after_commit = StringUtils::EqualsIgnoreCase(it->second, "true") || it->second == "1";

StringUtils::ToLower allocates new memory and StringUtils::EqualsIgnoreCase doesn't.

Comment on lines +331 to +339
std::ranges::for_each(
base->metadata_log |
std::views::filter(
[current_files =
metadata->metadata_log |
std::ranges::to<std::unordered_set<MetadataLogEntry,
MetadataLogEntry::Hasher>>()](
const auto& entry) { return !current_files.contains(entry); }),
[&io](const auto& entry) { auto status = io.DeleteFile(entry.metadata_file); });
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::ranges::for_each(
base->metadata_log |
std::views::filter(
[current_files =
metadata->metadata_log |
std::ranges::to<std::unordered_set<MetadataLogEntry,
MetadataLogEntry::Hasher>>()](
const auto& entry) { return !current_files.contains(entry); }),
[&io](const auto& entry) { auto status = io.DeleteFile(entry.metadata_file); });
auto current_files =
metadata->metadata_log |
std::ranges::to<std::unordered_set<MetadataLogEntry, MetadataLogEntry::Hasher>>();
std::ranges::for_each(
base->metadata_log | std::views::filter([&current_files](const auto& entry) {
return !current_files.contains(entry);
}),
[&io](const auto& entry) { std::ignore = io.DeleteFile(entry.metadata_file); });

}

try {
return std::stoi(
Copy link
Contributor

Choose a reason for hiding this comment

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

If we ensure there are only numbers here, we can use std::from_chars here. It doesn't need to use substr to create a new string and is faster than std::stoi.

#include <charconv>

int result;
if (std::from_chars(metadata_location.data() + version_start,
                    metadata_location.data() + version_end, result)
        .ec == std::errc()) {
  return result;
} else {
  return -1;
}


Result<std::string> TableMetadataUtil::NewTableMetadataFilePath(const TableMetadata& meta,
int new_version) {
std::string codec_name = "none";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::string codec_name = "none";
std::string_view codec_name = "none";

TableProperties::kMetadataPreviousVersionsMax.key());
iter != impl_->metadata.properties.end()) {
try {
max_metadata_log_size = std::stoi(iter->second);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants