adding ndjson format #218

jose-sherpa · 2023-09-06T13:10:54Z

While the omniparser tool outputs JSON format currently, you will often need another tool or package to stream the JSON output. While I am aware this tool will only be used for JSON output, there is a type of JSON called NDJSON which stands for new line delimited JSON. This makes it easy to stream parse and process a JSON array with no added packages or complexity since you just read each line and parse them one by one. Since a strength of omniparser is to stream parse large files, we think it makes sense to make the output easily streamable without violating the output of JSON. It also results in a smaller file size.

http://ndjson.org/

jose-sherpa · 2023-09-06T13:11:34Z

header/header.go

@@ -15,6 +15,7 @@ type ParserSettings struct {
 	Version        string  `json:"version,omitempty"`
 	FileFormatType string  `json:"file_format_type,omitempty"`
 	Encoding       *string `json:"encoding,omitempty"`
+	NDJSON         bool    `json:"ndjson,omitempty"`


Not sure if this is the type of case preferred in this project

jose-sherpa · 2023-09-06T13:12:21Z

cli/cmd/transformCmd.go

+	start := "[\n%s"
+	middle := ",\n%s"
+	end := "\n]"


Might be able to come up with better names or create a method on parser settings that returns a struct that encapsulates these variables

probably lparen and rparen, and delim might be a better naming option?

I'm fine with the current implementation you proposed. But do need to add unit tests - we tried very hard to keep coverage at 100%.

I'm thinking about adding a utility into https://github.com/jf-tech/go-corelib/tree/master/jsons which this omniparser uses extensively, that the utility is a json writer and encapsulates the functionalities you implement here. But that's a later optimization/refactoring. No need for this time. Just unittest coverage.

jose-sherpa · 2023-09-06T13:13:08Z

If accepted I will add the tests and refactor

codecov · 2023-09-06T13:15:23Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (79a540b) 100.00% compared to head (b26cb6d) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #218   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           53        53           
  Lines         3041      3041           
=========================================
  Hits          3041      3041

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jose-sherpa · 2023-09-06T19:19:02Z

Oh and just curious @jf-tech , have you thought about updating the go version? From my experience it's completely backwards compatible and has had some good improvements on the performance side and new methods

jf-tech · 2023-09-07T21:54:57Z

@jose-sherpa

if the desired effect is just to change the output from the command line tool, not the actual output format (which is currently standard JSON) from the Transform.Read() interface, then I would be against putting the setting into the schema. It seems to be more of a command line option that would be the better place for the switch. What do you think?

Yes, maybe in the future, we actually want to introduce different output formats for the transform interface, offering callers the choice of the current JSON, and maybe protobuf, or XML, or something else. At that time, we need to introduce the proper ParserSettings option and an extensible and pluggable mechanism for hooking up various writers. But doesn't seem to be the case in your request.
As for go version, we pinned our go version to 1.14 against the underlying goja (the javascript engine version) at that time because that was the version we thoroughly tested against. Now most likely upgrading to a higher version is fine, but that might unknowingly break other dependent customers who might still be running on a lower go version. So our philosophy is unless we start to use some go features only available in a higher version, we will stick with the current one, well, until sometime in the future it's too antiquated or Google simply stops supporting it.

Thoughts? And again, thanks for chiming in and contributing!! Love it!

jose-sherpa · 2023-09-08T18:18:39Z

@jose-sherpa

if the desired effect is just to change the output from the command line tool, not the actual output format (which is currently standard JSON) from the Transform.Read() interface, then I would be against putting the setting into the schema. It seems to be more of a command line option that would be the better place for the switch. What do you think?
Yes, maybe in the future, we actually want to introduce different output formats for the transform interface, offering callers the choice of the current JSON, and maybe protobuf, or XML, or something else. At that time, we need to introduce the proper ParserSettings option and an extensible and pluggable mechanism for hooking up various writers. But doesn't seem to be the case in your request.

As for go version, we pinned our go version to 1.14 against the underlying goja (the javascript engine version) at that time because that was the version we thoroughly tested against. Now most likely upgrading to a higher version is fine, but that might unknowingly break other dependent customers who might still be running on a lower go version. So our philosophy is unless we start to use some go features only available in a higher version, we will stick with the current one, well, until sometime in the future it's too antiquated or Google simply stops supporting it.

Thoughts? And again, thanks for chiming in and contributing!! Love it!

Thanks for the prompt reply! I think it makes sense to have it as a command line option, I'll make that change!

jose-sherpa · 2023-09-11T18:01:16Z

cli/cmd/transformCmd.go

@@ -39,6 +40,8 @@ func init() {

 	transformCmd.Flags().StringVarP(
 		&input, "input", "i", "", "input file (optional; if not specified, stdin/pipe is used)")
+	transformCmd.Flags().BoolVarP(
+		&ndjson, "ndjson", "", false, "change the output format to ndjson")


@jf-tech is this what you meant with a command line option or did you want something like --format?

how about changing it to a long flag only --stream. By default or not specified, it's false. The flag doesn't have a short form, only the --stream long form.

jf-tech

overall looking good. thanks for contributing. please do add unittest coverage.

jf-tech · 2023-09-13T05:14:19Z

cli/cmd/transformCmd.go

@@ -39,6 +40,8 @@ func init() {

 	transformCmd.Flags().StringVarP(
 		&input, "input", "i", "", "input file (optional; if not specified, stdin/pipe is used)")
+	transformCmd.Flags().BoolVarP(
+		&ndjson, "ndjson", "", false, "change the output format to ndjson")


how about changing it to a long flag only --stream. By default or not specified, it's false. The flag doesn't have a short form, only the --stream long form.

jf-tech · 2023-09-13T05:45:12Z

cli/cmd/transformCmd.go

+
+		if ndjson {
+			return string(b), nil
+		}
+
 		return strings.Join(
 			strs.NoErrMapSlice(
 				strings.Split(jsons.BPJ(string(b)), "\n"),


Look the code can be a little bit optimized here:

b = string(b) if stream { return b, nil } return strings.Join( strs.NoErrMapSlice( strings.Split(jsons.BPJ(string(b)), "\n"),

since string(b) needs to be done no matter what so do it early-on.

jf-tech · 2023-09-13T21:15:20Z

cli/cmd/transformCmd.go

+	start := "[\n%s"
+	middle := ",\n%s"
+	end := "\n]"


probably lparen and rparen, and delim might be a better naming option?

I'm fine with the current implementation you proposed. But do need to add unit tests - we tried very hard to keep coverage at 100%.

I'm thinking about adding a utility into https://github.com/jf-tech/go-corelib/tree/master/jsons which this omniparser uses extensively, that the utility is a json writer and encapsulates the functionalities you implement here. But that's a later optimization/refactoring. No need for this time. Just unittest coverage.

jose-sherpa · 2023-09-20T16:13:16Z

@jf-tech I'll make the changes and add tests, thank you!

jose-sherpa · 2023-09-21T00:36:26Z

@jf-tech I'm looking through the tests and I think this should be in extensions/omniv21/samples however when looking in there it seems to use SampleTestCommon which does it's own logic for making the json output. Is there a different test that tests through the command? Or should I make it so the output logic are in different functions that SampleTestCommon can use?

Maybe something like this will allow multiple places to use the same writing that the transform command uses:

package writer

import (
	"github.com/jf-tech/go-corelib/jsons"
	"github.com/jf-tech/go-corelib/strs"
	"github.com/jf-tech/omniparser"
	"io"
	"strings"
)

type Writer interface {
	Write(printf func(string, ...any) (int, error), println func(...any) (int, error)) error
}

func NewJSON(transform omniparser.Transform) Writer {
	return &writer{
		LParen: "[\n%s",
		Delim:  ",\n%s",
		RParen: "\n]",
		Empty:  "[]",
		DoOne: func() (string, error) {
			b, err := transform.Read()
			if err != nil {
				return "", err
			}

			return strings.Join(
				strs.NoErrMapSlice(
					strings.Split(jsons.BPJ(string(b)), "\n"),
					func(s string) string { return "\t" + s }),
				"\n"), nil
		},
	}
}

func NewNDJSON(transform omniparser.Transform) Writer {
	return &writer{
		LParen: "%s",
		Delim:  "\n%s",
		RParen: "",
		Empty:  "",
		DoOne: func() (string, error) {
			b, err := transform.Read()
			if err != nil {
				return "", err
			}

			return string(b), nil
		},
	}
}

type writer struct {
	LParen string
	Delim  string
	RParen string
	Empty  string
	DoOne  func() (string, error)
}

func (w *writer) Write(printf func(string, ...any) (int, error), println func(...any) (int, error)) error {
	record, err := w.DoOne()
	if err == io.EOF {
		_, _ = println(w.Empty)
		return nil
	}
	if err != nil {
		return err
	}

	_, _ = printf(w.LParen, record)
	for {
		record, err = w.DoOne()
		if err == io.EOF {
			break
		}
		if err != nil {
			return err
		}
		_, _ = printf(w.Delim, record)
	}
	_, _ = println(w.RParen)
	return nil
}

which reduces doTransform to:

func doTransform() error {
	schemaName := filepath.Base(schema)
	schemaReadCloser, err := openFile("schema", schema)
	if err != nil {
		return err
	}
	defer schemaReadCloser.Close()

	inputReadCloser := io.ReadCloser(nil)
	inputName := ""
	if strs.IsStrNonBlank(input) {
		inputName = filepath.Base(input)
		inputReadCloser, err = openFile("input", input)
		if err != nil {
			return err
		}
		defer inputReadCloser.Close()
	} else {
		inputName = "(stdin)"
		inputReadCloser = os.Stdin
		// Note we don't defer Close() on this since os/golang runtime owns it.
	}

	schema, err := omniparser.NewSchema(schemaName, schemaReadCloser)
	if err != nil {
		return err
	}

	transform, err := schema.NewTransform(inputName, inputReadCloser, &transformctx.Ctx{})
	if err != nil {
		return err
	}

	var w writer.Writer
	if ndjson {
		w = writer.NewNDJSON(transform)
	} else {
		w = writer.NewJSON(transform)
	}

	return w.Write(fmt.Printf, fmt.Println)
}

jf-tech · 2023-09-30T07:49:38Z

@jose-sherpa sorry for the delay. We were out/offline for several days.

First I'm not seeing the updated PR. Did you push your updates (based on my feedbacks) to origin?
Second, I just checked, the dir cli actually has no tests, because originally the purpose of it to create a quick cli tool for interactive schema authoring and a HTTP endpoint for playground (which retired last year).

So I would say don't worry about the tests for changes in this directory. Just push your latest updates to the PR and we'll take a look again and approve.

jose-sherpa · 2023-10-03T15:48:52Z

@jf-tech hey sorry I was out of town, yeah I made the changes you suggested unless you see I missed something. The only commit I did not push was the commit containing the changes I outlined in my previous comment. If you like those changes let me know and I can push that commit

jf-tech · 2023-10-04T01:04:03Z

cli/cmd/transformCmd.go

@@ -31,6 +31,7 @@ var (
 	}
 	schema string
 	input  string
+	ndjson bool


thought we said changing ndjson to stream?

Ah ok I changed the command option name but not in code, I'll make that change now

jf-tech · 2023-10-04T01:07:30Z

cli/cmd/transformCmd.go

@@ -39,6 +40,8 @@ func init() {

 	transformCmd.Flags().StringVarP(
 		&input, "input", "i", "", "input file (optional; if not specified, stdin/pipe is used)")
+	transformCmd.Flags().BoolVarP(
+		&ndjson, "stream", "", false, "change the output format to ndjson")


change the argument help display text to "if specified, each record will be a standalone/full JSON blob and printed out immediately once transform is done"

again, ndjson -> stream

jf-tech · 2023-10-04T01:08:03Z

cli/cmd/transformCmd.go

@@ -86,22 +89,42 @@ func doTransform() error {
 		if err != nil {
 			return "", err
 		}
+
+		s := string(b)
+		if ndjson {


jf-tech · 2023-10-04T01:08:11Z

cli/cmd/transformCmd.go

 				func(s string) string { return "\t" + s }),
 			"\n"), nil
 	}

 	record, err := doOne()
 	if err == io.EOF {
-		fmt.Println("[]")
+		if ndjson {


jf-tech · 2023-10-04T01:09:34Z

cli/cmd/transformCmd.go

+		if ndjson {
+			fmt.Println("")
+		} else {
+			fmt.Println("[]")
+		}


well this is just written weirdly, why not something inline with:

if !stream { println("[]") }

Println writes a new line even with a blank string, should we just leave it blank?

jf-tech · 2023-10-04T01:10:13Z

cli/cmd/transformCmd.go

+	lparen := "[\n%s"
+	delim := ",\n%s"
+	rparen := "\n]"
+	if ndjson {


jf-tech · 2023-10-05T07:34:43Z

cli/cmd/transformCmd.go

@@ -39,6 +40,8 @@ func init() {

 	transformCmd.Flags().StringVarP(
 		&input, "input", "i", "", "input file (optional; if not specified, stdin/pipe is used)")
+	transformCmd.Flags().BoolVarP(
+		&stream, "stream", "", false, "change the output format to ndjson")


i thought i had a comment on the argument help text?

adding ndjson format

455c053

jose-sherpa commented Sep 6, 2023

View reviewed changes

changing to use command line switch

188e01d

jose-sherpa commented Sep 11, 2023

View reviewed changes

jf-tech requested changes Sep 13, 2023

View reviewed changes

cr comments

8cfd180

jf-tech requested changes Oct 4, 2023

View reviewed changes

jose-sherpa added 2 commits October 4, 2023 11:41

changing bool from ndjson to stream

c362e55

no new line for empty json if streaming

bcf00e7

jf-tech requested changes Oct 5, 2023

View reviewed changes

changed help text

b26cb6d

jf-tech approved these changes Oct 9, 2023

View reviewed changes

jf-tech merged commit dd04a11 into jf-tech:master Oct 9, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding ndjson format #218

adding ndjson format #218

jose-sherpa commented Sep 6, 2023 •

edited

Loading

jose-sherpa Sep 6, 2023

jose-sherpa Sep 6, 2023

jf-tech Sep 13, 2023

jose-sherpa commented Sep 6, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading

jose-sherpa commented Sep 6, 2023

jf-tech commented Sep 7, 2023

jose-sherpa commented Sep 8, 2023

jose-sherpa Sep 11, 2023

jf-tech Sep 13, 2023

jf-tech left a comment

jf-tech Sep 13, 2023

jf-tech Sep 13, 2023

jf-tech Sep 13, 2023

jose-sherpa commented Sep 20, 2023

jose-sherpa commented Sep 21, 2023 •

edited

Loading

jf-tech commented Sep 30, 2023

jose-sherpa commented Oct 3, 2023

jf-tech Oct 4, 2023

jose-sherpa Oct 4, 2023

jf-tech Oct 4, 2023

jf-tech Oct 4, 2023

jf-tech Oct 4, 2023

jf-tech Oct 4, 2023

jf-tech Oct 4, 2023

jose-sherpa Oct 4, 2023

jf-tech Oct 4, 2023

jf-tech Oct 5, 2023

adding ndjson format #218

adding ndjson format #218

Conversation

jose-sherpa commented Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jose-sherpa commented Sep 6, 2023 • edited Loading

codecov bot commented Sep 6, 2023 • edited Loading

Codecov Report

jose-sherpa commented Sep 6, 2023

jf-tech commented Sep 7, 2023

jose-sherpa commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jf-tech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jose-sherpa commented Sep 20, 2023

jose-sherpa commented Sep 21, 2023 • edited Loading

jf-tech commented Sep 30, 2023

jose-sherpa commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jose-sherpa commented Sep 6, 2023 •

edited

Loading

jose-sherpa commented Sep 6, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading

jose-sherpa commented Sep 21, 2023 •

edited

Loading