Symbol Token Write Support #165

justing-bq · 2020-10-02T19:37:50Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2020-10-06T17:16:18Z

Codecov Report

Merging #165 into master will decrease coverage by 0.12%.
The diff coverage is 55.88%.

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   74.90%   74.78%   -0.13%     
==========================================
  Files          25       25              
  Lines        4857     4893      +36     
==========================================
+ Hits         3638     3659      +21     
- Misses        724      734      +10     
- Partials      495      500       +5

Impacted Files	Coverage Δ
ion/symboltable.go	`56.29% <21.05%> (-3.18%)`	⬇️
ion/marshal.go	`69.39% <66.66%> (ø)`
ion/binarywriter.go	`76.00% <68.75%> (-0.08%)`	⬇️
ion/textwriter.go	`61.45% <78.94%> (+2.25%)`	⬆️
ion/reader.go	`65.00% <100.00%> (-0.35%)`	⬇️
ion/symboltoken.go	`92.45% <100.00%> (+1.34%)`	⬆️
ion/writer.go	`66.66% <100.00%> (ø)`
ion/bitstream.go	`79.46% <0.00%> (-0.39%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d00cfc...2c8bed2. Read the comment docs.

R-maan · 2020-10-07T20:30:17Z

cmd/ion-go/main.go

 			return err
 		}
 		if err := w.WriteString(internal.GitCommit); err != nil {
 			return err
 		}

+		if err := w.FieldName(ion.NewSimpleSymbolToken("build_time")); err != nil {


Shouldn't w.FieldName() happens only if ion.NewTimestampFromStr() returns no error? Condition line 93 new

Look at the old code. w.FieldName("build_time") is in both the nil err case and the non-nil err case.

R-maan · 2020-10-07T20:58:13Z

ion/skipper.go

@@ -546,44 +546,42 @@ func (t *tokenizer) skipLongStringHelper(handler commentHandler) error {
 // false indicating this is not the end of the long string, and true for consumed '
 // as we have read the closing ''' of the first long string.
 func (t *tokenizer) skipEndOfLongString(handler commentHandler) (bool, bool, error) {
-	isConsumed := false


I personally think this can be helpful. reading return false, isConsumed, err is easier to understand compared to return false, false, err

Reverted back my change.

R-maan · 2020-10-07T21:00:47Z

ion/symboltable.go

@@ -202,7 +214,7 @@ func (s *sst) String() string {
 	buf := strings.Builder{}

 	w := NewTextWriter(&buf)
-	s.WriteTo(w)
+	_ = s.WriteTo(w)


I think WriteTo() returns error.
Why not checking the error whether it's nil or not?

If this were inside a function that returns an error then I would have added the error checking. But it doesn't and I'm not gonna add an error to the return type because that would block sst from satisfying the Stringer interface.
I added this explicit _ = so that Goland would no longer flag this as a warning.

R-maan · 2020-10-07T21:08:29Z

ion/symboltable.go

@@ -281,19 +293,22 @@ func (s *bogusSST) String() string {

 	buf := strings.Builder{}
 	w := NewTextWriter(&buf)
-	w.Annotations(SymbolToken{Text: &ionSharedSymbolTableText, LocalSID: 9}, SymbolToken{Text: &bogusText, LocalSID: SymbolIDUnknown})


Warnings, ha!? Yikes

Yes all of this was just to make Goland stop flagging it.

R-maan · 2020-10-07T21:14:19Z

ion/writer.go

-
-// FieldNameSymbol sets the field name symbol for the next value written.
-// It may only be called while writing a struct.
-func (w *writer) FieldNameSymbol(val SymbolToken) error {
 	if !w.IsInStruct() {
 		w.err = &UsageError{"Writer.FieldNameSymbol", "called when not writing a struct"}


"Writer.FieldName" instead of "Writer.FieldNameSymbol"?

R-maan · 2020-10-07T21:17:49Z

ion/unmarshal.go

 		return *val, err
 	case Int64:
 		val, err := d.r.Int64Value()
+		if err != nil {
+			return nil, err
+		}
 		return *val, err
 	default:
 		return d.r.BigIntValue()


BigIntValue also returns value, error.
We can the same check as above caseses for default too

R-maan · 2020-10-07T21:31:30Z

ion/textwriter.go

-		}
-
-		if err := writeSymbol(text, w.out); err != nil {
+		if err := writeSymbol(a.toText(), w.out); err != nil {


If I read the code correctly, when we have a.Text is nil and a.LocalSID is Unknown, then toText() will return an empty string. And this empty text is sent to writeSymbol.
I think if a.Text is nil and a.LocalSID is Unknown, error should be returned.

R-maan · 2020-10-07T21:35:40Z

ion/symboltoken.go

+	return SymbolToken{Text: &text, LocalSID: SymbolIDUnknown}
+}
+
+func newSimpleSymbolTokenPtr(text string) *SymbolToken {


Personally, I think we should not have this. we have NewSimpleSymbolToken() and if its pointer is needed that should be sufficient.

I would love to not have this either but without it I would need two lines to get the address.

ie.

st := NewSimpleSymbolToken(text) &st

instead of

newSimpleSymbolTokenPtr(text)

R-maan · 2020-10-07T21:43:22Z

ion/decimal.go

@@ -274,10 +272,9 @@ func (d *Decimal) trunc() (int64, error) {
 	if err != nil {
 		return 0, err
 	}
-	d = ud


I think we need this.
ud is different from the original d. I think when we change the scale, we need to keep d with its updated version in case another operation is called on d

Consider d.checkToUpscale().
In the old code, it reassigns d with d = d.upscale(0). However I discovered that this change of state is not reflected in the calling function d.trunc() which must be why d.checkToUpscale() returns the new decimal. (Yes I'm confused by this as well since I expect a pointer receiver to change state.)

codecov-io · 2020-10-07T23:18:25Z

Codecov Report

Merging #165 into master will decrease coverage by 0.30%.
The diff coverage is 56.94%.

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   74.94%   74.64%   -0.31%     
==========================================
  Files          25       25              
  Lines        4865     4906      +41     
==========================================
+ Hits         3646     3662      +16     
- Misses        724      739      +15     
- Partials      495      505      +10

Impacted Files	Coverage Δ
ion/timestamp.go	`68.63% <ø> (ø)`
ion/unmarshal.go	`67.74% <0.00%> (-0.90%)`	⬇️
ion/writer.go	`66.66% <ø> (ø)`
ion/symboltable.go	`56.12% <23.40%> (-3.35%)`	⬇️
ion/readlocalsymboltable.go	`66.93% <50.00%> (-1.87%)`	⬇️
ion/binarywriter.go	`75.75% <65.51%> (-0.32%)`	⬇️
ion/marshal.go	`69.39% <66.66%> (ø)`
ion/textwriter.go	`60.63% <78.57%> (+1.43%)`	⬆️
ion/textutils.go	`82.78% <83.33%> (-0.08%)`	⬇️
ion/decimal.go	`75.16% <100.00%> (-0.33%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21c1a81...4411767. Read the comment docs.

zslayton · 2020-10-14T20:56:43Z

ion/binarywriter.go

+			return w.err
+		}
+	} else {
+		return &UsageError{"Writer.WriteSymbol", "invalid symbol token"}


Could you add some detail to the error message?

zslayton · 2020-10-15T13:19:18Z

ion/binarywriter.go

+	return w.writeSymbolFromID("Writer.WriteSymbol", id)
+}
+
+// WriteSymbolFromString writes a symbol value given a string.


