Skip to content

[WIP] Unified Binary Reader/Writer #30

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Alphasite
Copy link
Owner

This need a really through review, since it changes a core part of the system. Please check it over.

@Alphasite Alphasite self-assigned this Nov 3, 2017
@Alphasite Alphasite requested review from evqna and tyleri November 3, 2017 02:20
@Alphasite
Copy link
Owner Author

Alphasite commented Nov 3, 2017

This file in particular: src/db/datastore/tuple/binary/BinaryTupleReaderWriter.java

@Alphasite Alphasite changed the title Unified Binary Reader/Writer [WIP] Unified Binary Reader/Writer Nov 3, 2017
@Alphasite Alphasite added the WIP label Nov 3, 2017
@@ -120,7 +123,7 @@ private Tuple getNextTuple() {
* @inheritDoc
*/
@Override
public void seek(long index) {
public void seek(int index) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, why the change to int?

Copy link
Owner Author

Choose a reason for hiding this comment

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

originally the BB required inputs as longs, but now a ton of external APIs require ints, so I flipped to what was most common.

@evqna
Copy link
Collaborator

evqna commented Nov 3, 2017

Why do we need this exactly ? Is it required before we do bulk loading or is it just to simplify some code in external sort / SMJ ?

@Alphasite
Copy link
Owner Author

It simplifies the index creation, since you can implement random read and random write into the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants