-
Notifications
You must be signed in to change notification settings - Fork 40
feat: Refresh method support for table #152
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.
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
base: main
Are you sure you want to change the base?
Conversation
friend class Table; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
friend class Table; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this, right?
test/in_memory_catalog_test.cc
Outdated
ASSERT_TRUE(table.value()->current_snapshot().has_value()); | ||
ASSERT_EQ(table.value()->current_snapshot().value()->snapshot_id, 3055729675574597004); | ||
|
||
// Now we don't support commit method in catalog, so here only test refresh with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add a test/mock_catalog.h
to use gmock to write a MockCatalog
so that we can inject table metadata at any time by mocking the call of Catalog::LoadTable
. In the future, we may need to mock the catalog a lot of times, so I think it deserves to be added.
8e29a16
to
323aebe
Compare
src/iceberg/table.cc
Outdated
|
||
namespace iceberg { | ||
|
||
const std::string& Table::uuid() const { return metadata_->table_uuid; } | ||
|
||
Status Table::Refresh() { | ||
if (!catalog_) { | ||
return InvalidArgument("Refresh is not supported for table without a catalog"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return InvalidArgument("Refresh is not supported for table without a catalog"); | |
return NotSupported("Refresh is not supported for table without a catalog"); |
@@ -93,6 +93,10 @@ class ICEBERG_EXPORT InMemoryCatalog | |||
std::unique_ptr<iceberg::TableBuilder> BuildTable(const TableIdentifier& identifier, | |||
const Schema& schema) const override; | |||
|
|||
protected: | |||
Status UpdateTableMetaLocationInternal(const TableIdentifier& identifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this function. We need to use GMock to update the location without hacking any API.
|
||
namespace iceberg { | ||
|
||
class ICEBERG_EXPORT MockInMemoryCatalog : public InMemoryCatalog { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at https://google.github.io/googletest/gmock_for_dummies.html#writing-the-mock-class to use MOCK_METHOD
to override functions of Catalog. Then you can use EXPECT_CALL
with WillOnce
of MockCatalog::LoadTable
to mock different returns for each time.
No description provided.