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.

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

Add/Drop Indexes #337

Closed
wants to merge 745 commits into from

Conversation

@yeshengm
Copy link
Member

yeshengm commented Apr 6, 2019

  • Add catalog support.
  • Support building index in a very basic way.

Known issues:

  • Segfault in SQLTable when there are too many rows. Possibly caused by GC.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #337 into master will increase coverage by 0.28%.
The diff coverage is 95.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
+ Coverage   87.64%   87.92%   +0.28%     
==========================================
  Files         212      217       +5     
  Lines        8246     8466     +220     
==========================================
+ Hits         7227     7444     +217     
- Misses       1019     1022       +3
Impacted Files Coverage Δ
src/include/storage/index/index_builder.h 100% <ø> (+1.75%) ⬆️
src/include/catalog/type_handle.h 100% <ø> (ø) ⬆️
src/include/catalog/catalog_entry.h 100% <ø> (ø) ⬆️
src/include/storage/index/index.h 100% <ø> (ø) ⬆️
src/include/transaction/transaction_manager.h 100% <ø> (ø) ⬆️
src/include/catalog/catalog.h 100% <ø> (ø) ⬆️
src/include/catalog/database_handle.h 100% <ø> (ø) ⬆️
src/transaction/transaction_manager.cpp 95.62% <100%> (+0.3%) ⬆️
src/catalog/database_handle.cpp 97.61% <100%> (+0.11%) ⬆️
src/storage/index/index_builder.cpp 100% <100%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1c3979...0bf7b7c. Read the comment docs.

@yeshengm yeshengm self-assigned this Apr 7, 2019
@apavlo apavlo added the class-project label Apr 8, 2019
@yeshengm yeshengm changed the title Add/drop indexes Add/Drop Indexes Apr 8, 2019
@DarkForte DarkForte self-requested a review Apr 10, 2019
@YuzeLiao YuzeLiao self-requested a review Apr 11, 2019
src/include/storage/index/index_populator.h Outdated Show resolved Hide resolved
test/storage/index_populator_test.cpp Outdated Show resolved Hide resolved
src/storage/lock_manager.cpp Outdated Show resolved Hide resolved
src/include/storage/index/index_populator.h Outdated Show resolved Hide resolved
src/catalog/index_handle.cpp Outdated Show resolved Hide resolved
src/include/catalog/index_handle.h Outdated Show resolved Hide resolved
src/include/catalog/index_handle.h Outdated Show resolved Hide resolved
src/include/catalog/index_handle.h Show resolved Hide resolved
src/include/catalog/index_handle.h Outdated Show resolved Hide resolved
src/include/catalog/index_handle.h Outdated Show resolved Hide resolved
src/include/storage/index/index_populator.h Outdated Show resolved Hide resolved
src/include/storage/index/index_populator.h Outdated Show resolved Hide resolved
src/storage/lock_manager.cpp Outdated Show resolved Hide resolved
test/storage/index_populator_test.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

YuzeLiao left a comment

[Conclusion]
Generally speaking, I think the code (index-related code) is good and there is probably no severe problems in terms of the very basic functions (i.e. populating tuples). The code is readable and probably because current logic is not complex so there are few log functions for debug purpose. Probably they should be leveraged in the near future.

The documentation looks good but I do think typos should be fixed. A basic test is provided to test the naive index populator. I think its logic can correctly test the basic function of the index populator. They are not able to work in multi-threaded scenarios for now since the blocking semantics has not been implemented so they haven't written such tests. Therefore, more tests might be added when more functions are implemented in the near future.

I think it will be good to follow the roadmap and keep going:)

test/storage/index_populator_test.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

DarkForte left a comment

Some code in the index populator test is either not elegant or confusing, please have a look at the comments below.

test/storage/index_populator_test.cpp Outdated Show resolved Hide resolved
test/storage/index_populator_test.cpp Outdated Show resolved Hide resolved
test/storage/index_populator_test.cpp Outdated Show resolved Hide resolved
test/storage/index_populator_test.cpp Outdated Show resolved Hide resolved
test/storage/index_populator_test.cpp Outdated Show resolved Hide resolved
test/storage/index_populator_test.cpp Outdated Show resolved Hide resolved
mbutrovich and others added 17 commits Apr 19, 2019
… into 15-721-project2-stats
wenxuanq
Catalog
… into 15-721-project2-stats
wenxuanq
wenxuanq
wenxuanq
… into 15-721-project2-stats
wenxuanq
lagoon0o0 and others added 27 commits May 13, 2019
This reverts commit f2d6f32.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.