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

ARROW-5918: [Java] Add get to BaseIntVector interface #4859

Closed
wants to merge 9 commits into from
Closed

ARROW-5918: [Java] Add get to BaseIntVector interface #4859

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5739cc4
ARROW-5918: [Java] Add get to BaseIntVector interface
tianchen92 Jul 12, 2019
9f2fc09
fix
tianchen92 Jul 12, 2019
9b8afa6
enable null checking
tianchen92 Jul 13, 2019
fe9c2ec
fix
tianchen92 Jul 13, 2019
e65473c
fix comments
tianchen92 Jul 13, 2019
26d30be
fix
tianchen92 Jul 16, 2019
7ddce12
fix
tianchen92 Jul 18, 2019
1562ae1
fix 2
tianchen92 Jul 18, 2019
3732a78
update javadoc and add unsafe method in BaseIntVector
tianchen92 Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion java/vector/src/main/java/org/apache/arrow/vector/BaseIntVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,14 @@
public interface BaseIntVector extends ValueVector {

/**
* set value at specific index, note this value may need to be need truncated.
* Sets the value at index, note this value may need to be need truncated.
*/
void setWithPossibleTruncate(int index, long value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need a setSafe method here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this method is implemented with setSafe in sub classes :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks. I am not sure if we should use separate APIs for set and setSafe methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets update the javadoc to indicate this is the safe version. Unfortuantely, I think this made it into the 0.14.0, so we should try not to rename it. We can always add an unsafe version later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks, will also add a method like "setUnsafeWithPossibleTruncate"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your efforts, PR updated.


/**
* Gets the value at index.
* This value may have been extended to long and will throw {@link NullPointerException}
* if the value is null. Note null check could be turned off via {@link NullCheckingForGet}.
*/
long getValueAsLong(int index);
}
11 changes: 8 additions & 3 deletions java/vector/src/main/java/org/apache/arrow/vector/BigIntVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,6 @@ public MinorType getMinorType() {
* @return element at given index
*/
public long get(int index) throws IllegalStateException {
if (NULL_CHECKING_ENABLED && isSet(index) == 0) {
throw new IllegalStateException("Value at index is null");
}
return valueBuffer.getLong(index * TYPE_WIDTH);
}

Expand Down Expand Up @@ -327,6 +324,14 @@ public void setWithPossibleTruncate(int index, long value) {
this.setSafe(index, value);
}

@Override
public long getValueAsLong(int index) {
if (NULL_CHECKING_ENABLED && isSet(index) == 0) {
tianchen92 marked this conversation as resolved.
Show resolved Hide resolved
tianchen92 marked this conversation as resolved.
Show resolved Hide resolved
throw new IllegalStateException("Value at index is null");
}
return this.get(index);
}

private class TransferImpl implements TransferPair {
BigIntVector to;

Expand Down
5 changes: 5 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/IntVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,11 @@ public void setWithPossibleTruncate(int index, long value) {
this.setSafe(index, (int) value);
}

@Override
public long getValueAsLong(int index) {
return this.get(index);
}

private class TransferImpl implements TransferPair {
IntVector to;

Expand Down
5 changes: 5 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/SmallIntVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,11 @@ public void setWithPossibleTruncate(int index, long value) {
this.setSafe(index, (int) value);
}

@Override
public long getValueAsLong(int index) {
return this.get(index);
}

private class TransferImpl implements TransferPair {
SmallIntVector to;

Expand Down
5 changes: 5 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/TinyIntVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,11 @@ public void setWithPossibleTruncate(int index, long value) {
this.setSafe(index, (int) value);
}

@Override
public long getValueAsLong(int index) {
return this.get(index);
}

private class TransferImpl implements TransferPair {
TinyIntVector to;

Expand Down
5 changes: 5 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/UInt1Vector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,11 @@ public void setWithPossibleTruncate(int index, long value) {
this.setSafe(index, (int) value);
}

@Override
public long getValueAsLong(int index) {
return this.get(index);
}



private class TransferImpl implements TransferPair {
Expand Down
5 changes: 5 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/UInt2Vector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,11 @@ public void setWithPossibleTruncate(int index, long value) {
this.setSafe(index, (int) value);
}

@Override
public long getValueAsLong(int index) {
return this.get(index);
}

private class TransferImpl implements TransferPair {
UInt2Vector to;

Expand Down
5 changes: 5 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/UInt4Vector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,11 @@ public void setWithPossibleTruncate(int index, long value) {
this.setSafe(index, (int) value);
}

@Override
public long getValueAsLong(int index) {
return this.get(index);
}

private class TransferImpl implements TransferPair {
UInt4Vector to;

Expand Down
5 changes: 5 additions & 0 deletions java/vector/src/main/java/org/apache/arrow/vector/UInt8Vector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,11 @@ public void setWithPossibleTruncate(int index, long value) {
this.setSafe(index, value);
}

@Override
public long getValueAsLong(int index) {
return this.get(index);
}

private class TransferImpl implements TransferPair {
UInt8Vector to;

Expand Down
7 changes: 4 additions & 3 deletions java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,11 @@ public static ValueVector decode(ValueVector indices, Dictionary dictionary) {
// copy the dictionary values into the decoded vector
TransferPair transfer = dictionaryVector.getTransferPair(indices.getAllocator());
transfer.getTo().allocateNewSafe();

BaseIntVector baseIntVector = (BaseIntVector) indices;
for (int i = 0; i < count; i++) {
Object index = indices.getObject(i);
if (index != null) {
int indexAsInt = ((Number) index).intValue();
if (!baseIntVector.isNull(i)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

encoded value can never be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, see https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java#L72

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. The benefit of not encoding null is efficient storage. However, the benefit of encoding null is efficient computation (no if branch in the encoding/decoding logic, which makes the CPU pipeline/prefetch more efficient).

We may need some investigation on this. For this particular case, it may be OK to stick to the original assumption.

int indexAsInt = (int) baseIntVector.getValueAsLong(i);
if (indexAsInt > dictionaryCount) {
throw new IllegalArgumentException("Provided dictionary does not contain value for index " + indexAsInt);
}
Expand Down