Skip to content

Support Date32 data type.#175

Merged
Enmk merged 6 commits into
ClickHouse:masterfrom
huyphams:support-date32
Jun 7, 2022
Merged

Support Date32 data type.#175
Enmk merged 6 commits into
ClickHouse:masterfrom
huyphams:support-date32

Conversation

@huyphams

Copy link
Copy Markdown
Contributor
  • Added date32 (Which is 4 bytes when compared with 2 byte Date).

@CLAassistant

CLAassistant commented Apr 27, 2022

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@huyphams

Copy link
Copy Markdown
Contributor Author

I think I should add a test for Date32

@Enmk

Enmk commented May 3, 2022

Copy link
Copy Markdown
Contributor

I think I should add a test for Date32

Hi, thank you for contribution!

Yes, please add some tests. There are two kinds of them:

  • unit-tests (ut/columns_ut.cpp), where one should test APIs of Column: creating, appending, getting values, slicing, etc.
  • integration-tests (ut/client_ut.cpp), where one should test roundtrip of Column data to/from CH server.

If you find it difficult adding both, please add at least something and allow edit from maintainers in this PR.

Comment thread clickhouse/types/types.h Outdated
FixedString,
DateTime,
Date,
Date32,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the new value to the end. This is enum is visible to users and some may rely on order of elements. I think we can re-arrange those to look better in a next major version, like 3.0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Enmk Enmk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix minor issue mentioned

@huyphams

huyphams commented May 3, 2022

Copy link
Copy Markdown
Contributor Author

Thank you for reviewing, I will fix it 🙌

@Enmk

Enmk commented May 4, 2022

Copy link
Copy Markdown
Contributor

Please rebase/merge master and fix the builds

@huyphams huyphams requested a review from Enmk May 6, 2022 04:37
@huyphams

huyphams commented May 6, 2022

Copy link
Copy Markdown
Contributor Author

Hi @Enmk I've merged master into this branch. Could you help me to write the test (if you have time)? 🙌

@Enmk

Enmk commented May 7, 2022

Copy link
Copy Markdown
Contributor

Hi @Enmk I've merged master into this branch. Could you help me to write the test (if you have time)? raised_hands

Sure, I've drafted a scaffold for unit-testing Column implementation. Will try to finalize and commit it on Monday.

@Enmk

Enmk commented May 12, 2022

Copy link
Copy Markdown
Contributor

Hi @Enmk I've merged master into this branch. Could you help me to write the test (if you have time)? raised_hands

Hello @huyphams !
It took a bit more time to iron everything out, but it is almost ready now:
Once #179 is merged, you can use a test suite to verify that column correctly implements most of the expected methods.

In order to benefit form test suite, you'll need to make some changes to the ut/Column_ut.cpp:

  • add ColumnDateTime32 to the type-list using ValueColumns = ::testing::Types<
  • add meaningful value generator to static auto GenerateValues(size_t values_size) {
  • optionally add proper constructor to static auto MakeColumn() {

If you have some very specific test cases, that are not covered by generic test suite, you can add those into ut/coulns_ut.cpp.

Also you are welcome to review the pr #179

@Enmk

Enmk commented May 20, 2022

Copy link
Copy Markdown
Contributor

@huyphams ping?

@Enmk Enmk merged commit cd0b27a into ClickHouse:master Jun 7, 2022
@huyphams huyphams deleted the support-date32 branch May 30, 2026 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants