-
Notifications
You must be signed in to change notification settings - Fork 466
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
persist: Add columnar format to HollowBatchPart
#27800
persist: Add columnar format to HollowBatchPart
#27800
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, but I think you may indeed have a backcompat problem - at a glance roundtripping between versions seems like it may lose the new field. Approving assuming you can either add a dyncfg or convince yourself it's not a real problem...
Some(cfg.batch_columnar_format) | ||
} else { | ||
None | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may fail to roundtrip during upgrade, if a batch with a columnar format is written by a new version but retracted by the old version without the proto change.
This is probably fine iff no environments currently have the new columnar format enabled, but otherwise you may want an additional flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! I think adding a second flag to control the writing is easier to reason about so I updated the PR to do that
a38c6e9
to
22e3f91
Compare
This PR adds the format of a
Part
(e.g. Row or RowAndColumnar) to theHollowBatchPart
, so our format is observable from CRDB state without having to read a blob.Dan originally mentioned adding this to
HollowBatch
, which I tried but it seems like we might concat the parts from multiple batches into a single batch, which creates the possibility for a singleBatch
to containPart
s with different formats.@bkirwi I'm not sure what our stability guarantees are for
HollowBatchPart
. I added this in a way that should be backwards and forwards compatible, but please double check me hereMotivation
Making columnar format observable from State, follow-up from #26561 (review)
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.