Object statistics lru#10
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces Redis-like object “type + encoding” metadata to stored values and starts using it to implement an INCR command.
Changes:
- Add type/encoding helpers (
getType,getEncoding,assertType,assertEncoding) and a string encoding deduction helper. - Extend object creation to include a packed
TypeEncodingfield. - Add an
evalINCRhandler and integrate type/encoding deduction intoSET.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| core/typeencoding.go | Adds bitwise helpers for extracting/asserting object type and encoding. |
| core/type_string.go | Adds logic to deduce string encoding (int/embstr/raw). |
| core/store.go | Updates NewObj to accept and store packed type+encoding. |
| core/object.go | Introduces Obj with TypeEncoding and defines type/encoding constants. |
| core/eval.go | Adds INCR, updates SET to store encoding, wires command dispatch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -28,6 +30,7 @@ func NewObj(value interface{},DurationMs int64) *Obj{ | |||
|
|
|||
| return &Obj{ | |||
| Value: value, | |||
| TypeEncoding: oType|oEnc, | |||
| ExpiresAt: expiresAt, | |||
| } | |||
There was a problem hiding this comment.
NewObj sets TypeEncoding, but Obj as defined in this file does not have a TypeEncoding field, so this won’t compile. Either add TypeEncoding uint8 to the Obj struct here, or (preferably) remove this duplicate Obj definition and use a single Obj type shared with core/object.go.
| type Obj struct{ | ||
| TypeEncoding uint8 | ||
| Value interface{} | ||
| ExpiresAt int64 | ||
| } |
There was a problem hiding this comment.
Obj is defined here, but core/store.go also defines an Obj type in the same core package. Having two type Obj struct { ... } declarations in one package will not compile; consolidate into a single Obj definition (and update references accordingly).
| type Obj struct{ | |
| TypeEncoding uint8 | |
| Value interface{} | |
| ExpiresAt int64 | |
| } | |
| // Obj is defined in core/store.go for the core package. |
|
|
||
| var OBJ_ENCODING_RAW uint8=0 | ||
| var OBJ_ENCODING_INT uint8=1 | ||
| var OBJ_ENCODING_ENDSTR uint8=8 |
There was a problem hiding this comment.
OBJ_ENCODING_ENDSTR looks like a typo and doesn’t match usage elsewhere (OBJ_ENCODING_EMBSTR in deduceTypeEncoding). As written, deduceTypeEncoding won’t compile; rename/align the constant (and its value) so the encoding name is consistent across the package.
| var OBJ_ENCODING_ENDSTR uint8=8 | |
| var OBJ_ENCODING_EMBSTR uint8=8 |
| //tryObjectEncoding Function in redis | ||
| func deduceTypeEncoding(v string)(uint8,uint8){ | ||
| oType:=OBJ_TYPE_STRING | ||
| if _,rr:=strconv.ParseInt(v,10,64);err==nil{ |
There was a problem hiding this comment.
This ParseInt check won’t compile: it declares rr but then checks err (which is undefined), and rr is unused. Use the standard if _, err := strconv.ParseInt(...); err == nil { ... } pattern.
| if _,rr:=strconv.ParseInt(v,10,64);err==nil{ | |
| if _, err := strconv.ParseInt(v, 10, 64); err == nil { |
| if _,rr:=strconv.ParseInt(v,10,64);err==nil{ | ||
| return oType,OBJ_ENCODING_INT //we are converting ans stroing int as a string inside my redis object | ||
| } | ||
| if len(v)<=44{ | ||
| return oType,OBJ_ENCODING_EMBSTR //Object encoding is embedded string |
There was a problem hiding this comment.
OBJ_ENCODING_EMBSTR is referenced here but no such identifier exists in the package (the constants in core/object.go define OBJ_ENCODING_ENDSTR instead). Define OBJ_ENCODING_EMBSTR or rename usages/constants so they match.
| if _,rr:=strconv.ParseInt(v,10,64);err==nil{ | |
| return oType,OBJ_ENCODING_INT //we are converting ans stroing int as a string inside my redis object | |
| } | |
| if len(v)<=44{ | |
| return oType,OBJ_ENCODING_EMBSTR //Object encoding is embedded string | |
| if _,err:=strconv.ParseInt(v,10,64);err==nil{ | |
| return oType,OBJ_ENCODING_INT //we are converting ans stroing int as a string inside my redis object | |
| } | |
| if len(v)<=44{ | |
| return oType,OBJ_ENCODING_ENDSTR //Object encoding is embedded string |
| return Encode(err,false) | ||
| } | ||
| //which means value is indeed an integer | ||
| i,_:=strconv.ParseInt(obj.Value,(string),10,64) |
There was a problem hiding this comment.
strconv.ParseInt(obj.Value,(string),10,64) is invalid Go and won’t compile; obj.Value is interface{} so you need a type assertion/conversion to string before parsing (and handle the parse error, since stored values can still be non-numeric).
| i,_:=strconv.ParseInt(obj.Value,(string),10,64) | |
| value,ok:=obj.Value.(string) | |
| if !ok{ | |
| return Encode(errors.New("ERR value is not an integer or out of range"),false) | |
| } | |
| i,err:=strconv.ParseInt(value,10,64) | |
| if err!=nil{ | |
| return Encode(errors.New("ERR value is not an integer or out of range"),false) | |
| } |
| case "BGREWRITEAOF": | ||
| buf.Write(evalBGREWRITEAOF(cmd.Args)) | ||
| case "INCR": | ||
| buf.Write(evalPING(cmd.Args)) |
There was a problem hiding this comment.
The INCR command dispatch is wired to evalPING here, so INCR will behave like PING. This should call evalINCR(cmd.Args) instead.
| buf.Write(evalPING(cmd.Args)) | |
| buf.Write(evalINCR(cmd.Args)) |
| //TODO: Change ExpiresAt it to LRU Bits as handled by Redis | ||
| type Obj struct{ | ||
| TypeEncoding uint8 |
There was a problem hiding this comment.
The PR title mentions "Object statistics lru", but this change introduces type/encoding bits and an INCR command; the only LRU-related change is a TODO. Consider updating the PR title (or adding the intended LRU/statistics implementation) so the PR intent matches the code.
No description provided.