Adapt internal data structures to 512-bit era#42564
Adapt internal data structures to 512-bit era#42564nickitat merged 3 commits intoClickHouse:masterfrom
Conversation
robot-clickhouse
added
the
pr-not-for-changelog
nickitat
force-pushed
the
increase_padding_to_64_bytes
branch
from
8aa41ba to
5a3cdf4
Compare
nickitat
force-pushed
the
increase_padding_to_64_bytes
branch
from
d0aee17 to
5f5668d
Compare
nickitat
force-pushed
the
increase_padding_to_64_bytes
branch
from
5f5668d to
52b42bb
Compare
| < X-ClickHouse-Progress: {"read_rows":"131011","read_bytes":"1048081","written_rows":"0","written_bytes":"0","total_rows_to_read":"100000","result_rows":"0","result_bytes":"0"} | ||
| < X-ClickHouse-Progress: {"read_rows":"131011","read_bytes":"1048081","written_rows":"0","written_bytes":"0","total_rows_to_read":"100000","result_rows":"1","result_bytes":"80"} | ||
| < X-ClickHouse-Summary: {"read_rows":"131011","read_bytes":"1048081","written_rows":"0","written_bytes":"0","total_rows_to_read":"100000","result_rows":"1","result_bytes":"80"} | ||
| < X-ClickHouse-Progress: {"read_rows":"131011","read_bytes":"1048081","written_rows":"0","written_bytes":"0","total_rows_to_read":"100000","result_rows":"1","result_bytes":"272"} |
There was a problem hiding this comment.
Why has this PR changed the return bytes? Why is it dependent on the padding?
There was a problem hiding this comment.
Looking into it it seems that ColumnNumber uses PaddedPODArray as storage and this grew in size due to the extra padding (used to have 64B of padding and now it has 256B), so I guess it's expected.
Not sure why this is marked as not for changelog when the behaviour changes (as we can see in the tests) and CH uses more memory. I'm not saying that's wrong, but a notice would be nice.
There was a problem hiding this comment.
I don't think extra 200B memory consumption per column is a significant change for users
There was a problem hiding this comment.
I agree that's not significant, but it's changing the behaviour of the database. In our case we detected it because of integration tests.
I think my expectations about what should be in the changelog are different to the project's guidelines, because I keep having to look back into PRs that made noticeable (and sometimes really important) changes and they never made it to the changelog.
There was a problem hiding this comment.
Well, this was added to the changelog anyway. So sorry for the confusion and the noise.
1. Add PODArray ut 2. pick ClickHouse/ClickHouse#21533 3. pick ClickHouse/ClickHouse#21678 4. pick ClickHouse/ClickHouse#9498 5. pick ClickHouse/ClickHouse#7791 6. pick ClickHouse/ClickHouse#24271 7. if c_end_of_storage == null, allocated_bytes return 0. 8. Add some checks for DEBUG. 9. TODO: Adapt internal data structures to 512-bit era ClickHouse/ClickHouse#42564
1. Add PODArray ut 2. pick ClickHouse/ClickHouse#21533 3. pick ClickHouse/ClickHouse#21678 4. pick ClickHouse/ClickHouse#9498 5. pick ClickHouse/ClickHouse#7791 6. pick ClickHouse/ClickHouse#24271 7. if c_end_of_storage == null, allocated_bytes return 0. 8. Add some checks for DEBUG. 9. TODO: Adapt internal data structures to 512-bit era ClickHouse/ClickHouse#42564
1. Add PODArray ut 2. pick ClickHouse/ClickHouse#21533 3. pick ClickHouse/ClickHouse#21678 4. pick ClickHouse/ClickHouse#9498 5. pick ClickHouse/ClickHouse#7791 6. pick ClickHouse/ClickHouse#24271 7. if c_end_of_storage == null, allocated_bytes return 0. 8. Add some checks for DEBUG. 9. TODO: Adapt internal data structures to 512-bit era ClickHouse/ClickHouse#42564
4 participants
Changelog category (leave one):
Padding in internal data structures increased to 64 bytes.