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.

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 ArrayIterator to iterate over array values #254

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

80avin
Copy link

@80avin 80avin commented Oct 2, 2022

Description: Fixes #253

Benchmark before change:

Benchmark after change:

For running benchmarks use:

go test -test.benchmem -bench JsonParser ./benchmark/ -benchtime 5s -v
# OR
make bench (runs inside docker)

Sample usage

package main

import (
	"fmt"
	"github.com/buger/jsonparser"
)

func main() {
	data := []byte(`{"menu": {
    "header": "SVG Viewer",
    "items": [
        {"id": "Open"},
        {"id": "OpenNew", "label": "Open New"},
        null,
        {"id": "ZoomIn", "label": "Zoom In"},
        {"id": "ZoomOut", "label": "Zoom Out"},
        {"id": "OriginalView", "label": "Original View"},
        null,
        {"id": "Quality"},
        {"id": "Pause"},
        {"id": "Mute"},
        null,
        {"id": "Find", "label": "Find..."},
        {"id": "FindAgain", "label": "Find Again"},
        {"id": "Copy"},
        {"id": "CopyAgain", "label": "Copy Again"},
        {"id": "CopySVG", "label": "Copy SVG"},
        {"id": "ViewSVG", "label": "View SVG"},
        {"id": "ViewSource", "label": "View Source"},
        {"id": "SaveAs", "label": "Save As"},
        null,
        {"id": "Help"},
        {"id": "About", "label": "About Adobe CVG Viewer..."}
    ]
}}`)
	next, err := jsonparser.ArrayIterator(data, "menu", "items")
	if err != nil {
		fmt.Println("err", err)
	}
	for v, t, o, e := next(); e == nil; {
		fmt.Println(string(v), t, o, e)
		v, t, o, e = next()
		if e != nil {
			fmt.Println("err-loop", e)
		}
	}
}

@80avin 80avin marked this pull request as draft October 2, 2022 11:29
@prateek
Copy link

prateek commented Jul 18, 2023

@buger any chance this can be merged in? i'm happy to pickup the PR and add any changes/tests/etc you want.

imo the proposed API feels a lot more ergonomic than ArrayEach.

@buger
Copy link
Owner

buger commented Jul 18, 2023

@prateek can you provide example of use-cases when using such iterator allows you to do smth more then current API?

Overall if I get convinced, I do not mind to add iterators, but better to do it consitently, and also implement it for ObjectEach, and KeyEach.

Also I wonder if there is room for refactoring here, e.g. make ArrayEach use ArrayIterator internally, will there be big performance penalty?

@prateek
Copy link

prateek commented Jul 25, 2023

@buger I don't think there's anything this API lets you do that the other doesn't. The iterator version would lead to simpler/easier to maintain code IMO. Maybe this should be a separate PR for discussion but while I have you here --

Alternative API

package jsoniter

// similar to api choice today - function, not method
func ArrayIterator(data []byte, keys ...string) (Iterator, error) { ... } 

// similarly for other iterator types (object/etc)

type Iterator interface { 
  Next() (value []byte, dataType jsonparser.ValueType, done bool)
  Err() error
  // you could simplify this into one method if you'd prefer by returning a typed error to indicate "done" instead of returning a bool.
}
// only using interface for sake of brevity, i'd imagine you'd export a concrete type - struct/function

sample usage (using @80avin's data set from above)

iter, err := jsonparser.ArrayIterator(data, "menu", "items")
if err != nil {
  // i.e. unable to construct iterator. 
}

for {
  datum, _, done := iter.Next()
  if done {
    break
    // either no more data or error
  }
}

if err := iter.Err(); err != nil {
  // handle error how you want here.
}

Things I prefer about this:

  • It doesn't expose offset details to end users.
  • The error handling logic is simpler to reason about. For e.g. today i don't know which error i'm receiving out of ArrayEach at the top-level, vs in the callback.

As to your question about refactoring - i don't know how you handle offsets internally and/or if you want to maintain a b/w compat API. probably easiest to share the internals of the iterator + arrayeach code to start.

other considerations

  • Go's got a recent proposal aimed adding first class support for range to work over non-standard types. Great blog talking about it. Would be good to work in a direction that could be integrated into that eventually.

@buger
Copy link
Owner

buger commented Jul 25, 2023

I see, seems like you have a good sense about this. I trust your view here, and lets make iterators as alternative API.
As mentioned it may require to have it for ObjectEach, KeysEach. And as mentioned it can be significantly refactored, by either making Interator methods use the "old" API internally, or both this functions use some shared helper.

Thanks!

@80avin
Copy link
Author

80avin commented Aug 9, 2024

@prateek Thanks for the suggestions.
The alternative API you proposed looks definitely better than what I originally proposed.

Looking at the range proposal, I'm finding that to be better as we can write like:

Taking inspiration from scanner example
jsoniter := jsonparser.ArrayIterator(data, "menu", "items")
if err := jsoniter.Err(); err != nil {
  fmt.Println("failed to read", err)
}

for _type, val := jsoniter.All {
  // do something
}

if err := jsoniter.Err(); err != nil {
  fmt.Println("error", err)
}

Iterators are already in 1.23 release candidates and the stable release is also planned to for this month.
However, I'm not sure if it would be fine to use such a cutting edge feature in the library, making the library probably incompatible with older go versions.

Maybe @buger you can take the decision here. I'm way too nascent in golang to decide.

@rossb83
Copy link

rossb83 commented Sep 8, 2024

Having played with this a bit the go 1.23 iter package may be able to give similar behavior; although it could get increasingly complex depending on what you wanted to iterate through. See: https://go.dev/play/p/O3bP6Y9mAjT

For example you can range over string IDs in your menu json like this

func main() {
	for id := range Response(data).IterateIDs() {
		fmt.Println(id)
	}
}

By defining an iterator

type Response []byte

func (r Response) IterateIDs() iter.Seq[string] {
	return func(yield func(string) bool) {
		jsonparser.ArrayEach(r, func(value []byte, _ jsonparser.ValueType, _ int, _ error) {
			nextID, err := jsonparser.GetUnsafeString(value, "id")
			if err != nil || !yield(nextID) {
				return
			}
		}, "menu", "items")
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Iterator support for Arrays & Objects
4 participants