Add rvalue vector ctors for ColumnDate, ColumnDate32 and ColumnIPv4 #332
Conversation
There was a problem hiding this comment.
Hi @bduminiuc, thanks for your contibution!
As for ColumnDateTime -- feel free to add constructor that takes vector<uint32_t> &&, the underlying storage type is not going to be changed in the foreseeable future.
Also, please consider adding unit-tests that verify the added constructors.
|
Hi, @Enmk, The constructor for |
| @@ -528,6 +562,23 @@ TEST(ColumnsCase, ColumnIPv4_construct_from_data) | |||
| EXPECT_ANY_THROW(ColumnIPv4(ColumnRef(std::make_shared<ColumnString>()))); | |||
| } | |||
|
|
|||
| TEST(ColumnsCase, ColumnIPv4_construct_from_rvalue_data) { | |||
| std::vector<uint32_t> data = { | |||
There was a problem hiding this comment.
Please use MakeIPv4s, unless you have a good reason not to.
There was a problem hiding this comment.
The newly added constructor for ColumnIPv4 accepts a vector of raw values std::vector<uint32_t>&&, while MakeIPv4 returns in_addr. It seems that the MakeIPv4 function cannot be used to provide test coverage for ColumnIPv4(std::vector<uint32_t>&&).
Should something specific be done for the case?
ut/columns_ut.cpp
Outdated
| ASSERT_EQ(col1->Size(), expected.size()); | ||
| for (size_t i = 0; i < expected.size(); ++i) { | ||
| ASSERT_EQ(col1->At(i), static_cast<std::time_t>(expected[i])); | ||
| } | ||
| } |
There was a problem hiding this comment.
you can use a single check instead of for-loop here:
EXPECT_TRUE(CompareRecursive(*col1, expected));Takes care of size checks, value checks, and forms a pretty explanation message if comparison fails.
|
|
||
| ASSERT_EQ(col1->Size(), expected.size()); | ||
| for (size_t i = 0; i < expected.size(); ++i) { | ||
| ASSERT_EQ(col1->RawAt(i), expected[i]); |
There was a problem hiding this comment.
(note in regards of https://github.com/ClickHouse/clickhouse-cpp/pull/332/files#r1338877216)
CompareRecursive isn't going to work here due to value transformation by ColumnDate, same for DateTime32.
Methods returning bogus time_t values is the hard legacy we need to get rid of, but I haven't decided how to make a smooth transition for the users...
…d ColumnDateTime unit-tests
These constructors can be valuable for enhancing performance in scenarios where a user knows the row count and can preallocate memory.
I also considered adding a similar constructor for
ColumnDateTime, but its public interface exclusively utilizestime_t, whileuint32_tis an implementation detail. I'm unsure if extending the interface with implementation details would be a good idea.