-
Notifications
You must be signed in to change notification settings - Fork 285
add committs in objects #22529
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
add committs in objects #22529
Conversation
PR Reviewer Guide 🔍
Here are some key observations to aid the review process:
Possible Issue
In the special-column handling, putFillHolder
stores the resolved seqnum
(which may be metaColCnt-1
) into the placeholder Size
marker. Downstream code likely relies on original requested seqnum semantics; verify consumers correctly interpret this marker and that overloading Size
with a seqnum cannot collide with legitimate zero-length reads or cause misalignment when multiple placeholders exist.
putFillHolder
stores the resolved seqnum
(which may be metaColCnt-1
) into the placeholder Size
marker. Downstream code likely relies on original requested seqnum semantics; verify consumers correctly interpret this marker and that overloading Size
with a seqnum cannot collide with legitimate zero-length reads or cause misalignment when multiple placeholders exist.var filledEntries []fileservice.IOEntry putFillHolder := func(i int, seqnum uint16) { if filledEntries == nil { filledEntries = make([]fileservice.IOEntry, len(seqnums)) } filledEntries[i] = fileservice.IOEntry{ Size: int64(seqnum), // a marker, it can not be zero } } blkmeta := meta.GetBlockMeta(uint32(blk)) maxSeqnum := blkmeta.GetMaxSeqnum() for i, seqnum := range seqnums { // special columns if seqnum >= SEQNUM_UPPER { metaColCnt := blkmeta.GetMetaColumnCount() switch seqnum { case SEQNUM_COMMITTS: seqnum = metaColCnt - 1 case SEQNUM_ABORT: panic("not support") default: panic(fmt.Sprintf("bad path to read special column %d", seqnum)) } // if the last column is not commits, do not read it // 1. created by cn // 2. old version tn nonappendable block col := blkmeta.ColumnMeta(seqnum) if col.DataType() != uint8(types.T_TS) { putFillHolder(i, seqnum) } else { ext := col.Location() ioVec.Entries = append(ioVec.Entries, fileservice.IOEntry{ Offset: int64(ext.Offset()), Size: int64(ext.Length()), ToCacheData: factory(int64(ext.OriginSize()), ext.Alg()), }) } continue } // need fill vector if seqnum > maxSeqnum || blkmeta.ColumnMeta(seqnum).DataType() == 0 { putFillHolder(i, seqnum) continue
Edge Case
Mapping the last requested column if it is of TS type to SEQNUM_COMMITTS
assumes the TS column is always the final one. If clients request a subset or reorder columns, this may mis-map a user TS column. Validate the assumption and consider a more explicit detection of the commit-ts attribute rather than positional last-column logic.
SEQNUM_COMMITTS
assumes the TS column is always the final one. If clients request a subset or reorder columns, this may mis-map a user TS column. Validate the assumption and consider a more explicit detection of the commit-ts attribute rather than positional last-column logic.if col.DataType() == uint8(types.T_TS) && i == len(c.cols)-1 { idxs = append(idxs, objectio.SEQNUM_COMMITTS) } else { idxs = append(idxs, idx) } typs = append(typs, tp)
Logging Noise
The new logutil.Info
inside replaceCommitts
will emit a log per scan with batch length; under heavy scans this can be noisy. Consider lowering to Debug or adding rate limiting/feature flags.
logutil.Info
inside replaceCommitts
will emit a log per scan with batch length; under heavy scans this can be noisy. Consider lowering to Debug or adding rate limiting/feature flags.logutil.Info("committs replace", zap.Bool("createdByCN", stats.GetCNCreated()), zap.Bool("appendable", stats.GetAppendable()), zap.String("createdTS", createTS.String()), zap.Int("length", length), )
PR Code Suggestions ✨Explore these optional code suggestions:
|
Uh oh!
There was an error while loading. Please reload this page.
User description
What type of PR is this?
Which issue(s) this PR fixes:
issue #22524
What this PR does / why we need it:
PR Type
Enhancement
Description
Add commit timestamp column to all objects for data change backtracing
Handle legacy objects without commit timestamps by returning ConstNull vectors
Refactor column reading logic to support special commit timestamp column
Update merge and flush operations to include commit timestamps
Diagram Walkthrough
File Walkthrough
txnts.go
Add String method to TS type
pkg/container/types/txnts.go
String()
method to TS type for formatting interface compatibilityfuncs.go
Enhance column reading with commit timestamp support
pkg/objectio/funcs.go
putFillHolder
helper functiontool.go
Handle commit timestamp in object data retrieval
pkg/vm/engine/tae/rpc/tool.go
flushTableTail.go
Always include commit timestamp in flush operations
pkg/vm/engine/tae/tables/jobs/flushTableTail.go
mergeobjects.go
Always include commit timestamp in merge operations
pkg/vm/engine/tae/tables/jobs/mergeobjects.go
pnode.go
Handle legacy objects with commit timestamp fallback
pkg/vm/engine/tae/tables/pnode.go
replaceCommitts
function to handle legacy objects withouttimestamps
utils.go
Enhance column loading with commit timestamp tracking
pkg/vm/engine/tae/tables/utils.go
operations