test: add write operations test suite by Elbehery · Pull Request #497 · apache/iceberg-go

Elbehery · 2025-07-25T08:45:48Z

This PR adds test suite for write operations.

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

tests/write_operations_test.go

laskoviymishka · 2025-07-25T11:09:53Z

tests/write_operations_test.go

+
+		// Second should succeed since conflict detection may not be fully implemented yet
+		// In a full implementation, this would fail due to conflict
+		_, err2 = tx2.Commit(s.ctx)


maybe better s.T().Skip("not yet implemented")?

so if i skip, i would skip the whole test, i think currently the comments and logs explain, wdyt ?

the only change between log and skip is that skip test is not GREEN in CI (it's yellow), so it's more honest.

laskoviymishka · 2025-07-25T11:10:24Z

tests/write_operations_test.go

+		s.writeParquet(fs, consolidatedPath, replacementData)
+
+		// For this test, we'll demonstrate overwrite by creating a new file
+		// and replacing specific known files rather than discovering them dynamically


need an assertion of this row

would you kindly suggest a change ?

i think what you can check that lists of files in snapshot differ before and after writes.

added 3500678

laskoviymishka · 2025-07-25T11:10:42Z

tests/write_operations_test.go

+		// manifests to include the delete files, which is a more complex operation
+		// that would need transaction-level support for delete file management
+
+		s.T().Log("Position delete file structure verified - integration with table scanning requires manifest updates")


s.T().Skip()

iiuc this is the end of the test .. u mean to skip this test

s.Run("ApplyPositionDeletes", func() {

?

deleted 👍🏽

laskoviymishka · 2025-07-25T11:11:23Z

tests/write_operations_test.go

+		// 3. The current scanner has error handling for equality deletes but
+		//    returns "not yet supported" for actual application
+
+		s.T().Log("Equality delete file structure verified - scanner integration shows 'not yet supported'")


same as above

ditto ? skip whole test ?

i'm think so, all other aspect in this tests already covered in tests above, and this skip would be vocal here.

deleted 👍🏽

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

zeroshade · 2025-07-25T14:31:42Z

Many of these types of tests already exist in table/table_test.go, can we move these tests there instead?

Go doesn't really have a convention of a "tests" directory like this, the tests should be in the same package as the code being tested.

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

Elbehery · 2025-07-25T14:50:17Z

Many of these types of tests already exist in table/table_test.go, can we move these tests there instead?

Go doesn't really have a convention of a "tests" directory like this, the tests should be in the same package as the code being tested.

check now 👍🏽

Elbehery · 2025-07-25T16:36:03Z

i fixed the linter issues, can u re-run plz

cc @zeroshade

Elbehery · 2025-07-25T16:38:37Z

i ran golanglint run ./... but no error locally

zeroshade · 2025-07-25T16:48:21Z

table/table_test.go

+func TestRewriteFiles(t *testing.T) {
+	suite.Run(t, &WriteOperationsTestSuite{})
+}


I'm confused, this is identical to TestWriteOptions, you don't need both of them

zeroshade · 2025-07-25T16:48:46Z

table/table_test.go

+}
+
+func (s *WriteOperationsTestSuite) TestRewriteFiles() {
+	s.Run("RewriteDataFiles", func() {


you don't need this, the function itself does this

cant get it, this is a sub-test,

which function u mean ?

When you use the test suite pattern, having a function TestRewriteDataFiles will create the subtest for you. You don't need to manually add s.Run here. Since there's no commonality between the subtests it would be better for them to be separate functions.

done 👍🏽

zeroshade · 2025-07-25T16:51:29Z

table/table_test.go

+		s.T().Log("EXPECTED: In full implementation, should only contain consolidated file")
+	})
+
+	s.Run("RewriteWithConflictDetection", func() {


make this a separate function named TestRewriteWithConflictDetection instead

done 👍🏽

zeroshade · 2025-07-25T16:52:18Z

table/table_test.go

+}
+
+func (s *WriteOperationsTestSuite) TestOverwriteFiles() {
+	s.Run("OverwriteByPartition", func() {


make a function TestOverwriteByPartition instead

done 👍🏽

zeroshade · 2025-07-25T16:52:35Z

table/table_test.go

+		s.T().Log("EXPECTED: In partition overwrite, should only contain new partition data")
+	})
+
+	s.Run("OverwriteWithFilter", func() {


same thing, make this a separate function

done 👍🏽

zeroshade · 2025-07-25T16:53:17Z

table/table_test.go

+}
+
+func (s *WriteOperationsTestSuite) TestPositionDeletes() {
+	s.Run("WritePositionDeleteFiles", func() {


you get the idea, not going to repeat this comment over and over 😄

done 👍🏽

zeroshade · 2025-07-25T16:54:30Z

table/table_test.go


-	require.NoError(t, cat.CreateNamespace(t.Context(), table.Identifier{"testing"}, nil))
-	tbl, err := cat.CreateTable(t.Context(), table.Identifier{"testing", "nullable_struct_required_field"}, sc,
+	require.NoError(t, cat.CreateNamespace(context.Background(), table.Identifier{"testing"}, nil))


make a variable ctx := context.Background() and just reuse that instead

done 👍🏽

Elbehery · 2025-07-25T16:56:32Z

@zeroshade ptal :)

zeroshade · 2025-07-25T17:00:26Z

@Elbehery see my comments :)

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

test: add write operations test suite

28ed86a

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

laskoviymishka suggested changes Jul 25, 2025

View reviewed changes

Elbehery added 2 commits July 25, 2025 14:30

test: add assert for snapshot state

7210f86

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

test: assert snapshot files before and after write

3500678

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

test: remove redundant tests

8fd8438

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

Elbehery force-pushed the 20250725_add_write_operations_test branch from e833d44 to 1bdc32e Compare July 25, 2025 16:34

zeroshade requested changes Jul 25, 2025

View reviewed changes

Elbehery force-pushed the 20250725_add_write_operations_test branch from 1bdc32e to b25c5a1 Compare July 25, 2025 16:56

test: merge write_operations_tests into table_test

a05303c

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

Elbehery force-pushed the 20250725_add_write_operations_test branch from b25c5a1 to a05303c Compare July 25, 2025 17:25

Elbehery changed the title ~~test: add write operations test suite~~ feat(test): add write operations test suite Jul 26, 2025

Elbehery changed the title ~~feat(test): add write operations test suite~~ test: add write operations test suite Jul 26, 2025

Conversation

Elbehery commented Jul 25, 2025

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!

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!

Elbehery Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zeroshade commented Jul 25, 2025

Uh oh!

Elbehery commented Jul 25, 2025

Uh oh!

Elbehery commented Jul 25, 2025

Uh oh!

Elbehery commented Jul 25, 2025

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!

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!

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!

Elbehery commented Jul 25, 2025

Uh oh!

zeroshade commented Jul 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Elbehery Jul 25, 2025 •

edited

Loading