Fix bug related to filtering of dedicated columns - #6586
Conversation
|
|
||
| func CreateBlock(ctx context.Context, cfg *common.BlockConfig, meta *backend.BlockMeta, i common.Iterator, r backend.Reader, to backend.Writer) (*backend.BlockMeta, error) { | ||
| s := newStreamingBlock(ctx, cfg, meta, r, to, tempo_io.NewBufferedWriter) | ||
| s, newMeta := newStreamingBlock(ctx, cfg, meta, r, to, tempo_io.NewBufferedWriter) |
There was a problem hiding this comment.
The meta returned here is only partially populated. i.e. final stats like size aren't populated yet. But things happen to work because it's a pointer and the underlying object is continued to be updated. Might prefer to put it behind a method like .Meta() that implies it's the latest at the time of the call, thoughts?
There was a problem hiding this comment.
Good point. The main motivation behind the signature change was to make it explicit to the caller that the returned meta has been modified and needs to be respected on the caller side. Not surfacing this clearly was likely what caused the earlier bug with filtered vs. unfiltered dedicated columns. Using a .Meta() doesn’t communicate as clearly that the caller should update its local meta.
I can think of a couple of alternative ways to address this:
- Return a more fully populated
newMeta - Return the filtered dedicated columns instead, and update
metaat the call site, e.g (preferred)
s, meta.DedicatedColumns = newStreamingBlock(ctx, cfg, meta, r, to, tempo_io.NewBufferedWriter)What do you think?
There was a problem hiding this comment.
Ok yes that it was likely the cause for the error, has convinced me. This signature is good.
* Fix dedicated column filtering in vp4 * Fix dedicated column filtering in vp3 * Fix dedicated column filtering in vp5 * CHANGELOG.md
2 participants
What this PR does:
This bug can cause missing attributes when the tempo config or overrides contain dedicated columns that are filtered. For example when vp4 blocks contain dedicated columns that correspond to well-known attributes.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]