You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kazuyukitanimura (via GitHub)" <gi...@apache.org> on 2024/07/22 23:40:35 UTC
[PR] fix: dictionary decimal vector optimization [datafusion-comet]
kazuyukitanimura opened a new pull request, #705:
URL: https://github.com/apache/datafusion-comet/pull/705
## Which issue does this PR close?
Part of https://github.com/apache/datafusion-comet/issues/679 and https://github.com/apache/datafusion-comet/issues/670
Related https://github.com/apache/datafusion-comet/pull/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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#discussion_r1687313607
##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -100,17 +102,21 @@ public void close() {
}
private void initialize() {
- switch (values.getValueVector().getMinorType()) {
+ ValueVector vector = values.getValueVector();
+ switch (vector.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);
+ if (values.useDecimal128
Review Comment:
It is true that this is an optimization only for the integer decimal case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#discussion_r1688389477
##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -100,17 +102,21 @@ public void close() {
}
private void initialize() {
- switch (values.getValueVector().getMinorType()) {
+ ValueVector vector = values.getValueVector();
+ switch (vector.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);
+ if (values.useDecimal128
Review Comment:
I think this is still worth it. At the moment we are likely to see a fair amount of usage with only scan enabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura merged PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#discussion_r1688486187
##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -100,17 +102,21 @@ public void close() {
}
private void initialize() {
- switch (values.getValueVector().getMinorType()) {
+ ValueVector vector = values.getValueVector();
+ switch (vector.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);
+ if (values.useDecimal128
Review Comment:
I can probably move this logic to the reader side, so that it helps for native exec scenario as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#discussion_r1688697138
##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -83,6 +82,18 @@ public byte[] decodeToBinary(int index) {
case FIXEDSIZEBINARY:
return values.getBinary(index);
case DECIMAL:
+ if (binaries == null) {
Review Comment:
I wonder if this need to be made thread safe?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#discussion_r1688845740
##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -83,6 +82,18 @@ public byte[] decodeToBinary(int index) {
case FIXEDSIZEBINARY:
return values.getBinary(index);
case DECIMAL:
+ if (binaries == null) {
Review Comment:
I don't think this gets read in multiple threads, but even if it would be, it is overwrite, so should be fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#discussion_r1687278083
##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -100,17 +102,21 @@ public void close() {
}
private void initialize() {
- switch (values.getValueVector().getMinorType()) {
+ ValueVector vector = values.getValueVector();
+ switch (vector.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);
+ if (values.useDecimal128
Review Comment:
When we execute natively, I think `useDecimal128` will always be true, so this won't currently improve performance?
```
// Switch to use Decimal128 regardless of precision, since Arrow native execution
// doesn't support Decimal32 and Decimal64 yet.
SQLConf.get.setConfString(CometConf.COMET_USE_DECIMAL_128.key, "true")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#issuecomment-2246055198
@andygrove @parthchandra I moved the logic to the reader side, in that way, this PR will help for native exec as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#discussion_r1687267060
##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -100,17 +102,21 @@ public void close() {
}
private void initialize() {
- switch (values.getValueVector().getMinorType()) {
+ ValueVector vector = values.getValueVector();
+ switch (vector.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);
+ if (values.useDecimal128
+ || ((DecimalVector) vector).getPrecision() > Decimal.MAX_INT_DIGITS()) {
Review Comment:
`Decimal.MAX_LONG_DIGITS`, perhaps?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#discussion_r1687275670
##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -100,17 +102,21 @@ public void close() {
}
private void initialize() {
- switch (values.getValueVector().getMinorType()) {
+ ValueVector vector = values.getValueVector();
+ switch (vector.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);
+ if (values.useDecimal128
+ || ((DecimalVector) vector).getPrecision() > Decimal.MAX_INT_DIGITS()) {
Review Comment:
Yes, updated, thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#discussion_r1687282806
##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -100,17 +102,21 @@ public void close() {
}
private void initialize() {
- switch (values.getValueVector().getMinorType()) {
+ ValueVector vector = values.getValueVector();
+ switch (vector.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);
+ if (values.useDecimal128
Review Comment:
useDecimal128 is hardcoded to true in this code in `importVector`.
```
// Native execution should always have 'useDecimal128' set to true since it doesn't support
// other cases.
arrayVectors += CometVector.getVector(
importer.importVector(arrowArray, arrowSchema, dictionaryProvider),
true, // <-- useDecimal128
dictionaryProvider)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#issuecomment-2246355106
> 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: dictionary decimal vector optimization [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #705:
URL: https://github.com/apache/datafusion-comet/pull/705#issuecomment-2246649241
Merged Thanks @parthchandra @andygrove
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org