Skip to content
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.

Sign up for GitHub

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

Add ReadOnly iterators and refactor other iterators #329

Closed
wants to merge 1 commit into from
Closed

Add ReadOnly iterators and refactor other iterators #329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 56 additions & 31 deletions array.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2195,6 +2195,7 @@ type ArrayIterator struct {
dataSlab *ArrayDataSlab
index int
remainingCount int
readOnly bool
}

func (i *ArrayIterator) Next() (Value, error) {
Expand Down Expand Up @@ -2257,6 +2258,19 @@ func (a *Array) Iterator() (*ArrayIterator, error) {
}, nil
}

// ReadOnlyIterator returns readonly iterator for array elements.
// If elements of child containers are mutated, those changes
// are not guaranteed to persist.
func (a *Array) ReadOnlyIterator() (*ArrayIterator, error) {
Copy link
Member

@SupunS SupunS Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More of a question on use-cases: What is the advantage of using the read-only iterators over the non-read-only ones? Is there a performance gain?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good questions. From performance & API perspectives:

  • performance: non-readonly iterator needs to keep track of child mutable elements so when child elements are modified, parent container would re-inline them. So there is overhead and additional data for tracking, etc.

  • API: array iterators (readonly and non-readonly) would remain the same. However, non-readonly map iterators need additional parameters to generate hash for map key and compare map key. I think there are enough use cases for client code iterating elements without changes, so requiring these extra parameters is overkill.

Most likely, I'll postpone merging this PR until after register inlining is implemented (in a separate PR). So we'll have a chance to tweak the API if needed before this PR is merged.

iterator, err := a.Iterator()
if err != nil {
// Don't need to wrap error as external error because err is already categorized by Iterator().
return nil, err
}
iterator.readOnly = true
return iterator, nil
}

func (a *Array) RangeIterator(startIndex uint64, endIndex uint64) (*ArrayIterator, error) {
count := a.Count()

Expand Down Expand Up @@ -2307,16 +2321,18 @@ func (a *Array) RangeIterator(startIndex uint64, endIndex uint64) (*ArrayIterato
}, nil
}

type ArrayIterationFunc func(element Value) (resume bool, err error)

