feat: metadata access support for table #111

lishuxu · 2025-05-27T02:14:59Z

No description provided.

src/iceberg/table.cc

src/iceberg/table.h

test/table_test.cc

src/iceberg/table.h

test/table_test_helper.h

src/iceberg/table.h

src/iceberg/table.cc

src/iceberg/table.h

src/iceberg/table_scan.h

test/test_common.h

src/iceberg/table_metadata.h

src/iceberg/table.cc

test/table_test.cc

wgtmac · 2025-05-29T06:19:10Z

src/iceberg/table.h

-  /// \brief Return the schema for this table
-  virtual const std::shared_ptr<Schema>& schema() const = 0;
+  /// \brief Return the schema for this table, return NotFoundError if not found
+  virtual Result<std::shared_ptr<Schema>> schema() const = 0;


Suggested change

virtual Result<std::shared_ptr<Schema>> schema() const = 0;

virtual Result<std::shared_ptr<Schema>> Schema() const = 0;

These functions are no longer trivial accessors. We need to rename them with capitalized initial.

The method name schema will conflicts with the Schema object name, and since this is actually a lazy getter, I think it can be treated as an accessor.

wgtmac · 2025-05-29T06:19:19Z

src/iceberg/table.h

-  /// \brief Return the partition spec for this table
-  virtual const std::shared_ptr<PartitionSpec>& spec() const = 0;
+  /// \brief Return the partition spec for this table, return NotFoundError if not found
+  virtual Result<std::shared_ptr<PartitionSpec>> spec() const = 0;


Suggested change

virtual Result<std::shared_ptr<PartitionSpec>> spec() const = 0;

virtual Result<std::shared_ptr<PartitionSpec>> Spec() const = 0;

wgtmac · 2025-05-29T06:19:29Z

src/iceberg/table.h

-  /// \brief Return the sort order for this table
-  virtual const std::shared_ptr<SortOrder>& sort_order() const = 0;
+  /// \brief Return the sort order for this table, return NotFoundError if not found
+  virtual Result<std::shared_ptr<SortOrder>> sort_order() const = 0;


Suggested change

virtual Result<std::shared_ptr<SortOrder>> sort_order() const = 0;

virtual Result<std::shared_ptr<SortOrder>> SortOrder() const = 0;

wgtmac · 2025-05-29T06:19:57Z

src/iceberg/table.h

-  /// \brief Return the table's current snapshot
-  virtual const std::shared_ptr<Snapshot>& current_snapshot() const = 0;
+  /// \brief Return the table's current snapshot, or NotFoundError if not found
+  virtual Result<std::shared_ptr<Snapshot>> current_snapshot() const = 0;


Suggested change

virtual Result<std::shared_ptr<Snapshot>> current_snapshot() const = 0;

virtual Result<std::shared_ptr<Snapshot>> CurrentSnapshot() const = 0;

wgtmac

I think there are still issues to address. Please also rebase this PR on the latest main branch to resolve the conflict.

src/iceberg/table_impl.cc

wgtmac · 2025-05-30T02:57:15Z

src/iceberg/table.cc

+BaseTable::BaseTable(std::string name, std::shared_ptr<TableMetadata> metadata)
+    : name_(std::move(name)), metadata_(std::move(metadata)) {
+  if (!metadata_) {
+    throw std::invalid_argument("Table metadata cannot be null");


This is not resolved yet.

wgtmac · 2025-05-30T02:58:03Z

src/iceberg/table.cc

+void BaseTable::InitSchema() const {
+  for (const auto& schema : metadata_->schemas) {
+    if (schema->schema_id()) {
+      schemas_map_.emplace(schema->schema_id().value(), schema);


I think these changes are still required

src/iceberg/type_fwd.h

src/iceberg/snapshot.h

src/iceberg/table_metadata.h

src/iceberg/table_metadata.cc

src/iceberg/table.cc

wgtmac · 2025-06-20T05:49:47Z

src/iceberg/table.cc

+}
+
+const std::shared_ptr<PartitionSpec>& Table::spec() const {
+  std::call_once(init_partition_spec_once_, [this]() {


I'd suggest to follow similar implementation as Table::schema(). In this function, we can cache and return PartitionSpec::Unpartitioned() when missing.

src/iceberg/table.cc

wgtmac · 2025-06-20T05:51:33Z

src/iceberg/table.h

-  /// \brief Return the table's current snapshot
-  virtual const std::shared_ptr<Snapshot>& current_snapshot() const = 0;
+  /// \brief Return the table's current snapshot, return null if not found
+  std::shared_ptr<Snapshot> current_snapshot() const;


Suggested change

std::shared_ptr<Snapshot> current_snapshot() const;

const std::shared_ptr<Snapshot>& current_snapshot() const;

Every table should have at least one snapshot. I think we can safely return a reference.

src/iceberg/table.cc

wgtmac

+1

src/iceberg/table_metadata.h

src/iceberg/table.h

zhjwpku · 2025-06-23T13:49:44Z

src/iceberg/table.cc

+
+const std::shared_ptr<std::unordered_map<int32_t, std::shared_ptr<Schema>>>&
+Table::schemas() const {
+  if (!schemas_map_) {


I guess there can be race conditions here and the same pattern below, no?

zhjwpku · 2025-06-23T13:53:58Z

src/iceberg/table.h

@@ -19,13 +19,14 @@

 #pragma once

-#include <memory>
+#include <mutex>


I don't see any usage of mutex, what's the intent here?

Fokko · 2025-06-27T21:27:58Z

Let's move this in, thanks @lishuxu for working on this, and thanks @lidavidm, @zhjwpku, @wgtmac and @gty404 for the review 🚀

lishuxu closed this May 27, 2025

lishuxu reopened this May 27, 2025

gty404 reviewed May 27, 2025

View reviewed changes

src/iceberg/table.cc Outdated Show resolved Hide resolved

lidavidm reviewed May 28, 2025

View reviewed changes

src/iceberg/table.h Outdated Show resolved Hide resolved

test/table_test.cc Outdated Show resolved Hide resolved

wgtmac suggested changes May 28, 2025

View reviewed changes

src/iceberg/table.h Outdated Show resolved Hide resolved

test/table_test_helper.h Outdated Show resolved Hide resolved

src/iceberg/table.h Outdated Show resolved Hide resolved

src/iceberg/table.cc Outdated Show resolved Hide resolved

src/iceberg/table.cc Outdated Show resolved Hide resolved

lishuxu force-pushed the feature/my-feature branch from fc2ca57 to 6a26f30 Compare May 29, 2025 02:07

wgtmac suggested changes May 29, 2025

View reviewed changes

wgtmac reviewed May 30, 2025

View reviewed changes

lishuxu changed the title ~~static table metadata access support~~ feat: static table metadata access support Jun 3, 2025

lishuxu force-pushed the feature/my-feature branch 2 times, most recently from 50c93a4 to 6aef4b4 Compare June 3, 2025 02:50

shuxu.li added 13 commits June 3, 2025 11:32

feat: static table metadata access support

aa8f652

feat: static table metadata access support

7a50991

feat: static table metadata access support

0df0217

feat: static table metadata access support

f2a718d

feat: static table metadata access support

1f94b9b

feat: static table metadata access support

8c854f9

static table metadata access support

675bdd8

feat: static table metadata access support

9b10557

feat: static table metadata access support

f933198

feat: static table metadata access support

c9fe0ac

feat: static table metadata access support

a09cb8b

feat: static table metadata access support

401a019

feat: static table metadata access support

345355a

lishuxu force-pushed the feature/my-feature branch from 5304adf to 345355a Compare June 3, 2025 03:47

shuxu.li added 3 commits June 14, 2025 21:12

Merge remote-tracking branch 'upstream/main' into feature/my-feature

fc8bf0f

feat: metadata access support for table

9257726

feat: metadata access support for table

d95bd4b

lishuxu changed the title ~~feat: static table metadata access support~~ feat: metadata access support for table Jun 18, 2025

feat: metadata access support for table

515bd86

wgtmac suggested changes Jun 20, 2025

View reviewed changes

feat: metadata access support for table

43979f2

wgtmac approved these changes Jun 23, 2025

View reviewed changes

src/iceberg/table_metadata.h Outdated Show resolved Hide resolved

src/iceberg/table_metadata.h Outdated Show resolved Hide resolved

src/iceberg/table.h Outdated Show resolved Hide resolved

src/iceberg/table.h Show resolved Hide resolved

shuxu.li added 2 commits June 23, 2025 17:02

feat: metadata access support for table

3fc2374

feat: metadata access support for table

3dd5096

zhjwpku reviewed Jun 23, 2025

View reviewed changes

shuxu.li added 3 commits June 23, 2025 23:09

feat: metadata access support for table

84d82eb

feat: metadata access support for table

13c411f

feat: metadata access support for table

e6ac508

gty404 approved these changes Jun 26, 2025

View reviewed changes

Fokko approved these changes Jun 27, 2025

View reviewed changes

Fokko merged commit bfb19f6 into apache:main Jun 27, 2025
7 checks passed

	virtual Result<std::shared_ptr<Schema>> schema() const = 0;
	virtual Result<std::shared_ptr<Schema>> Schema() const = 0;

	virtual Result<std::shared_ptr<PartitionSpec>> spec() const = 0;
	virtual Result<std::shared_ptr<PartitionSpec>> Spec() const = 0;

	virtual Result<std::shared_ptr<SortOrder>> sort_order() const = 0;
	virtual Result<std::shared_ptr<SortOrder>> SortOrder() const = 0;

	virtual Result<std::shared_ptr<Snapshot>> current_snapshot() const = 0;
	virtual Result<std::shared_ptr<Snapshot>> CurrentSnapshot() const = 0;

	std::shared_ptr<Snapshot> current_snapshot() const;
	const std::shared_ptr<Snapshot>& current_snapshot() const;

feat: metadata access support for table #111

feat: metadata access support for table #111

Uh oh!

Conversation

lishuxu commented May 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko commented Jun 27, 2025

Uh oh!

Uh oh!

Uh oh!