[Enhancement] Support complex type in column with row #41476
Conversation
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| template <typename T> | ||
| void ObjectColumn<T>::deserialize_and_append_batch(Buffer<Slice>& srcs, size_t chunk_size) { | ||
| DCHECK(false) << "Don't support object column deserialize and append"; | ||
| } | ||
|
|
||
| template <typename T> | ||
| uint32_t ObjectColumn<T>::serialize_size(size_t idx) const { | ||
| DCHECK(false) << "Don't support object column byte size"; | ||
| return 0; | ||
| return static_cast<uint32_t>(get_object(idx)->serialize_size()); | ||
| } | ||
|
|
||
| template <typename T> |
There was a problem hiding this comment.
The most risky bug in this code is:
Assuming ObjectColumn does not support certain operations originally, changing implementations to "support" them without proper checks or understanding of the overall system design might introduce serious logical errors or mismatches with expected functionality.
You can modify the code like this:
Restore the original behavior that explicitly mentions the lack of support for certain functions, ensuring that any attempts to use these functionalities knowingly fail, prompting a review of usage patterns and possibly a more suitable design adjustment.
template <typename T>
size_t ObjectColumn<T>::byte_size(size_t idx) const {
DCHECK(false) << "Don't support object column byte size";
return 0;
}
template <typename T>
uint32_t ObjectColumn<T>::serialize(size_t idx, uint8_t* pos) {
DCHECK(false) << "Don't support object column serialize";
return 0;
}
template <typename T>
void ObjectColumn<T>::serialize_batch(uint8_t* dst, Buffer<uint32_t>& slice_sizes, size_t chunk_size, uint32_t max_one_row_size) {
DCHECK(false) << "Don't support object column serialize batch";
}
template <typename T>
void ObjectColumn<T>::deserialize_and_append_batch(Buffer<Slice>& srcs, size_t chunk_size) {
DCHECK(false) << "Don't support object column deserialize and append";
}
template <typename T>
uint32_t ObjectColumn<T>::serialize_size(size_t idx) const {
DCHECK(false) << "Don't support object column byte size";
return 0;
}This change effectively reverts modifications that assumed the introduction of operational implementations where none were intended. The principle here is to maintain explicit indication of non-support for specific methods within ObjectColumn, which could prevent misuse or misunderstanding about what the class supports. Additional design considerations and safety mechanisms should be applied if and when these functionalities are to be officially supported, including thorough validations and error handling as appropriate.
decster
force-pushed
the
cwr-object-type
branch
3 times, most recently
from
8b047df
to
31b7971
Compare
Signed-off-by: Binglin Chang <decstery@gmail.com>
decster
force-pushed
the
cwr-object-type
branch
from
31b7971
to
0ad86d8
Compare
|
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 27 / 31 (87.10%) file detail
|
|
@Mergifyio backport branch-3.2 |
✅ Backports have been created
|
Signed-off-by: Binglin Chang <decstery@gmail.com> (cherry picked from commit 655a124)
… (#41855) Co-authored-by: Binglin Chang <decstery@gmail.com>
Signed-off-by: Binglin Chang <decstery@gmail.com> Signed-off-by: Seaven <seaven_7@qq.com>
Why I'm doing:
Currently, column with row storage type does not support complex types like bitmap, array, struct, bitmap
What I'm doing:
Support those types
Fixes #38174 #38090
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: