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

Oak v0.1 #5732

Closed
wants to merge 25 commits into from
Closed

Oak v0.1 #5732

wants to merge 25 commits into from

Conversation

sanastas
Copy link

@sanastas sanastas commented May 2, 2018

This is the Pull Request I am going to review

sanastas and others added 25 commits March 12, 2018 10:38
@sanastas
first files separation
a5d7673
@sanastas
Starting to getting getFacts() method out of IncrementalIndex
9072116
@sanastas
Intermediate commit
f203ea3
@sanastas
Once again, intermediate commit
5630e69
Started working on InternalDataIncrementalIndex.java
616e808
Merge branch 'OAK-V0.1' of https://github.com/sanastas/druid into OAK…
8b21b84 
…-V0.1
Finished timeAndDimsSerialization and timeAndDimsByteBufferComp
6efb278
@sanastas
Finished getFacts() elimination
fc832de
@sanastas
Merge branch 'OAK-V0.1' of github.com:sanastas/druid into OAK-V0.1
ca47842
Adding a TimeAndDims deserializer
832857e
Merge branch 'OAK-V0.1' of https://github.com/sanastas/druid into OAK…
7e6d04c 
…-V0.1

A TimeAndDims deserializer is essential.
Adding TimeAndDims serialization and comparator tests
8161fd5
@sanastas
Adding new class OffheapOakIncrementalIndex
be924d8
@sanastas
fix check style
6eb4b3c
Some small InternalDataIncrementalIndex fixes
2a6e30d
checkstyle fixes
3bce030
TimeAndDimsCompTest is now successful
68bfcda
moving methods from InternalDataIncrementalIndex to OffheapOakIncreme…
44ecb33 
…ntalIndex
Implementing the OffheapOakIncrementalIndex api, not including method…
fb5c835 
…s that use row indexes
the TimeAndDimsCompTest is now successful
43a4394
finished implementing the OffheapOakIncrementalIndex api
89f7a5a
Working on the OffheapOakIncrementalIndexTest
0b37c2f
OffheapOakIncrementalIndexTest goes over the full OffheapOakIncrement…
52a6673 
…alIndex API
Inserting into Ok using a value creator
6ee53e4
non-copying iterators
f477ad7
@jihoonson
Copy link
Contributor

Hi @sanastas, thank you for this work. It looks not ready for merging yet, but I'll take a look.

Before that, I wonder what's your plan about opening the source code of Oak you noted here. Also which license are you thinking to use?

kaijianding
kaijianding reviewed May 3, 2018
View reviewed changes
processing/src/main/java/io/druid/segment/incremental/OffheapOakIncrementalIndex.java
ByteBuffer from = timeAndDimsSerialization(new TimeAndDims(timeStart, null, dimensionDescsList));
ByteBuffer to = timeAndDimsSerialization(new TimeAndDims(timeEnd + 1, null, dimensionDescsList));
OakMap subMap = oak.subMap(from, true, to, false);
if (descending == true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (descending)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @jihoonson ,
Thanks for taking a look! The pull request is really an initial one and will be improved before we ask for a merge. Your comments are very welcome!
Regarding the open sourcing the code of Oak and its license, we currently discussing it internally and we will update you soon.

@kaijianding ,
Thank you as well, we will fix those styling issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sanastas great! Looking forward to your contribution!

@kaijianding
Copy link
Contributor

@sanastas how about indexer.processRowValsToUnsortedEncodedKeyComponent()? this is called in index.toTimeAndDims(); the StringDimensionIndexer uses on-heap to hold dictionary data, do you think it's worth moving the dictionary to off-heap? If it is high cardinality, it may cause frequent young gc

@sanastas
Copy link
Author

sanastas commented May 3, 2018

Hi @kaijianding ,
You have a valid point about this translation to TimeAndDims. We saw the existence of this dictionary, however we believe it is better to postpone dealing with this translation after we merge Oak to Druid. As you can see already now the pool request is big and there is no need to make it any bigger.
Currently we concentrate on the keys that are TimeAndDims, and their creation remains as it was before Oak.
Generally it should be beneficial to take the dictionary off-heap, however we postpone this and currently disregard this translation.

@kaijianding
Copy link
Contributor

@sanastas I see. hope this pr be merged soon

@fjy fjy added this to the 0.13.0 milestone Jul 5, 2018
@sanastas sanastas closed this Sep 2, 2018
@dclim dclim removed this from the 0.13.0 milestone Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaijianding kaijianding kaijianding left review comments

@jihoonson jihoonson jihoonson left review comments

Assignees
No one assigned
Labels
Area - Batch Ingestion Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sanastas @jihoonson @kaijianding @fjy @drcrallen @dclim