Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make Field::get type-safe in debug builds #7386

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Make Field::get type-safe in debug builds #7386

merged 1 commit into from
Dec 17, 2019

Conversation

akuzm
Copy link
Contributor

@akuzm akuzm commented Oct 18, 2019

Category (leave one):

  • Build/Testing/Packaging Improvement

Short description (up to few sentences):
Make Field methods more type-safe to find more errors.

@akuzm akuzm added the pr-build Pull request with build/testing/packaging improvement label Oct 18, 2019
@akuzm akuzm changed the title [wip] more type safety for fields Make Field::get<> type-safe in debug builds Oct 25, 2019
@akuzm akuzm changed the title Make Field::get<> type-safe in debug builds Make Field::get type-safe in debug builds Oct 25, 2019
@akuzm akuzm marked this pull request as ready for review October 25, 2019 14:32
alexey-milovidov
alexey-milovidov reviewed Oct 25, 2019
View reviewed changes
dbms/src/Core/Field.h Outdated
{
assert(false);
Null null;
return f(null);
Copy link
Member

Choose a reason for hiding this comment

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

Why we need this code? Maybe just throw an exception?

Copy link
Member

Choose a reason for hiding this comment

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

Or add __buitin_unreachable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we don't need an Int128 Field at all, I'll try to remove it.

alexey-milovidov
alexey-milovidov approved these changes Oct 25, 2019
View reviewed changes
Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

Ok.

@alexey-milovidov
Copy link
Member

@akuzm Need to investigate tests.

@alexey-milovidov
Copy link
Member

@akuzm Tests have failed.

@akuzm akuzm force-pushed the aku/field-type-safety branch 2 times, most recently from d5a5247 to e335ae3 Compare November 8, 2019 12:12
@alexey-milovidov
Copy link
Member

@akuzm ?

@akuzm akuzm force-pushed the aku/field-type-safety branch 2 times, most recently from 260fd49 to e84240b Compare December 9, 2019 17:50
@alexey-milovidov
Copy link
Member

It looks like it started to work:

9.12.09 21:45:23.703176 [ 54 ] {} <Fatal> BaseDaemon: 8. 0x119b5d67 unsigned long& DB::Field::get<unsigned long>() /build/obj-x86_64-linux-gnu/../dbms/src/Core/Field.h:0
2019.12.09 21:45:23.715984 [ 54 ] {} <Fatal> BaseDaemon: 9. 0x1483d3de DB::FlatDictionary::blockToAttributes(DB::Block const&) /build/obj-x86_64-linux-gnu/../dbms/src/Dictionaries/FlatDictionary.cpp:316

@akuzm
Copy link
Contributor Author

akuzm commented Dec 10, 2019 •
edited
Loading

It looks like it started to work:

Nope, it's the same boring thing that accounts for the most test failures -- implicit reinterpret_cast between signed and unsigned ints. Most instances of it are really OK so maybe I should just allow it.

@akuzm akuzm mentioned this pull request Dec 10, 2019
Merged
@akuzm akuzm force-pushed the aku/field-type-safety branch 2 times, most recently from f26b4b7 to 004cfa9 Compare December 13, 2019 13:20
@akuzm
Copy link
Contributor Author

akuzm commented Dec 13, 2019

The test are too slow now, I'll have to profile them. Will separately commit the fixes I made, without enabling the assert.

@akuzm
Copy link
Contributor Author

akuzm commented Dec 13, 2019 •
edited
Loading

Too slow meaning that the functional stateless tests in debug take longer to complete than integration tests, which were the longest before (not including performance tests).

akuzm added a commit that referenced this pull request Dec 13, 2019
@akuzm
Fix some implicit Field casts.
beaf790 
This is a part of #7386
@akuzm akuzm mentioned this pull request Dec 13, 2019
Merged
akuzm added a commit that referenced this pull request Dec 16, 2019
@akuzm
Fix some implicit Field casts.
9f6dbe0 
This is a part of #7386
@akuzm
Check Field type in get().
5ed561f 
Allow silent conversion only between Int64 and UInt64.
@akuzm
Copy link
Contributor Author

akuzm commented Dec 17, 2019

Test failures are flaps or broken in master. The run time improved after rewriting the assertion, now it's about 2h:20m, the same as in master.

@akuzm akuzm merged commit 89a2ec3 into master Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees
No one assigned
Labels
pr-build Pull request with build/testing/packaging improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@akuzm @alexey-milovidov