Symbol Token Write Support #165
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,8 +21,6 @@ import ( | |
| "io" | ||
| "math" | ||
| "math/big" | ||
| "time" | ||
| ) | ||
|
|
||
|
|
@@ -251,22 +249,51 @@ func (w *binaryWriter) WriteTimestamp(val Timestamp) error { | |
| return w.writeValue("Writer.WriteTimestamp", buf) | ||
| } | ||
|
|
||
| // WriteSymbol writes a symbol value given a SymbolToken. | ||
| func (w *binaryWriter) WriteSymbol(val SymbolToken) error { | ||
| var id uint64 | ||
| if val.LocalSID != SymbolIDUnknown { | ||
| id = uint64(val.LocalSID) | ||
| } else if val.Text != nil { | ||
| text := *val.Text | ||
| if _, ok := symbolIdentifier(text); ok { | ||
| // 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) | ||
|
There was a problem hiding this comment. I'm not sure I understand the behavior that's intended here. Why are we wrapping If I pass a There was a problem hiding this comment. We have roundtrip tests (such as When the text reader reads I moved the single quote wrapping logic over to There was a problem hiding this comment.
Here's a link to this test case for anyone who's interested.
This sounds like the correct behavior to me.
This is something we need to sort out; I don't think there's any writer API where the golang string Can you link to the function(s) that are causing the problems? Why is the reinterpretation happening at all? There was a problem hiding this comment. 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 Now that everything at the reader/writer level is converted to SymbolTokens, any remnants of that questionable decision can and should be removed. There was a problem hiding this comment. This makes total sense. Thanks for the added context! There was a problem hiding this comment. Alright I reworked this so the Symbol Token's text value is the unquoted |
||
| } | ||
|
|
||
| id, w.err = w.resolveFromSymbolTable("Writer.WriteSymbol", text) | ||
| if w.err != nil { | ||
| return w.err | ||
| } | ||
| } else { | ||
| return &UsageError{"Writer.WriteSymbol", "invalid symbol token"} | ||
|
There was a problem hiding this comment. Could you add some detail to the error message? |
||
| } | ||
|
|
||
| return w.writeSymbolFromID("Writer.WriteSymbol", id) | ||
| } | ||
|
|
||
| // WriteSymbolFromString writes a symbol value given a string. | ||
|
There was a problem hiding this comment. 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. |
||
| func (w *binaryWriter) WriteSymbolFromString(val string) error { | ||
| var id uint64 | ||
| id, w.err = w.resolve("Writer.WriteSymbolFromString", val) | ||
| if w.err != nil { | ||
| return w.err | ||
| } | ||
|
|
||
| return w.writeSymbolFromID("Writer.WriteSymbolFromString", id) | ||
| } | ||
|
|
||
| func (w *binaryWriter) writeSymbolFromID(api string, id uint64) error { | ||
| vlength := uintLen(id) | ||
| bufLength := vlength + tagLen(vlength) | ||
| buf := make([]byte, 0, bufLength) | ||
|
|
||
| buf = appendTag(buf, 0x70, vlength) | ||
| buf = appendUint(buf, id) | ||
|
|
||
| return w.writeValue(api, buf) | ||
| } | ||
|
|
||
| // WriteString writes a string. | ||
|
|
@@ -492,9 +519,17 @@ func (w *binaryWriter) beginValue(api string) error { | |
| return &UsageError{api, "field name not set"} | ||
| } | ||
|
|
||
| var id uint64 | ||
| if name.LocalSID != SymbolIDUnknown { | ||
| id = uint64(name.LocalSID) | ||
| } else if name.Text != nil { | ||
| var err error | ||
| id, err = w.resolve(api, *name.Text) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| return &UsageError{api, "invalid field name symbol token"} | ||
|
There was a problem hiding this comment. The error message should communicate why the symbol token is invalid. |
||
| } | ||
|
|
||
| buf := make([]byte, 0, 10) | ||
|
|
@@ -591,13 +626,14 @@ func (w *binaryWriter) end(api string, t ctx) error { | |
|
|
||
| // Resolve resolves a symbol to its ID. | ||
| func (w *binaryWriter) resolve(api, sym string) (uint64, error) { | ||
| if id, ok := symbolIdentifier(sym); ok { | ||
|
There was a problem hiding this comment. 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 |
||
| return uint64(id), nil | ||
| } | ||
|
|
||
| return w.resolveFromSymbolTable(api, sym) | ||
| } | ||
|
|
||
| func (w *binaryWriter) resolveFromSymbolTable(api, sym string) (uint64, error) { | ||
| if w.lst != nil { | ||
| id, ok := w.lst.FindByName(sym) | ||
| if !ok { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't
w.FieldName()happens only ifion.NewTimestampFromStr()returns no error? Condition line 93 newThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the old code.
w.FieldName("build_time")is in both the nil err case and the non-nil err case.