This comment should be more explicit about how the provided string is going to be used; it expects the string to already be in the symbol table and will return an error if it isn't.

zslayton · 2020-10-15T13:24:51Z

ion/binarywriter.go

+			// Wrap text value in single quotes if the symbol's text is a symbol identifier
+			// (ie. of form $n for some integer n)
+			// This is done to distinguish from actual symbol table mappings.
+			text = fmt.Sprintf("'%v'", text)


I'm not sure I understand the behavior that's intended here. Why are we wrapping $37 in single quotes?

If I pass a SymbolToken with LocalSID=SymbolIDUnknown and Text="$37", I would expect WriteSymbol to try to resolve the text literal $37 (unmodified) in the symbol table. If I'd wanted to write SID 37, I would have set LocalSID=37 in the SymbolToken, right?

We have roundtrip tests (such as ion-tests/iontestdata/good/symbols.ion with the test case '$99') that fail without any internal logic wrapping $99 in quotes.

When the text reader reads '$99', it produces the string "$99" (note the lack of single quotes) and then creates a SymbolToken using that string value. The absence of single quotes creates confusion further downstream about whether we are dealing with an actual symbol table mapping vs a non-mapping SymbolToken with this text value.

I moved the single quote wrapping logic over to textReader::nextBeforeTypeAnnotations instead of binaryWriter::WriteSymbol and textWriter::WriteSymbol.

such as ion-tests/iontestdata/good/symbols.ion with the test case '$99'

Here's a link to this test case for anyone who's interested.

When the text reader reads '$99', it produces the string "$99" (note the lack of single quotes) and then creates a SymbolToken using that string value.

This sounds like the correct behavior to me.

The absence of single quotes creates confusion further downstream about whether we are dealing with an actual symbol table mapping vs a non-mapping SymbolToken with this text value.

This is something we need to sort out; I don't think there's any writer API where the golang string $99 should be reinterpreted as anything other than a string literal.

Can you link to the function(s) that are causing the problems? Why is the reinterpretation happening at all?

I don't remember off the top of my head where specifically to go looking, but the root cause on this one is likely me. :)

At some point back before the introduction of SymbolToken, if the binary reader hit a symbol ID it couldn't resolve (or the text reader hit an unquoted ${n}), they'd return the string ${id} instead of an error. The writers correspondingly mapped WriteSymbol("$123") to being a symbol with that ID, bypassing the symbol table. That came at the expense of being able to read/write an actual symbol whose text is ${n}, but that wasn't a thing I thought I would ever need to do (😬😅).

Now that everything at the reader/writer level is converted to SymbolTokens, any remnants of that questionable decision can and should be removed.

This makes total sense. Thanks for the added context!

Alright I reworked this so the Symbol Token's text value is the unquoted $99.
However when the text writer is writing out the symbol in textutils.go:writeSymbol(), we still need to wrap this value in single quotes.

zslayton · 2020-10-15T13:26:43Z

ion/binarywriter.go

