Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 17 additions & 13 deletions src/iceberg/catalog/memory/in_memory_catalog.cc
Original file line number Diff line number Diff line change
Expand Up @@ -384,23 +384,25 @@ Result<std::vector<TableIdentifier>> InMemoryCatalog::ListTables(
}

Result<std::unique_ptr<Table>> InMemoryCatalog::CreateTable(
const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec,
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) {
[[maybe_unused]] const TableIdentifier& identifier,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove the parameters' names instead of adding [[maybe_unused]] to keep the code clear.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure your suggestion seems to be aligned with the original design. These functions overrode the pure virtual functions in Catalog.

Copy link
Contributor

@HuaHuaY HuaHuaY Nov 21, 2025

Choose a reason for hiding this comment

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

I don't get your point. Removing the names in child class don't change the function signature even though it overrides virtual function.

Result<std::unique_ptr<Table>> InMemoryCatalog::CreateTable(
    const TableIdentifier&, const Schema&, const PartitionSpec&,
    const std::string&,
    const std::unordered_map<std::string, std::string>&) {
  return NotImplemented("create table");
}

Copy link
Author

Choose a reason for hiding this comment

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

@HuaHuaY Let's compare the two options. I prefer to use [[maybe_unused]] rather than omitting the parameter name. Adding the attribute can bring better readability and maintenance when enabling parameters.

Copy link
Contributor

@HuaHuaY HuaHuaY Nov 25, 2025

Choose a reason for hiding this comment

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

According to C++ core guidelines https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rf-unused, [[maybe_unused]] is used when parameters are conditionally unused.
I think it's the responsibility of who implements this feature in the future to add the name back. If we don't need the variable, we should not give a name to it.

Copy link
Member

Choose a reason for hiding this comment

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

I've seen several patterns regarding to this:

void func_a([[may_unused]] int a);
void func_b(int /*a*/);
void func_c(int);

I think void func_b(int /*a*/) strikes a good balance.

[[maybe_unused]] const Schema& schema, [[maybe_unused]] const PartitionSpec& spec,
[[maybe_unused]] const std::string& location,
[[maybe_unused]] const std::unordered_map<std::string, std::string>& properties) {
return NotImplemented("create table");
}

Result<std::unique_ptr<Table>> InMemoryCatalog::UpdateTable(
const TableIdentifier& identifier,
const std::vector<std::unique_ptr<TableRequirement>>& requirements,
const std::vector<std::unique_ptr<TableUpdate>>& updates) {
[[maybe_unused]] const TableIdentifier& identifier,
[[maybe_unused]] const std::vector<std::unique_ptr<TableRequirement>>& requirements,
[[maybe_unused]] const std::vector<std::unique_ptr<TableUpdate>>& updates) {
return NotImplemented("update table");
}

Result<std::shared_ptr<Transaction>> InMemoryCatalog::StageCreateTable(
const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec,
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) {
[[maybe_unused]] const TableIdentifier& identifier,
[[maybe_unused]] const Schema& schema, [[maybe_unused]] const PartitionSpec& spec,
[[maybe_unused]] const std::string& location,
[[maybe_unused]] const std::unordered_map<std::string, std::string>& properties) {
return NotImplemented("stage create table");
}

Expand All @@ -409,14 +411,15 @@ Result<bool> InMemoryCatalog::TableExists(const TableIdentifier& identifier) con
return root_namespace_->TableExists(identifier);
}

Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) {
Status InMemoryCatalog::DropTable(const TableIdentifier& identifier,
[[maybe_unused]] bool purge) {
std::unique_lock lock(mutex_);
// TODO(Guotao): Delete all metadata files if purge is true.
return root_namespace_->UnregisterTable(identifier);
}

Status InMemoryCatalog::RenameTable(const TableIdentifier& from,
const TableIdentifier& to) {
Status InMemoryCatalog::RenameTable([[maybe_unused]] const TableIdentifier& from,
[[maybe_unused]] const TableIdentifier& to) {
std::unique_lock lock(mutex_);
return NotImplemented("rename table");
}
Expand Down Expand Up @@ -455,7 +458,8 @@ Result<std::shared_ptr<Table>> InMemoryCatalog::RegisterTable(
}

std::unique_ptr<Catalog::TableBuilder> InMemoryCatalog::BuildTable(
const TableIdentifier& identifier, const Schema& schema) const {
[[maybe_unused]] const TableIdentifier& identifier,
[[maybe_unused]] const Schema& schema) const {
throw IcebergError("not implemented");
}

Expand Down
28 changes: 14 additions & 14 deletions src/iceberg/catalog/rest/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ namespace iceberg::rest {

/// \brief Server-provided configuration for the catalog.
struct ICEBERG_REST_EXPORT CatalogConfig {
std::unordered_map<std::string, std::string> defaults; // required
std::unordered_map<std::string, std::string> overrides; // required
std::vector<std::string> endpoints;
std::unordered_map<std::string, std::string> defaults{}; // required
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to add {} for everything, at least not for STL containers and std::string. Please try to minimize lines of change like this.

Copy link
Author

Choose a reason for hiding this comment

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

It looks like they are necessary. The following warnings occurred when removing the initialization. Do you have any better idea?

../src/iceberg/test/rest_json_internal_test.cc:774:55: warning: missing initializer for member ‘iceberg::rest::CatalogConfig::endpoints’
../src/iceberg/test/rest_json_internal_test.cc:780:76: warning: missing initializer for member ‘iceberg::rest::CatalogConfig::overrides’

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest adding -Wno-missing-field-initializers to suppress such kind of warnings.

std::unordered_map<std::string, std::string> overrides{}; // required
std::vector<std::string> endpoints{};

/// \brief Validates the CatalogConfig.
Status Validate() const {
Expand All @@ -58,7 +58,7 @@ struct ICEBERG_REST_EXPORT ErrorModel {
std::string message; // required
std::string type; // required
uint32_t code; // required
std::vector<std::string> stack;
std::vector<std::string> stack{};

/// \brief Validates the ErrorModel.
Status Validate() const {
Expand Down Expand Up @@ -88,16 +88,16 @@ struct ICEBERG_REST_EXPORT ErrorResponse {
/// \brief Request to create a namespace.
struct ICEBERG_REST_EXPORT CreateNamespaceRequest {
Namespace namespace_; // required
std::unordered_map<std::string, std::string> properties;
std::unordered_map<std::string, std::string> properties{};

/// \brief Validates the CreateNamespaceRequest.
Status Validate() const { return {}; }
};

/// \brief Update or delete namespace properties request.
struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesRequest {
std::vector<std::string> removals;
std::unordered_map<std::string, std::string> updates;
std::vector<std::string> removals{};
std::unordered_map<std::string, std::string> updates{};

/// \brief Validates the UpdateNamespacePropertiesRequest.
Status Validate() const {
Expand Down Expand Up @@ -171,7 +171,7 @@ using LoadTableResponse = LoadTableResult;
/// \brief Response body for listing namespaces.
struct ICEBERG_REST_EXPORT ListNamespacesResponse {
PageToken next_page_token;
std::vector<Namespace> namespaces;
std::vector<Namespace> namespaces{};

/// \brief Validates the ListNamespacesResponse.
Status Validate() const { return {}; }
Expand All @@ -180,7 +180,7 @@ struct ICEBERG_REST_EXPORT ListNamespacesResponse {
/// \brief Response body after creating a namespace.
struct ICEBERG_REST_EXPORT CreateNamespaceResponse {
Namespace namespace_; // required
std::unordered_map<std::string, std::string> properties;
std::unordered_map<std::string, std::string> properties{};

/// \brief Validates the CreateNamespaceResponse.
Status Validate() const { return {}; }
Expand All @@ -189,17 +189,17 @@ struct ICEBERG_REST_EXPORT CreateNamespaceResponse {
/// \brief Response body for loading namespace properties.
struct ICEBERG_REST_EXPORT GetNamespaceResponse {
Namespace namespace_; // required
std::unordered_map<std::string, std::string> properties;
std::unordered_map<std::string, std::string> properties{};

/// \brief Validates the GetNamespaceResponse.
Status Validate() const { return {}; }
};

/// \brief Response body after updating namespace properties.
struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesResponse {
std::vector<std::string> updated; // required
std::vector<std::string> removed; // required
std::vector<std::string> missing;
std::vector<std::string> updated{}; // required
std::vector<std::string> removed{}; // required
std::vector<std::string> missing{};

/// \brief Validates the UpdateNamespacePropertiesResponse.
Status Validate() const { return {}; }
Expand All @@ -208,7 +208,7 @@ struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesResponse {
/// \brief Response body for listing tables in a namespace.
struct ICEBERG_REST_EXPORT ListTablesResponse {
PageToken next_page_token;
std::vector<TableIdentifier> identifiers;
std::vector<TableIdentifier> identifiers{};

/// \brief Validates the ListTablesResponse.
Status Validate() const { return {}; }
Expand Down
6 changes: 4 additions & 2 deletions src/iceberg/expression/binder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,13 @@ Result<bool> IsBoundVisitor::Or(bool left_result, bool right_result) {
return left_result && right_result;
}

Result<bool> IsBoundVisitor::Predicate(const std::shared_ptr<BoundPredicate>& pred) {
Result<bool> IsBoundVisitor::Predicate(
[[maybe_unused]] const std::shared_ptr<BoundPredicate>& pred) {
return true;
}

Result<bool> IsBoundVisitor::Predicate(const std::shared_ptr<UnboundPredicate>& pred) {
Result<bool> IsBoundVisitor::Predicate(
[[maybe_unused]] const std::shared_ptr<UnboundPredicate>& pred) {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion src/iceberg/expression/expression.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ class ICEBERG_EXPORT Expression : public util::Formattable {
/// \brief Returns whether this expression will accept the same values as another.
/// \param other another expression
/// \return true if the expressions are equivalent
virtual bool Equals(const Expression& other) const {
virtual bool Equals([[maybe_unused]] const Expression& other) const {
// only bound predicates can be equivalent
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions src/iceberg/expression/expression_visitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,13 @@ class ICEBERG_EXPORT BoundVisitor : public ExpressionVisitor<R> {

/// \brief Visit an IS_NAN unary predicate.
/// \param term The bound term being tested
virtual Result<R> IsNaN(const std::shared_ptr<BoundTerm>& term) {
virtual Result<R> IsNaN([[maybe_unused]] const std::shared_ptr<BoundTerm>& term) {
return NotSupported("IsNaN operation is not supported by this visitor");
}

/// \brief Visit a NOT_NAN unary predicate.
/// \param term The bound term being tested
virtual Result<R> NotNaN(const std::shared_ptr<BoundTerm>& term) {
virtual Result<R> NotNaN([[maybe_unused]] const std::shared_ptr<BoundTerm>& term) {
return NotSupported("NotNaN operation is not supported by this visitor");
}

Expand Down
3 changes: 2 additions & 1 deletion src/iceberg/expression/expressions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,8 @@ std::shared_ptr<NamedReference> Expressions::Ref(std::string name) {
return ref;
}

Literal Expressions::Lit(Literal::Value value, std::shared_ptr<PrimitiveType> type) {
Literal Expressions::Lit([[maybe_unused]] Literal::Value value,
[[maybe_unused]] std::shared_ptr<PrimitiveType> type) {
throw ExpressionError("Literal creation is not implemented");
}

Expand Down
6 changes: 3 additions & 3 deletions src/iceberg/expression/literal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ Result<Literal> LiteralCaster::CastFromBinary(
switch (target_type->type_id()) {
case TypeId::kFixed: {
auto target_fixed_type = internal::checked_pointer_cast<FixedType>(target_type);
if (binary_val.size() == target_fixed_type->length()) {
if (binary_val.size() == static_cast<size_t>(target_fixed_type->length())) {
return Literal::Fixed(std::move(binary_val));
}
return InvalidArgument("Failed to cast Binary with length {} to Fixed({})",
Expand Down Expand Up @@ -505,8 +505,8 @@ bool Literal::IsAboveMax() const { return std::holds_alternative<AboveMax>(value
bool Literal::IsNull() const { return std::holds_alternative<std::monostate>(value_); }

bool Literal::IsNaN() const {
return std::holds_alternative<float>(value_) && std::isnan(std::get<float>(value_)) ||
std::holds_alternative<double>(value_) && std::isnan(std::get<double>(value_));
return (std::holds_alternative<float>(value_) && std::isnan(std::get<float>(value_))) ||
(std::holds_alternative<double>(value_) && std::isnan(std::get<double>(value_)));
}

// LiteralCaster implementation
Expand Down
9 changes: 5 additions & 4 deletions src/iceberg/file_io.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ class ICEBERG_EXPORT FileIO {
/// the length to read, e.g. S3 `GetObject` has a Range parameter.
/// \return The content of the file if the read succeeded, an error code if the read
/// failed.
virtual Result<std::string> ReadFile(const std::string& file_location,
std::optional<size_t> length) {
virtual Result<std::string> ReadFile([[maybe_unused]] const std::string& file_location,
[[maybe_unused]] std::optional<size_t> length) {
// We provide a default implementation to avoid Windows linker error LNK2019.
return NotImplemented("ReadFile not implemented");
}
Expand All @@ -62,15 +62,16 @@ class ICEBERG_EXPORT FileIO {
/// \param overwrite If true, overwrite the file if it exists. If false, fail if the
/// file exists.
/// \return void if the write succeeded, an error code if the write failed.
virtual Status WriteFile(const std::string& file_location, std::string_view content) {
virtual Status WriteFile([[maybe_unused]] const std::string& file_location,
[[maybe_unused]] std::string_view content) {
return NotImplemented("WriteFile not implemented");
}

/// \brief Delete a file at the given location.
///
/// \param file_location The location of the file to delete.
/// \return void if the delete succeeded, an error code if the delete failed.
virtual Status DeleteFile(const std::string& file_location) {
virtual Status DeleteFile([[maybe_unused]] const std::string& file_location) {
return NotImplemented("DeleteFile not implemented");
}
};
Expand Down
14 changes: 7 additions & 7 deletions src/iceberg/file_reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,23 +87,23 @@ class ReaderProperties : public ConfigBase<ReaderProperties> {
/// \brief Options for creating a reader.
struct ICEBERG_EXPORT ReaderOptions {
/// \brief The path to the file to read.
std::string path;
std::string path{};
/// \brief The total length of the file.
std::optional<size_t> length;
std::optional<size_t> length{};
/// \brief The split to read.
std::optional<Split> split;
std::optional<Split> split{};
/// \brief FileIO instance to open the file. Reader implementations should down cast it
/// to the specific FileIO implementation. By default, the `iceberg-bundle` library uses
/// `ArrowFileSystemFileIO` as the default implementation.
std::shared_ptr<class FileIO> io;
std::shared_ptr<class FileIO> io{};
/// \brief The projection schema to read from the file. This field is required.
std::shared_ptr<class Schema> projection;
std::shared_ptr<class Schema> projection{};
/// \brief The filter to apply to the data. Reader implementations may ignore this if
/// the file format does not support filtering.
std::shared_ptr<class Expression> filter;
std::shared_ptr<class Expression> filter{};
/// \brief Name mapping for schema evolution compatibility. Used when reading files
/// that may have different field names than the current schema.
std::shared_ptr<class NameMapping> name_mapping;
std::shared_ptr<class NameMapping> name_mapping{};
/// \brief Format-specific or implementation-specific properties.
std::shared_ptr<ReaderProperties> properties = ReaderProperties::default_properties();
};
Expand Down
7 changes: 4 additions & 3 deletions src/iceberg/manifest_adapter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -164,13 +164,14 @@ ManifestEntryAdapter::~ManifestEntryAdapter() {
Status ManifestEntryAdapter::AppendPartitionValues(
ArrowArray* array, const std::shared_ptr<StructType>& partition_type,
const std::vector<Literal>& partition_values) {
if (array->n_children != partition_type->fields().size()) [[unlikely]] {
auto fields = partition_type->fields();
auto num_fields = static_cast<int64_t>(fields.size());
if (array->n_children != num_fields) [[unlikely]] {
return InvalidArrowData("Arrow array of partition does not match partition type.");
}
if (partition_values.size() != partition_type->fields().size()) [[unlikely]] {
if (partition_values.size() != fields.size()) [[unlikely]] {
return InvalidArrowData("Literal list of partition does not match partition type.");
}
auto fields = partition_type->fields();

for (size_t i = 0; i < fields.size(); i++) {
const auto& partition_value = partition_values[i];
Expand Down
8 changes: 5 additions & 3 deletions src/iceberg/manifest_reader_internal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ Result<std::vector<ManifestFile>> ParseManifestList(ArrowSchema* schema,
return InvalidManifestList("Columns size not match between schema:{} and array:{}",
schema->n_children, array_in->n_children);
}
if (iceberg_schema.fields().size() != array_in->n_children) {
if (iceberg_schema.fields().size() != static_cast<size_t>(array_in->n_children)) {
return InvalidManifestList("Columns size not match between schema:{} and array:{}",
iceberg_schema.fields().size(), array_in->n_children);
}
Expand Down Expand Up @@ -333,7 +333,8 @@ Status ParseDataFile(const std::shared_ptr<StructType>& data_file_schema,
if (view_of_column->storage_type != ArrowType::NANOARROW_TYPE_STRUCT) {
return InvalidManifest("DataFile field should be a struct.");
}
if (view_of_column->n_children != data_file_schema->fields().size()) {
if (static_cast<size_t>(view_of_column->n_children) !=
data_file_schema->fields().size()) {
return InvalidManifest("DataFile schema size:{} not match with ArrayArray columns:{}",
data_file_schema->fields().size(), view_of_column->n_children);
}
Expand Down Expand Up @@ -459,7 +460,8 @@ Result<std::vector<ManifestEntry>> ParseManifestEntry(ArrowSchema* schema,
return InvalidManifest("Columns size not match between schema:{} and array:{}",
schema->n_children, array_in->n_children);
}
if (iceberg_schema.fields().size() != array_in->n_children) {
if (array_in->n_children < 0 ||
iceberg_schema.fields().size() != static_cast<size_t>(array_in->n_children)) {
return InvalidManifest("Columns size not match between schema:{} and array:{}",
iceberg_schema.fields().size(), array_in->n_children);
}
Expand Down
2 changes: 1 addition & 1 deletion src/iceberg/name_mapping.cc
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ class CreateMappingVisitor {
}

template <typename T>
Result<std::unique_ptr<MappedFields>> Visit(const T& type) const {
Result<std::unique_ptr<MappedFields>> Visit([[maybe_unused]] const T& type) const {
return nullptr;
}

Expand Down
2 changes: 1 addition & 1 deletion src/iceberg/name_mapping.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ struct ICEBERG_EXPORT MappedField {
std::optional<int32_t> field_id;
/// \brief An optional list of field mappings for child field of structs, maps, and
/// lists.
std::shared_ptr<class MappedFields> nested_mapping;
std::shared_ptr<class MappedFields> nested_mapping{};

friend bool operator==(const MappedField& lhs, const MappedField& rhs);
};
Expand Down
11 changes: 6 additions & 5 deletions src/iceberg/schema.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ class PositionPathVisitor {
}

// Non-struct types are not supported yet, but it is not an error.
Status Visit(const ListType& type) { return {}; }
Status Visit(const MapType& type) { return {}; }
Status Visit([[maybe_unused]] const ListType& type) { return {}; }
Status Visit([[maybe_unused]] const MapType& type) { return {}; }

std::unordered_map<int32_t, std::vector<size_t>> Finish() {
return std::move(position_path_);
Expand Down Expand Up @@ -214,7 +214,7 @@ IdToFieldVisitor::IdToFieldVisitor(
std::unordered_map<int32_t, std::reference_wrapper<const SchemaField>>& id_to_field)
: id_to_field_(id_to_field) {}

Status IdToFieldVisitor::Visit(const PrimitiveType& type) { return {}; }
Status IdToFieldVisitor::Visit([[maybe_unused]] const PrimitiveType& type) { return {}; }

Status IdToFieldVisitor::Visit(const NestedType& type) {
const auto& nested = internal::checked_cast<const NestedType&>(type);
Expand Down Expand Up @@ -300,8 +300,9 @@ Status NameToIdVisitor::Visit(const StructType& type, const std::string& path,
return {};
}

Status NameToIdVisitor::Visit(const PrimitiveType& type, const std::string& path,
const std::string& short_path) {
Status NameToIdVisitor::Visit([[maybe_unused]] const PrimitiveType& type,
[[maybe_unused]] const std::string& path,
[[maybe_unused]] const std::string& short_path) {
return {};
}

Expand Down
1 change: 1 addition & 0 deletions src/iceberg/schema.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ class ICEBERG_EXPORT Schema : public StructType {
friend bool operator==(const Schema& lhs, const Schema& rhs) { return lhs.Equals(rhs); }

private:
using Type::Equals;
Copy link
Contributor

@HuaHuaY HuaHuaY Nov 20, 2025

Choose a reason for hiding this comment

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

@wgtmac It seems that we should override the Equals methods in Schema instead of adding Schema::Equals(const Schema& other) const at line 108?

Copy link
Author

Choose a reason for hiding this comment

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

@lidavidm Did you intend to introduce non-overriding function Equals in this commit ebacc54?

Copy link
Author

Choose a reason for hiding this comment

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

Let me keep this change because this commit is to refactor the code.

Copy link
Contributor

@HuaHuaY HuaHuaY Nov 25, 2025

Choose a reason for hiding this comment

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

I didn't make it clear. This and the previous comments are just to remind wgtmac. You don't need to modify these.

/// \brief Compare two schemas for equality.
bool Equals(const Schema& other) const;

Expand Down
Loading
Loading