Conversation
bbengfort
left a comment
bbengfort
left a comment
There was a problem hiding this comment.
Looks great so far - and you've done a fairly complete implementation. I think this counts towards our language bindings count! Nice work!
| this.Min = count == 0 ? null : min; | ||
| this.Mean = count == 0 ? null : mean; | ||
| this.Max = count == 0 ? null : max; | ||
| this.Stddev = count == 0 ? null : stddev; |
There was a problem hiding this comment.
This at least is the one place where I prefer C# over Go … the ability to set primitive types to nil without a pointer!
There was a problem hiding this comment.
Well, you do need to declare them as nullable.
| /// <summary> | ||
| /// The UUID of the stream | ||
| /// </summary> | ||
| private readonly Guid id; |
There was a problem hiding this comment.
private readonly allows external callers to read the id but not write to it, or is it private and can only be set in the constructor? There are a few ingest related use cases, e.g. list_collections where access to the uuid is good to have. Would it be good to have an accessor, to allow public readers to access the property, e.g. UUID()?
There was a problem hiding this comment.
Oh, definitely want to have the UUID available, good catch
| /// <summary> | ||
| /// The tags for the stream, may be null | ||
| /// </summary> | ||
| private Dictionary<string, string> tags; |
There was a problem hiding this comment.
Just for my own reference, does C# allow null strings so we don't have to deal with any of that pointer business?
| throw new NotImplementedException("not implemented"); | ||
| var p = new V5Api.StreamInfoParams | ||
| { | ||
| Uuid = ByteString.CopyFrom(id.ToByteArray()), |
There was a problem hiding this comment.
This is a go programmer thing to caps the Uuid? Or is this also a C# thing?
There was a problem hiding this comment.
This is actually a property, so it should be capitalized as part of C# style. More specifically though, this property is autogenerated from protobuf
| /// <param name="end">The (exclusive) ending timestamp in nanoseconds since the Unix Epoch (Jan 1st 1970)</param> | ||
| /// <param name="version">The version of the stream to query. If unspecified, the latest version will be used</param> | ||
| /// <returns>A Cursor of RawValues that can be enumerated. <see cref="ICursor"/></returns> | ||
| public ICursor<RawPoint> RawValues(long start, long end, ulong version = BTrDB.LatestVersion) |
There was a problem hiding this comment.
I very much like this ICursor interface … I know you did extra work to get the Wait() in there and I think it's going to pay off.
There was a problem hiding this comment.
That ICursor was the result of many iterations...
| lastmaj = res.VersionMajor; | ||
| lastmin = res.VersionMinor; | ||
| } | ||
| return (lastmaj, lastmin); |
There was a problem hiding this comment.
+1 going to be nice to send any size iterable at this and have it batch for us. We may want to put suggested batch size as a constant outside the func in case we want to write concurrent inserts from PI
2 participants
Still more documentation required, probably some helper classes for inserting, and definitely more testing.
Any feedback so far?