-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: Add ARRAY support #19
Conversation
@olavloite Can we rebase this PR so that it can be easier for reviewing? |
Certainly, it should now be rebased and without conflicts. |
case sppb.TypeCode_NUMERIC: | ||
var v spanner.NullNumeric | ||
if err := col.Decode(&v); err != nil { | ||
return err | ||
} | ||
dest[i] = v.Numeric | ||
dest[i] = v |
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.
Add if v.Valid
?
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.
Yeah, I can see the confusion here. I've add a comment for why we are assigning v
here in all cases (that is; also when v.Valid
is false). The reason that we do so for NUMERIC
and DATE
data types is that the Go sql package does not have a native type mapping for these types. That means that if we were to return nil
here, Go sql would not correctly understand what type it is. So instead we return a NullNumeric
or NullDate
in these cases.
The reason we don't do that for for example INT64
is that the Go sql package natively maps the Spanner INT64
data type to the Go int64
type, and so we should return nil
in those cases.
} else { | ||
dest[i] = v.Date.In(time.UTC) // TODO(jbd): Add note about this. | ||
} | ||
dest[i] = v |
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.
Same here?
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.
See above
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.
LGTM.
Should not be merged before #10 (or should be merged instead of #10, as they both contain the same changes).