func (a *Array) Iterate(fn ArrayIterationFunc) error {

iterator, err := a.Iterator()
func (a *Array) ReadOnlyRangeIterator(startIndex uint64, endIndex uint64) (*ArrayIterator, error) {
iterator, err := a.RangeIterator(startIndex, endIndex)
if err != nil {
// Don't need to wrap error as external error because err is already categorized by Array.Iterator().
return err
return nil, err
}
iterator.readOnly = true
return iterator, nil
}

type ArrayIterationFunc func(element Value) (resume bool, err error)

func iterate(iterator *ArrayIterator, fn ArrayIterationFunc) error {
for {
value, err := iterator.Next()
if err != nil {
Expand All @@ -2337,33 +2353,42 @@ func (a *Array) Iterate(fn ArrayIterationFunc) error {
}
}

func (a *Array) IterateRange(startIndex uint64, endIndex uint64, fn ArrayIterationFunc) error {
func (a *Array) Iterate(fn ArrayIterationFunc) error {
iterator, err := a.Iterator()
if err != nil {
// Don't need to wrap error as external error because err is already categorized by Array.Iterator().
return err
}
return iterate(iterator, fn)
}

func (a *Array) IterateReadOnly(fn ArrayIterationFunc) error {
iterator, err := a.ReadOnlyIterator()
if err != nil {
// Don't need to wrap error as external error because err is already categorized by Array.ReadOnlyIterator().
return err
}
return iterate(iterator, fn)
}

func (a *Array) IterateRange(startIndex uint64, endIndex uint64, fn ArrayIterationFunc) error {
iterator, err := a.RangeIterator(startIndex, endIndex)
if err != nil {
// Don't need to wrap error as external error because err is already categorized by Array.RangeIterator().
return err
}
return iterate(iterator, fn)
}

for {
value, err := iterator.Next()
if err != nil {
// Don't need to wrap error as external error because err is already categorized by ArrayIterator.Next().
return err
}
if value == nil {
return nil
}
resume, err := fn(value)
if err != nil {
// Wrap err as external error (if needed) because err is returned by ArrayIterationFunc callback.
return wrapErrorAsExternalErrorIfNeeded(err)
}
if !resume {
return nil
}
func (a *Array) IterateReadOnlyRange(startIndex uint64, endIndex uint64, fn ArrayIterationFunc) error {
iterator, err := a.ReadOnlyRangeIterator(startIndex, endIndex)
if err != nil {
// Don't need to wrap error as external error because err is already categorized by Array.ReadOnlyRangeIterator().
return err
}
return iterate(iterator, fn)
}

func (a *Array) Count() uint64 {
return uint64(a.root.Header().count)
}
Expand All @@ -2390,7 +2415,7 @@ func (a *Array) Type() TypeInfo {
}

func (a *Array) String() string {
iterator, err := a.Iterator()
iterator, err := a.ReadOnlyIterator()
if err != nil {
return err.Error()
}
Expand Down Expand Up @@ -2863,8 +2888,8 @@ func (i *ArrayLoadedValueIterator) Next() (Value, error) {
return nil, nil
}

// LoadedValueIterator returns iterator to iterate loaded array elements.
func (a *Array) LoadedValueIterator() (*ArrayLoadedValueIterator, error) {
// ReadOnlyLoadedValueIterator returns iterator to iterate loaded array elements.
func (a *Array) ReadOnlyLoadedValueIterator() (*ArrayLoadedValueIterator, error) {
switch slab := a.root.(type) {

case *ArrayDataSlab:
Expand Down Expand Up @@ -2902,9 +2927,9 @@ func (a *Array) LoadedValueIterator() (*ArrayLoadedValueIterator, error) {
}
}

// IterateLoadedValues iterates loaded array values.
func (a *Array) IterateLoadedValues(fn ArrayIterationFunc) error {
iterator, err := a.LoadedValueIterator()
// IterateReadOnlyLoadedValues iterates loaded array values.
func (a *Array) IterateReadOnlyLoadedValues(fn ArrayIterationFunc) error {
iterator, err := a.ReadOnlyLoadedValueIterator()
if err != nil {
// Don't need to wrap error as external error because err is already categorized by Array.LoadedValueIterator().
return err
Expand Down
4 changes: 2 additions & 2 deletions array_bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ func benchmarkNewArrayFromAppend(b *testing.B, initialArraySize int) {
for i := 0; i < b.N; i++ {
copied, _ := NewArray(storage, array.Address(), array.Type())

_ = array.Iterate(func(value Value) (bool, error) {
_ = array.IterateReadOnly(func(value Value) (bool, error) {
_ = copied.Append(value)
return true, nil
})
Expand All @@ -379,7 +379,7 @@ func benchmarkNewArrayFromBatchData(b *testing.B, initialArraySize int) {
b.StartTimer()

for i := 0; i < b.N; i++ {
iter, err := array.Iterator()
iter, err := array.ReadOnlyIterator()
require.NoError(b, err)

copied, _ := NewArrayFromBatchData(storage, array.Address(), array.Type(), func() (Value, error) {
Expand Down
48 changes: 24 additions & 24 deletions array_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func verifyArray(

// Verify array elements by iterator
i := 0
err = array.Iterate(func(v Value) (bool, error) {
err = array.IterateReadOnly(func(v Value) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any tests that still utilize the old APIs, like Iterate, IterateRange, etc.?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, the old is the same as new API implementation until register inlining is added. So (for now) the tests for the new APIs currently cover the old functionality.

The old APIs like Iterate and IterateRange, etc. currently have an incomplete implementation regarding register inlining, so the tests for them will be done after they are implemented.

valueEqual(t, typeInfoComparator, values[i], v)
i++
return true, nil
Expand Down Expand Up @@ -644,7 +644,7 @@ func TestArrayIterate(t *testing.T) {
require.NoError(t, err)

i := uint64(0)
err = array.Iterate(func(v Value) (bool, error) {
err = array.IterateReadOnly(func(v Value) (bool, error) {
i++
return true, nil
})
Expand All @@ -671,7 +671,7 @@ func TestArrayIterate(t *testing.T) {
}

i := uint64(0)
err = array.Iterate(func(v Value) (bool, error) {
err = array.IterateReadOnly(func(v Value) (bool, error) {
require.Equal(t, Uint64Value(i), v)
i++
return true, nil
Expand Down Expand Up @@ -708,7 +708,7 @@ func TestArrayIterate(t *testing.T) {
}

i := uint64(0)
err = array.Iterate(func(v Value) (bool, error) {
err = array.IterateReadOnly(func(v Value) (bool, error) {
require.Equal(t, Uint64Value(i), v)
i++
return true, nil
Expand Down Expand Up @@ -741,7 +741,7 @@ func TestArrayIterate(t *testing.T) {
}

i := uint64(0)
err = array.Iterate(func(v Value) (bool, error) {
err = array.IterateReadOnly(func(v Value) (bool, error) {
require.Equal(t, Uint64Value(i), v)
i++
return true, nil
Expand Down Expand Up @@ -777,7 +777,7 @@ func TestArrayIterate(t *testing.T) {

i := uint64(0)
j := uint64(1)
err = array.Iterate(func(v Value) (bool, error) {
err = array.IterateReadOnly(func(v Value) (bool, error) {
require.Equal(t, Uint64Value(j), v)
i++
j += 2
Expand All @@ -803,7 +803,7 @@ func TestArrayIterate(t *testing.T) {
}

i := 0
err = array.Iterate(func(_ Value) (bool, error) {
err = array.IterateReadOnly(func(_ Value) (bool, error) {
if i == count/2 {
return false, nil
}
Expand Down Expand Up @@ -832,7 +832,7 @@ func TestArrayIterate(t *testing.T) {
testErr := errors.New("test")

i := 0
err = array.Iterate(func(_ Value) (bool, error) {
err = array.IterateReadOnly(func(_ Value) (bool, error) {
if i == count/2 {
return false, testErr
}
Expand All @@ -858,7 +858,7 @@ func testArrayIterateRange(t *testing.T, array *Array, values []Value) {
count := array.Count()

// If startIndex > count, IterateRange returns SliceOutOfBoundsError
err = array.IterateRange(count+1, count+1, func(v Value) (bool, error) {
err = array.IterateReadOnlyRange(count+1, count+1, func(v Value) (bool, error) {
i++
return true, nil
})
Expand All @@ -871,7 +871,7 @@ func testArrayIterateRange(t *testing.T, array *Array, values []Value) {
require.Equal(t, uint64(0), i)

// If endIndex > count, IterateRange returns SliceOutOfBoundsError
err = array.IterateRange(0, count+1, func(v Value) (bool, error) {
err = array.IterateReadOnlyRange(0, count+1, func(v Value) (bool, error) {
i++
return true, nil
})
Expand All @@ -883,7 +883,7 @@ func testArrayIterateRange(t *testing.T, array *Array, values []Value) {

// If startIndex > endIndex, IterateRange returns InvalidSliceIndexError
if count > 0 {
err = array.IterateRange(1, 0, func(v Value) (bool, error) {
err = array.IterateReadOnlyRange(1, 0, func(v Value) (bool, error) {
i++
return true, nil
})
Expand All @@ -898,7 +898,7 @@ func testArrayIterateRange(t *testing.T, array *Array, values []Value) {
for startIndex := uint64(0); startIndex <= count; startIndex++ {
for endIndex := startIndex; endIndex <= count; endIndex++ {
i = uint64(0)
err = array.IterateRange(startIndex, endIndex, func(v Value) (bool, error) {
err = array.IterateReadOnlyRange(startIndex, endIndex, func(v Value) (bool, error) {
valueEqual(t, typeInfoComparator, v, values[int(startIndex+i)])
i++
return true, nil
Expand Down Expand Up @@ -980,7 +980,7 @@ func TestArrayIterateRange(t *testing.T) {
startIndex := uint64(1)
endIndex := uint64(5)
count := endIndex - startIndex
err = array.IterateRange(startIndex, endIndex, func(_ Value) (bool, error) {
err = array.IterateReadOnlyRange(startIndex, endIndex, func(_ Value) (bool, error) {
if i == count/2 {
return false, nil
}
Expand Down Expand Up @@ -1009,7 +1009,7 @@ func TestArrayIterateRange(t *testing.T) {
startIndex := uint64(1)
endIndex := uint64(5)
count := endIndex - startIndex
err = array.IterateRange(startIndex, endIndex, func(_ Value) (bool, error) {
err = array.IterateReadOnlyRange(startIndex, endIndex, func(_ Value) (bool, error) {
if i == count/2 {
return false, testErr
}
Expand Down Expand Up @@ -1846,7 +1846,7 @@ func TestEmptyArray(t *testing.T) {

t.Run("iterate", func(t *testing.T) {
i := uint64(0)
err := array.Iterate(func(v Value) (bool, error) {
err := array.IterateReadOnly(func(v Value) (bool, error) {
i++
return true, nil
})
Expand Down Expand Up @@ -2088,7 +2088,7 @@ func TestArrayFromBatchData(t *testing.T) {
require.NoError(t, err)
require.Equal(t, uint64(0), array.Count())

iter, err := array.Iterator()
iter, err := array.ReadOnlyIterator()
require.NoError(t, err)

// Create a new array with new storage, new address, and original array's elements.
Expand Down Expand Up @@ -2128,7 +2128,7 @@ func TestArrayFromBatchData(t *testing.T) {

require.Equal(t, uint64(arraySize), array.Count())

iter, err := array.Iterator()
iter, err := array.ReadOnlyIterator()
require.NoError(t, err)

// Create a new array with new storage, new address, and original array's elements.
Expand Down Expand Up @@ -2172,7 +2172,7 @@ func TestArrayFromBatchData(t *testing.T) {

require.Equal(t, uint64(arraySize), array.Count())

iter, err := array.Iterator()
iter, err := array.ReadOnlyIterator()
require.NoError(t, err)

address := Address{2, 3, 4, 5, 6, 7, 8, 9}
Expand Down Expand Up @@ -2222,7 +2222,7 @@ func TestArrayFromBatchData(t *testing.T) {

require.Equal(t, uint64(36), array.Count())

iter, err := array.Iterator()
iter, err := array.ReadOnlyIterator()
require.NoError(t, err)

storage := newTestPersistentStorage(t)
Expand Down Expand Up @@ -2272,7 +2272,7 @@ func TestArrayFromBatchData(t *testing.T) {

require.Equal(t, uint64(36), array.Count())

iter, err := array.Iterator()
iter, err := array.ReadOnlyIterator()
require.NoError(t, err)

storage := newTestPersistentStorage(t)
Expand Down Expand Up @@ -2318,7 +2318,7 @@ func TestArrayFromBatchData(t *testing.T) {

require.Equal(t, uint64(arraySize), array.Count())

iter, err := array.Iterator()
iter, err := array.ReadOnlyIterator()
require.NoError(t, err)

storage := newTestPersistentStorage(t)
Expand Down Expand Up @@ -2373,7 +2373,7 @@ func TestArrayFromBatchData(t *testing.T) {
err = array.Append(v)
require.NoError(t, err)

iter, err := array.Iterator()
iter, err := array.ReadOnlyIterator()
require.NoError(t, err)

storage := newTestPersistentStorage(t)
Expand Down Expand Up @@ -2738,7 +2738,7 @@ func TestArrayLoadedValueIterator(t *testing.T) {
verifyArrayLoadedElements(t, array, values)

i := 0
err := array.IterateLoadedValues(func(v Value) (bool, error) {
err := array.IterateReadOnlyLoadedValues(func(v Value) (bool, error) {
// At this point, iterator returned first element (v).

// Remove all other nested composite elements (except first element) from storage.
Expand Down Expand Up @@ -3414,7 +3414,7 @@ func createArrayWithSimpleAndCompositeValues(

func verifyArrayLoadedElements(t *testing.T, array *Array, expectedValues []Value) {
i := 0
err := array.IterateLoadedValues(func(v Value) (bool, error) {
err := array.IterateReadOnlyLoadedValues(func(v Value) (bool, error) {
require.True(t, i < len(expectedValues))
valueEqual(t, typeInfoComparator, expectedValues[i], v)
i++
Expand Down
Loading