Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9403 )

Change subject: IMPALA-6324: Support reading RLE-encoded boolean values in 
Parquet scanner

Patch Set 3:


The overall approach looks good, had some comments about the details and the 
test coverage.

File be/src/exec/parquet-column-readers.cc:

PS3, Line 673:     switch(page_encoding_) {
nit: space before (

PS3, Line 1304:   stringstream ss;
Convert to using Substitute() while you're here - we generally prefer using it 
over stringstream.

File be/src/util/rle-encoding.h:

PS3, Line 138:   /// Returns the number of consumed values or 0 if an error 
Can you add some tests to be/src/util/rle-test.cc for the new interface? There 
are already some tests there that exercise both GetSingleValue() and the 
NextNum*/Get*Values() interfaces, so we could also add a test for this variant 
of the interface.

PS3, Line 674:        memset(values + num_consumed, repeated_value, 
It's not valid to use memset() here in general since sizeof(T) may be > 1. The 
reason it worked for the Parquet level decoding was because they were always a 

In general we need a for loop here to do it correctly, but my understanding was 
that memset() is sometimes faster - it was used as an optimisation. I see two 
paths forward here:
1. maybe gcc converts the for loop to a memset() anyway when sizeof(T) is 1 
bytes (at least some compilers can do this conversion if the code fits the 
right pattern).
2. do an if(sizeof(T) == 1) here so that we can use the optimized memset() path 
in that case, then fall back to a for loop otherwise.

File testdata/data/README:

PS3, Line 160: Parquet v1 file with a single RLE encoded boolean column "b". 
Created for IMPALA-6324.
How was it created? Parquet-mr?


PS3, Line 4: select count(*) from rle_encoded_bool where b;
This wouldn't detect if the boolean values were returned in the wrong order. 
E.g. if we somehow got the bit-packing backwards and the values were in the 
wrong order (we had a long-standing bug with the BIT_PACKED levels).

Can we also add a test with two columns (e.g. id and boolean) and make sure 
that they match up correctly.

File tests/query_test/test_scanners.py:

PS3, Line 656:     """
Nit: trailing """ can go on same line.

To view, visit http://gerrit.cloudera.org:8080/9403
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4644bf8cf5d2b7238b05076407fbf78ab5d2c14f
Gerrit-Change-Number: 9403
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 23:20:10 +0000
Gerrit-HasComments: Yes

Reply via email to