wgtmac commented on code in PR #701:
URL: https://github.com/apache/iceberg-cpp/pull/701#discussion_r3675738807
##########
src/iceberg/table.h:
##########
@@ -29,6 +29,7 @@
#include <vector>
#include "iceberg/iceberg_export.h"
+#include "iceberg/metrics/metrics_reporter.h"
Review Comment:
Can we use forward declaration for this?
##########
src/iceberg/table.cc:
##########
@@ -219,7 +255,8 @@ Result<std::shared_ptr<UpdateLocation>>
Table::NewUpdateLocation() {
Result<std::shared_ptr<FastAppend>> Table::NewFastAppend() {
ICEBERG_ASSIGN_OR_RAISE(
auto ctx, TransactionContext::Make(shared_from_this(),
TransactionKind::kUpdate));
- return FastAppend::Make(name().name, std::move(ctx));
+ ICEBERG_ASSIGN_OR_RAISE(auto op, FastAppend::Make(name().name,
std::move(ctx)));
Review Comment:
Seems unrelated change. Let's revert it.
##########
src/iceberg/table.h:
##########
@@ -46,17 +47,25 @@ class ICEBERG_EXPORT Table : public
std::enable_shared_from_this<Table> {
/// \param[in] metadata_location The location of the table metadata file.
/// \param[in] io The FileIO to read and write table data and metadata files.
/// \param[in] catalog The catalog that this table belongs to.
- static Result<std::shared_ptr<Table>> Make(TableIdentifier identifier,
- std::shared_ptr<TableMetadata>
metadata,
- std::string metadata_location,
- std::shared_ptr<FileIO> io,
- std::shared_ptr<Catalog> catalog);
+ /// \param[in] reporter Optional metrics reporter for this table. Defaults
to nullptr
+ /// (noop).
+ static Result<std::shared_ptr<Table>> Make(
+ TableIdentifier identifier, std::shared_ptr<TableMetadata> metadata,
+ std::string metadata_location, std::shared_ptr<FileIO> io,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<MetricsReporter> reporter = nullptr);
virtual ~Table();
/// \brief Returns the identifier of this table
const TableIdentifier& name() const { return identifier_; }
+ /// \brief Returns the fully-qualified name of this table for metrics
reporting.
+ ///
+ /// Combines the owning catalog's name with the table identifier (e.g.
+ /// "catalog.namespace.table")
+ std::string FullyQualifiedName() const;
Review Comment:
Could we move this formatting out of Table and into a reusable
CatalogUtil::FullTableName helper under src/iceberg/catalog/catalog_util.h/.cc?
Java BaseTable does not derive its name: each catalog computes the final name
and passes it to BaseTable. Metastore catalogs use CatalogUtil.fullTableName,
while RESTSessionCatalog keeps its own dot-based rule. Applying the metastore
rule in Table makes it global and also produces .namespace.table when the C++
REST catalog name is empty. Please let each catalog compute and pass the final
name to Table::Make, store it as private table state, and remove
FullyQualifiedName() from the public API.
##########
src/iceberg/table.h:
##########
@@ -120,6 +129,15 @@ class ICEBERG_EXPORT Table : public
std::enable_shared_from_this<Table> {
/// \brief Returns the catalog that this table belongs to
const std::shared_ptr<Catalog>& catalog() const;
+ /// \brief Returns the metrics reporter for this table.
+ const std::shared_ptr<MetricsReporter>& reporter() const;
+
+ /// \brief Add an additional metrics reporter, combining with any existing
one.
+ ///
+ /// If a reporter is already set,
+ /// the new reporter is combined into a CompositeMetricsReporter.
+ void CombineReporter(std::shared_ptr<MetricsReporter> additional);
Review Comment:
Could we remove this public mutator? Java keeps combineMetricsReporter on
BaseTable for engine-specific integrations such as Spark, but there is no C++
caller here. This PR already combines the configured reporter and REST reporter
in MakeTableReporter, so Table::Make can receive the final reporter directly.
Removing this method also avoids exposing unsynchronized mutation of reporter_.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