+				return err
+			}
+		} else {
+			return &UsageError{api, "invalid field name symbol token"}


The error message should communicate why the symbol token is invalid.

zslayton · 2020-10-15T13:33:16Z

ion/binarywriter.go

-		if err == nil {
-			return id, nil
-		}
+	if id, ok := symbolIdentifier(sym); ok {


I want to understand the symbol ID interpretation behavior that we're implementing here. I'm having difficulty imagining a scenario where I'm using a binary writer and I pass it a string in the $id format ($12, $47, etc).

zslayton · 2020-10-15T13:42:34Z

ion/textwriter.go

+		// (ie. of form $n for some integer n)
+		// This is done to distinguish from actual symbol table mappings.
+		if _, ok := symbolIdentifier(text); ok {
+			text = fmt.Sprintf("'%v'", text)


Same question here RE: quote wrapping.

zslayton · 2020-10-15T13:45:56Z

ion/symboltoken.go

@@ -103,6 +112,15 @@ func symbolIdentifier(symbolText string) (int64, bool) {
 	return SymbolIDUnknown, false
 }

+// NewSimpleSymbolToken returns a Symbol Token with the given text value and undefined SID and Source.
+func NewSimpleSymbolToken(text string) SymbolToken {


This is subjective, but to me the phrase Simple doesn't communicate what this function will do. How about NewSymbolTokenFromString?

byronlin13 · 2020-10-23T21:40:10Z

ion/unmarshal.go

 		return *val, err
 	case Int64:
 		val, err := d.r.Int64Value()
+		if err != nil {
+			return nil, err
+		}
 		return *val, err
 	default:
 		return d.r.BigIntValue()


Could you update Unmarshal API to support shared symbol tables.

I was playing around marshalling/ unmarshalling roundtrip with sst and it was missing.

Update L95-97 to:

func Unmarshal(data []byte, v interface{}, ssts ...SharedSymbolTable ) error { catalog := NewCatalog(ssts...) return NewDecoder(NewReaderCat(bytes.NewReader(data), catalog)).DecodeTo(v) }

zslayton

Justin Gossett added 11 commits September 21, 2020 11:36

write support

1ec16ad

More changes

975ecbc

Update reader.go

0d46e0f

Define SymbolToken.toText()

c6b9851

Improve WriteSymbol logic

c719ae7

minor changes

be79b2b

Define newSimpleSymbolToken()

2eeb691

use newSimpleSymbolToken()

8acd4ca

rename newSimpleSymbolToken to NewSimpleSymbolToken

4dae54e

Define and use newSimpleSymbolTokenPtr

7092c4f

cmd/ion-go updates

2ce8d31

justing-bq mentioned this pull request Oct 2, 2020

Symbol Token Write Support amazon-ion/ion-hash-go#68

Merged

Minor improvements

2c8bed2

Justin Gossett added 2 commits October 6, 2020 11:37

Housekeeping

d33379a

Update textwriter.go

bfce4f8

R-maan reviewed Oct 7, 2020

View reviewed changes

Address some feedback

d3a3971

Justin Gossett added 2 commits October 7, 2020 16:21

Update symboltable.go

9c219f7

Rename ParseTimestampFromStr to ParseTimestamp

58bf6b7

zslayton reviewed Oct 15, 2020

View reviewed changes

Justin Gossett and others added 4 commits October 15, 2020 10:16

Rename NewSimpleSymbolToken to NewSymbolTokenFromString

143d1c3

Merge branch 'master' into symbolTokenWriteSupport

cd80266

Address review feedback

bec27ab

Address some more feedback

cee40f8

justing-bq mentioned this pull request Oct 16, 2020

Update docs to reflect Symbol Token write support changes amazon-ion/ion-docs#136

Merged

Justin Gossett and others added 2 commits October 20, 2020 12:06

more changes

cc57047

Merge branch 'master' into symbolTokenWriteSupport

4411767

Justin Gossett added 2 commits October 20, 2020 13:48

Address review feedback

14f7569

Define MustParseTimestamp()

2650e9b

byronlin13 reviewed Oct 23, 2020

View reviewed changes

Justin Gossett added 4 commits October 29, 2020 11:12

Update unmarshal.go

0fd2764

Added TestRoundtripMarshalUnmarshal

3b35693

Removed some unnecessary code

4d384ce

Rename to TestRoundtripMarshalUnmarshalWithSymbolTable

62bd9d4

zslayton approved these changes Nov 9, 2020

View reviewed changes

zslayton merged commit c1fd17e into amazon-ion:master Nov 9, 2020

Symbol Token Write Support #165

Symbol Token Write Support #165

Conversation

justing-bq commented Oct 2, 2020

codecov-commenter commented Oct 6, 2020 • edited Loading

Codecov Report

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

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

codecov-io commented Oct 7, 2020 • edited Loading

Codecov Report

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

justing-bq Oct 20, 2020 • edited Loading

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

zslayton left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 6, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading

justing-bq Oct 20, 2020 •

edited

Loading