Copilot commented on code in PR #876:
URL: https://github.com/apache/iceberg-cpp/pull/876#discussion_r3795799398
##########
src/iceberg/inspect/history_table.cc:
##########
@@ -26,37 +26,32 @@
#include "iceberg/schema.h"
#include "iceberg/schema_field.h"
#include "iceberg/table.h"
-#include "iceberg/table_identifier.h"
#include "iceberg/type.h"
+#include "iceberg/util/macros.h"
namespace iceberg {
-namespace {
-std::shared_ptr<Schema> MakeHistoryTableSchema() {
- return std::make_shared<Schema>(std::vector<SchemaField>{
+HistoryTable::HistoryTable(std::shared_ptr<Table> table)
+ : MetadataTable(std::move(table)) {}
+
+HistoryTable::~HistoryTable() = default;
+
+const std::shared_ptr<Schema>& HistoryTable::schema() const {
+ static const auto schema = std::make_shared<Schema>(std::vector<SchemaField>{
SchemaField::MakeRequired(1, "made_current_at", timestamp_tz()),
SchemaField::MakeRequired(2, "snapshot_id", int64()),
SchemaField::MakeOptional(3, "parent_id", int64()),
SchemaField::MakeRequired(4, "is_current_ancestor", boolean())});
+ return schema;
}
-TableIdentifier MakeHistoryTableName(const TableIdentifier& source_name) {
- return TableIdentifier{.ns = source_name.ns, .name = source_name.name +
".history"};
-}
-
-} // namespace
-
-HistoryTable::HistoryTable(std::shared_ptr<Table> table)
- : MetadataTable(table, MakeHistoryTableName(table->name()),
- MakeHistoryTableSchema()) {}
-
-HistoryTable::~HistoryTable() = default;
-
Result<std::unique_ptr<HistoryTable>>
HistoryTable::Make(std::shared_ptr<Table> table) {
- if (table == nullptr) [[unlikely]] {
- return InvalidArgument("Table cannot be null");
- }
+ ICEBERG_PRECHECK(table != nullptr, "Table cannot be null");
return std::unique_ptr<HistoryTable>(new HistoryTable(std::move(table)));
}
+Result<ArrowArrayStream> HistoryTable::Scan() {
+ return NotSupported("Scan is not supported for the history table");
+}
Review Comment:
HistoryTable is exposed as a concrete MetadataTable with a defined schema,
but Scan() unconditionally returns NotSupported. This makes the table unusable
through the public metadata-table API and is surprising for consumers expecting
system metadata tables to be scannable.
##########
src/iceberg/test/meson.build:
##########
@@ -55,6 +55,7 @@ iceberg_tests = {
'metrics_test.cc',
'snapshot_test.cc',
'snapshot_util_test.cc',
+ 'system_metadata_tables_test.cc',
'table_metadata_builder_test.cc',
'table_requirement_test.cc',
Review Comment:
Meson test target 'table_test' doesn't compile the new metadata-table unit
tests (history_table_test.cc, snapshots_table_test.cc). As a result, Meson
CI/builds won't exercise these new table implementations even though CMake does.
--
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]
