ledger: remove "rowid" from the public interface #5177
Conversation
icorderi
force-pushed
the
refactor/rowid
branch
2 times, most recently
from
f923d8e to
cfaa941
Compare
AlgoAxel
left a comment
AlgoAxel
left a comment
There was a problem hiding this comment.
Looking at your failing test I think it's just a missing assert update:
expected: int64(2)
actual : sqlitedriver.sqlRowRef(sqlitedriver.sqlRowRef{rowid:2})
Also thinking out loud about how this is implemented -- outside of unit testing, are there compilation errors if production code doesn't use the new Ref structure? Just thinking that since the Ref just an opaque interface, it might be possible to miss updates? Maybe not, hence why I'm asking out loud :)
There was a problem hiding this comment.
Just leaving the SQL NullInt64 implementation here, since this made me curious about what it looked like:
// NullInt64 represents an int64 that may be null.
// NullInt64 implements the Scanner interface so
// it can be used as a scan destination, similar to NullString.
type NullInt64 struct {
Int64 int64
Valid bool // Valid is true if Int64 is not NULL
}
Codecov Report
@@ Coverage Diff @@
## master #5177 +/- ##
==========================================
- Coverage 53.59% 53.52% -0.08%
==========================================
Files 439 439
Lines 54950 54985 +35
==========================================
- Hits 29452 29432 -20
- Misses 23218 23263 +45
- Partials 2280 2290 +10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
It's a valid point @AlgoAxel, I wasn't too happy with the auto impl of the interface for any type either. |
| // AccountRef is an opaque ref to an account in the db. | ||
| type AccountRef interface { | ||
| AccountRefMarker() | ||
| } | ||
|
|
||
| // OnlineAccountRef is an opaque ref to an "online" account in the db. | ||
| type OnlineAccountRef interface { | ||
| OnlineAccountRefMarker() | ||
| } | ||
|
|
||
| // ResourceRef is an opaque ref to a resource in the db. | ||
| type ResourceRef interface { | ||
| ResourceRefMarker() | ||
| } |
There was a problem hiding this comment.
cool idea! this will certainly help keep us from using incompatible ref types.
Tagging @cce specifically because this could be a valuable pattern in other places and I'd like to highlight it. Specifically, our Peer interface is also an opaque ref
| case nil: | ||
| if len(acctDataBuf) > 0 { | ||
| persistedAcctData := &trackerdb.PersistedAccountData{Addr: addr, Rowid: rowid} | ||
| persistedAcctData := &trackerdb.PersistedAccountData{Addr: addr, Ref: ref} |
There was a problem hiding this comment.
it looks like more like account id, not reference
There was a problem hiding this comment.
Its intended name and use is as an opaque reference to the database entry.
You can't do much with except to give it back to the database engine.
Having an opaque identifier is a bit odd. This has no meaning outside of the engine it came out of.
In the case of the account, which is the identifier, the address? or the sqlite specific rowid?
| if delta.oldResource.AcctRef != nil { | ||
| acctRef = delta.oldResource.AcctRef | ||
| } else if acctRef, ok = knownAddresses[addr]; !ok { | ||
| acctRef, err = arw.LookupAccountRowID(addr) |
There was a problem hiding this comment.
nit: LookupAccountRowID -> LookupAccountByRef
maybe a separate PR
| a.updateOld(missIdx, persistedResData) | ||
| } else { | ||
| err = fmt.Errorf("empty resource record: addrid=%d, aidx=%d", addrid, aidx) | ||
| err = fmt.Errorf("empty resource record: addrid=%d, aidx=%d", acctRef, aidx) |
There was a problem hiding this comment.
what it will print out? acctRef is an interface, %d expects a number
There was a problem hiding this comment.
func main() {
var acctRef AccountRef
acctRef = ar{123}
fmt.Println("ln", acctRef)
fmt.Printf("as num %d", acctRef)
}gives
ln {123}
as num {123}
So the output changes but not critical looks like
|
Please followup with with a PR |
algorandskiy
changed the title
3 participants
Summary
Test Plan
Existing tests.