[enhancement] [release-note] Update get bytes to return raw bytes of string and support getBytesMV #9441

61yao · 2022-09-20T18:11:08Z

This change is backward incompatible:

Change get bytes behavior to return raw bytes directly.

This doesn't impact the values that are originally stored as bytes
If the values are stored as strings and read as bytes, in the past, we returned the hex encoded bytes for the string value. This PR changes the behavior to return utf8 bytes directly.

Add support to getBytesMV

codecov-commenter · 2022-09-20T19:08:56Z

Codecov Report

Merging #9441 (2ac7f0a) into master (b043dd8) will increase coverage by 6.30%.
The diff coverage is 27.27%.

@@             Coverage Diff              @@
##             master    #9441      +/-   ##
============================================
+ Coverage     63.53%   69.84%   +6.30%     
- Complexity     5072     5136      +64     
============================================
  Files          1836     1910      +74     
  Lines         98545   101832    +3287     
  Branches      15062    15451     +389     
============================================
+ Hits          62614    71122    +8508     
+ Misses        31313    25699    -5614     
- Partials       4618     5011     +393

Flag	Coverage Δ
integration1	`26.09% <2.27%> (?)`
integration2	`24.72% <0.00%> (?)`
unittests1	`67.08% <27.27%> (+0.01%)`	⬆️
unittests2	`15.48% <0.00%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...a/org/apache/pinot/core/common/DataBlockCache.java	`89.44% <0.00%> (-1.47%)`	⬇️
...ore/operator/docvalsets/ProjectionBlockValSet.java	`86.48% <0.00%> (-3.66%)`	⬇️
...core/operator/docvalsets/TransformBlockValSet.java	`59.43% <0.00%> (+4.09%)`	⬆️
...ator/transform/function/BaseTransformFunction.java	`75.80% <0.00%> (+0.03%)`	⬆️
...ransform/function/IdentifierTransformFunction.java	`77.08% <0.00%> (-1.65%)`	⬇️
...r/transform/function/LiteralTransformFunction.java	`70.19% <0.00%> (+1.26%)`	⬆️
...e/pinot/core/query/reduce/RowBasedBlockValSet.java	`15.71% <0.00%> (-0.23%)`	⬇️
...pache/pinot/segment/local/io/util/ValueReader.java	`100.00% <ø> (ø)`
...ot/segment/local/io/util/VarLengthValueReader.java	`72.97% <0.00%> (-2.03%)`	⬇️
.../dictionary/BigDecimalOnHeapMutableDictionary.java	`34.83% <0.00%> (ø)`
... and 502 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Jackie-Jiang

Mostly minor comments

pinot-core/src/main/java/org/apache/pinot/core/common/DataBlockCache.java

pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java

Jackie-Jiang · 2022-09-21T22:10:17Z

pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java

-            }
-            break;
          case BYTES:
+          case STRING:


We can remove the switch and directly read the values

I thought we don't want to support this for other types?

pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java

...a/org/apache/pinot/segment/local/realtime/impl/dictionary/StringOnHeapMutableDictionary.java

Jackie-Jiang · 2022-09-21T22:48:38Z

...c/main/java/org/apache/pinot/segment/local/segment/index/readers/OnHeapStringDictionary.java

      String unpaddedString = getUnpaddedString(i, buffer);
      _unpaddedStrings[i] = unpaddedString;
      _unPaddedStringToIdMap.put(unpaddedString, i);
+      _unpaddedBytes[i] = getUnpaddedBytes(i, buffer);


Only read the unpadded bytes, and directly create unpaddedString from the unpadded bytes for better performance

The performance will not be good because we have do encode every time if we want to read unpadded string? Or you are talking about space efficiency?

I'm talking about the bytes/string reading during the construction

Suggested change

_unpaddedBytes[i] = getUnpaddedBytes(i, buffer);

byte[] unpaddedBytes = getUnpaddedBytes(i, buffer);

String unpaddedString = new String(unpaddedBytes, UTF_8);

...

...cal/src/main/java/org/apache/pinot/segment/local/segment/index/readers/StringDictionary.java

...egment-spi/src/main/java/org/apache/pinot/segment/spi/index/mutable/MutableForwardIndex.java

pinot-spi/src/main/java/org/apache/pinot/spi/utils/BytesUtils.java

Jackie-Jiang

LGTM otherwise

pinot-core/src/main/java/org/apache/pinot/core/common/DataBlockCache.java

pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java

pinot-core/src/test/java/org/apache/pinot/core/common/DataFetcherTest.java

Jackie-Jiang · 2022-09-23T22:25:56Z

...c/main/java/org/apache/pinot/segment/local/segment/index/readers/OnHeapStringDictionary.java

      String unpaddedString = getUnpaddedString(i, buffer);
      _unpaddedStrings[i] = unpaddedString;
      _unPaddedStringToIdMap.put(unpaddedString, i);
+      _unpaddedBytes[i] = getUnpaddedBytes(i, buffer);


I'm talking about the bytes/string reading during the construction

Suggested change

_unpaddedBytes[i] = getUnpaddedBytes(i, buffer);

byte[] unpaddedBytes = getUnpaddedBytes(i, buffer);

String unpaddedString = new String(unpaddedBytes, UTF_8);

...

.../apache/pinot/segment/local/segment/index/readers/ImmutableDictionaryTypeConversionTest.java

...-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java

...e/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java

…string and support getBytesMV (apache#9441) * get raw bytes for string value column * get raw bytes for string value column * support getBytesMV * fix fail test * fix style * Address sytle commets * address comments

get raw bytes for string value column

4313b32

61yao force-pushed the get_bytes branch from b897671 to 2cb5064 Compare September 20, 2022 18:21

get raw bytes for string value column

1bde84f

61yao force-pushed the get_bytes branch from 2cb5064 to 1bde84f Compare September 20, 2022 18:23

support getBytesMV

ff703a2

61yao force-pushed the get_bytes branch from 23f8729 to ff703a2 Compare September 21, 2022 17:06

61yao changed the title ~~[not for review] Update get bytes to return raw bytes of string~~ [enhancement] Update get bytes to return raw bytes of string and support getBytesMV Sep 21, 2022

61yao changed the title ~~[enhancement] Update get bytes to return raw bytes of string and support getBytesMV~~ [enhancement] [release-note] Update get bytes to return raw bytes of string and support getBytesMV Sep 21, 2022

Yao Liu added 2 commits September 21, 2022 14:19

fix fail test

ed2118c

fix style

ea1ebdd

Jackie-Jiang reviewed Sep 21, 2022

View reviewed changes

Jackie-Jiang added enhancement release-notes Referenced by PRs that need attention when compiling the next release notes backward-incompat Referenced by PRs that introduce or fix backward compat issues labels Sep 21, 2022

Address sytle commets

0826d0c

Jackie-Jiang approved these changes Sep 23, 2022

View reviewed changes

walterddr approved these changes Sep 26, 2022 •

edited

Loading

View reviewed changes

address comments

2ac7f0a

walterddr merged commit 3818397 into apache:master Sep 27, 2022

Jackie-Jiang mentioned this pull request Nov 30, 2022

Make it possible to read strings from Dictionary as byte[] #8094

Closed

-      _unpaddedBytes[i] = getUnpaddedBytes(i, buffer);
+     byte[] unpaddedBytes = getUnpaddedBytes(i, buffer);
+     String unpaddedString = new String(unpaddedBytes, UTF_8);
+     ...

[enhancement] [release-note] Update get bytes to return raw bytes of string and support getBytesMV #9441

[enhancement] [release-note] Update get bytes to return raw bytes of string and support getBytesMV #9441

Uh oh!

Conversation

61yao commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

61yao Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

61yao Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

61yao commented Sep 20, 2022 •

edited

Loading

codecov-commenter commented Sep 20, 2022 •

edited

Loading