Skip to content

fix: dictionary decimal vector optimization #705

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

Merged
kazuyukitanimura merged 6 commits into from
Jul 24, 2024
Merged

fix: dictionary decimal vector optimization #705

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e82b95d
fix: dictionary decimal vector optimization
kazuyukitanimura Jul 22, 2024
031ae48
address review comments
kazuyukitanimura Jul 23, 2024
f324022
address review comments
kazuyukitanimura Jul 23, 2024
31e29ec
Merge remote-tracking branch 'upstream/main' into fix-decimal-copy
kazuyukitanimura Jul 23, 2024
704734f
Merge remote-tracking branch 'upstream/main' into fix-decimal-copy
kazuyukitanimura Jul 23, 2024
6d85a7c
address review comments
kazuyukitanimura Jul 23, 2024
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
31 changes: 13 additions & 18 deletions common/src/main/java/org/apache/comet/vector/CometDictionary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ public class CometDictionary implements AutoCloseable {
public CometDictionary(CometPlainVector values) {
this.values = values;
this.numValues = values.numValues();
initialize();
}

public void setDictionaryVector(CometPlainVector values) {
Expand Down Expand Up @@ -83,6 +82,19 @@ public byte[] decodeToBinary(int index) {
case FIXEDSIZEBINARY:
return values.getBinary(index);
case DECIMAL:
if (binaries == null) {

@parthchandra parthchandra Jul 23, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if this need to be made thread safe?

@kazuyukitanimura kazuyukitanimura Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think this gets read in multiple threads, but even if it would be, it is overwrite, so should be fine.

// We only need to copy values for decimal 128 type as random access
// to the dictionary is not efficient for decimal (it needs to copy
// the value to a new byte array everytime).
ByteArrayWrapper[] binaries = new ByteArrayWrapper[numValues];
for (int i = 0; i < numValues; i++) {
// Need copying here since we re-use byte array for decimal
byte[] bytes = new byte[DECIMAL_BYTE_WIDTH];
bytes = values.copyBinaryDecimal(i, bytes);
binaries[i] = new ByteArrayWrapper(bytes);
}
this.binaries = binaries;
}
return binaries[index].bytes;
default:
throw new IllegalArgumentException(
Expand All @@ -99,23 +111,6 @@ public void close() {
values.close();
}

private void initialize() {
switch (values.getValueVector().getMinorType()) {
case DECIMAL:
// We only need to copy values for decimal type as random access
// to the dictionary is not efficient for decimal (it needs to copy
// the value to a new byte array everytime).
binaries = new ByteArrayWrapper[numValues];
for (int i = 0; i < numValues; i++) {
// Need copying here since we re-use byte array for decimal
byte[] bytes = new byte[DECIMAL_BYTE_WIDTH];
bytes = values.copyBinaryDecimal(i, bytes);
binaries[i] = new ByteArrayWrapper(bytes);
}
break;
}
}

private static class ByteArrayWrapper {
private final byte[] bytes;

Expand Down