Skip to content

fix: dictionary decimal vector optimization - #705

Merged
kazuyukitanimura merged 6 commits into
apache:mainfrom
kazuyukitanimura:fix-decimal-copy
Jul 24, 2024
Merged

kazuyukitanimura merged 6 commits into
apache:mainfrom
kazuyukitanimura:fix-decimal-copy

Conversation

@kazuyukitanimura

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #679 and #670
Related #490

Rationale for this change

For dictionary decimal vectors, it was unpacking even for Int and Long decimals that used more memory than necessary.

What changes are included in this PR?

Unpack only for Decimal 128

How are these changes tested?

Existing test

@kazuyukitanimura
kazuyukitanimura marked this pull request as ready for review July 22, 2024 23:54
Comment thread common/src/main/java/org/apache/comet/vector/CometDictionary.java Outdated
Comment thread common/src/main/java/org/apache/comet/vector/CometDictionary.java Outdated
@kazuyukitanimura

Copy link
Copy Markdown
Contributor Author

@andygrove @parthchandra I moved the logic to the reader side, in that way, this PR will help for native exec as well

@parthchandra parthchandra left a comment

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'm not entirely sure how this helps on the exec side, could you provide a short explanation?
Otherwise lgtm

case FIXEDSIZEBINARY:
return values.getBinary(index);
case DECIMAL:
if (binaries == null) {

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?

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.

@kazuyukitanimura

Copy link
Copy Markdown
Contributor Author

I'm not entirely sure how this helps on the exec side, could you provide a short explanation? Otherwise lgtm

Thanks @parthchandra
When the native exec is enabled CometDictionary is still called that decode the dictionary. However the native exec does not use CometDictionary to read the values. So by moving the logic to the reader, it decodes only when we need it (scan-only case)

@kazuyukitanimura
kazuyukitanimura merged commit c1b7c7d into apache:main Jul 24, 2024
@kazuyukitanimura

Copy link
Copy Markdown
Contributor Author

Merged Thanks @parthchandra @andygrove

himadripal pushed a commit to himadripal/datafusion-comet that referenced this pull request Sep 7, 2024
## Which issue does this PR close?

Part of apache#679 and apache#670
Related apache#490

## Rationale for this change

For dictionary decimal vectors, it was unpacking even for Int and Long decimals that used more memory than necessary.

## What changes are included in this PR?

Unpack only for Decimal 128

## How are these changes tested?

Existing test

(cherry picked from commit c1b7c7d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants