Fix optimized parquet reader complex hive types processing by kgalieva · Pull Request #9156 · prestodb/presto

kgalieva · 2017-10-15T22:31:05Z

Fix reading repeated fields, when parquet consists of multiple pages,
so the beginning of the field can be on one page
and it's ending on the next page.
Support empty arrays read
Determine null values of optional fields
Add tests for hive complex types: arrays, maps and structs
Rewrite tests to read parquets consising of multiple pages
Add TestDataWritableWriter with patch for empty array and empty map
because the bug https://issues.apache.org/jira/browse/HIVE-13632
is already fixed in current hive version,
so presto should be able to read empty arrays too

kgalieva · 2017-10-16T13:12:33Z

@nezihyigitbasi could you please take a look

nezihyigitbasi · 2017-10-16T17:24:15Z

@kgalieva PR #9110 also fixes the bugs in reading repeated fields. I will first review that and then we can take a look at this one.

kgalieva · 2017-10-23T22:32:34Z

@nezihyigitbasib Any comments or suggestions?

nezihyigitbasi · 2017-10-23T22:47:25Z

@kgalieva does this patch handle arbitrary level of nesting for complex types?

kgalieva · 2017-10-24T15:04:45Z

@nezihyigitbasi yes it does

nezihyigitbasi

I made a quick pass and left some comments/questions. I will take a detailed look in the second round.

nezihyigitbasi · 2017-10-24T17:14:41Z

...to-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetBinaryColumnReader.java

what does this change solve? how abut the cases where valueIsNull() == false (the else part)?

For non required fields there are three options:

Value is defined

Value is null

Value does not exist, because one of it's optional parent fields is null.

nezihyigitbasi · 2017-10-24T17:17:33Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetColumnReader.java

please add a comment that explains why definitionLevel == columnDescriptor.getMaxDefinitionLevel() - 1 indicates a null value.

nezihyigitbasi · 2017-10-24T17:19:55Z

...-hive/src/test/java/com/facebook/presto/hive/parquet/write/TestDataWritableWriteSupport.java

we don't mark method parameters as final.

This class is a copy of https://github.com/apache/parquet-mr/blob/master/parquet-hive/parquet-hive-storage-handler/src/main/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriteSupport.java
The only difference is the writer. I added copy of org.apache.hadoop.hive.ql.io.parquet.write.DataWritableWriter with patch for empty array and empty map (https://issues.apache.org/jira/browse/HIVE-13632) to be able to test how optimised parquet reader processes empty arrays/maps.
If you insist, I will apply the changes suggested below. But I'd rather leave those classes as they are defined in hive with just one necessary patch.

OK then please add a class level javadoc saying that we copied this file from this and that etc. and leave this as is.

nezihyigitbasi · 2017-10-24T17:26:26Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetReader.java

You can define int lastOffset = offsets.get(offsets.size() - 1); and update usages below.

nezihyigitbasi · 2017-10-24T17:27:16Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetReader.java

Can you explain what valueOffset tracks here?

refactored this method. Here valueOffset used to track offset(number of elements) of the array or map being processed. In case when array/map is defined and not empty, offset value is increased by the number of elements in a collection.

nezihyigitbasi · 2017-10-24T18:31:31Z

presto-hive/src/test/java/com/facebook/presto/hive/parquet/write/TestDataWritableWriter.java

prec -> precision

the same as above, need your opinion regarding updating copy of Hive Parquet writer

nezihyigitbasi · 2017-10-24T18:31:57Z

presto-hive/src/test/java/com/facebook/presto/hive/parquet/write/TestDataWritableWriter.java

tgt -> bytes or target ?

the same as above, need your opinion regarding updating copy of Hive Parquet writer

nezihyigitbasi · 2017-10-24T18:36:39Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetColumnReader.java

this method looks similar to skipValues (the core read logic is the same), can we merge the two somehow so that we get rid of code duplication?

agree, refactored

nezihyigitbasi · 2017-10-24T18:37:33Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetColumnReader.java

what is the reason behind this change?

this line was moved to skipValues method

nezihyigitbasi · 2017-10-24T18:50:39Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetReader.java

this part of the code looks error prone, please add some comments/links to the relevant parts of the spec that explains what you do here.

kgalieva · 2017-10-30T22:19:17Z

@nezihyigitbasi Made changes you suggested in comments. Please review one more time when you have time :)

kgalieva · 2017-11-03T15:56:52Z

@nezihyigitbasi any review updates on this PR?

nezihyigitbasi

thanks @kgalieva! I made another pass and left some more comments. This is getting close.

nezihyigitbasi · 2017-11-07T19:31:08Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetTypeUtils.java

!isRequired && (definitionLevel == maxDefinitionLevel - 1) will make this easier to read.

nezihyigitbasi · 2017-11-07T19:31:33Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetTypeUtils.java

it's -> its

nezihyigitbasi · 2017-11-07T19:32:56Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetTypeUtils.java

isRequired -> required

nezihyigitbasi · 2017-11-07T19:33:47Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetTypeUtils.java

rename isNullField -> isValueNull

nezihyigitbasi · 2017-11-07T19:39:09Z

...to-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetBinaryColumnReader.java

what happens when definitionLevel != columnDescriptor.getMaxDefinitionLevel() && !isNullValue()? If that's not a valid case please add a final else and throw an appropriate exception.

@nezihyigitbasi This is a valid case. Value can be undefined when it belongs to block under some nested structure.
For primitive columns it's true, that their value either defined or null.
For columns with nested structure it's different, because null can be at any level of optional field.

For example, column can be a struct with two nested optional fields A.B
In this case:
A can be defined or null
B can be defined, null or not defined (meaning A was null).

Presto spi blocks contain only defined and null values. So when block with values for B field is being written, cases where A field is null need to skipped.

When complex type is being decoded, be it RowType.getObjectValue(), ArrayType.getObjectValue() or MapType.getObjectValue() they all first check parent value for being not null and only if it's not, decode nested values from underlying blocks.
This means, that value from block B will be read only if corresponding parent field A value was not null.

nezihyigitbasi · 2017-11-07T20:22:56Z

presto-hive/src/test/java/com/facebook/presto/hive/parquet/AbstractTestParquetReader.java

testArrayOfMaps

nezihyigitbasi · 2017-11-07T20:24:17Z

presto-hive/src/test/java/com/facebook/presto/hive/parquet/AbstractTestParquetReader.java

please rename as this is confusing. maybe just testStructOfMaps

nezihyigitbasi · 2017-11-07T20:31:22Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetReader.java

the parameter list is huge here so let's reformat this as:

private void calculateStructOffsets( String[] path, IntList structOffsets, BooleanList structIsNull, IntList definitionLevels, IntList repetitionLevels, IntList fieldDefinitionLevels, IntList fieldRepetitionLevels)

nezihyigitbasi · 2017-11-07T20:32:06Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetReader.java

definitionLevels and repetitionLevels are for the struct I guess, so let's rename them as such. e.g., structDefinitionLevels ...

nezihyigitbasi · 2017-11-07T20:32:57Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetReader.java

please rename i to be more descriptive.

nezihyigitbasi · 2017-11-08T21:44:48Z

there are compilation failures.

nezihyigitbasi · 2018-04-27T20:36:24Z

@zhenxiao @kgalieva were you able to fix the issue? Let me know when you are done, so that we can proceed with this.

zhenxiao · 2018-04-27T20:38:02Z

sorry for the late reply. I will send parquet data to reproduce the problem to @kgalieva

kgalieva · 2018-04-27T22:18:10Z

Thanks @zhenxiao! When do you think you will be able to send me a parquet file?

meghapthakkar · 2018-04-30T22:34:44Z

Any updates on this PR?

kgalieva · 2018-05-01T02:53:12Z

@meghapthakkar unfortunately, no. I'm still waiting for @zhenxiao to send me the testing data to reproduce his problem.

kgalieva · 2018-05-04T22:53:45Z

Hi @zhenxiao, just a friendly reminder about the file :)

kgalieva · 2018-05-15T11:14:38Z

Hi @zhenxiao! Any updates from you?

Parth-Brahmbhatt · 2018-05-22T20:31:10Z

We at Netflix has been running with this patch for over 3 months now. The few fixes that we had to make are all part of the updated patch. I would recommend to merge this in.

zhenxiao · 2018-05-22T20:43:15Z

sorry for the late reply. I will do one more round test, with our highly nested data. @kgalieva will send you parquet file to reproduce problems, or greenlight.

kgalieva · 2018-05-22T20:55:18Z

@zhenxiao @Parth-Brahmbhatt Thank you very much for your help with testing it! @zhenxiao I would really appreciate your feedback and will be happy to check your test cases! :)

nezihyigitbasi

@kgalieva I left some more comments. Thanks again for working on this.
@Parth-Brahmbhatt Thanks for your input and testing this patch.
@zhenxiao Please provide feedback asap as we want to merge this soon.

This has been open for quite some time and I want to merge this as soon as possible. Thanks for everyone's patience.

nezihyigitbasi · 2018-05-22T21:34:47Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/Field.java

requireNonNull(type, "type is required");

nezihyigitbasi · 2018-05-22T21:35:01Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/GroupField.java

requireNonNull(children, "children is required");

nezihyigitbasi · 2018-05-22T21:35:40Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/GroupField.java

return ImmutableList.copyOf(children);

Changed children type to ImmutableList instead, to make it obvious that copying is not needed

nezihyigitbasi · 2018-05-22T21:37:34Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSource.java

We can use ImmutableList.Builder<Optional<Field>> fieldsBuilder = ImmutableList.builder(); to be consistent with the other fields.

nezihyigitbasi · 2018-05-22T21:40:28Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSource.java

What's the reason behind removing the systemMemoryContext field? It seems unrelated to the purpose of the PR. Same for the dataSource field.

ParquetPageSource class has a lot of constructor parameters. To simplify it a little bit it is possible to get systemMemoryContext and dataSource from parquetReader.

nezihyigitbasi · 2018-05-22T21:57:02Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetReader.java

static import calculateCollectionOffsets.

nezihyigitbasi · 2018-05-22T21:57:52Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetReader.java

Same question about optional above applies to here and to L159 below.

This is because an array always has a single inner element and a map always has two inner elements (key and value). We do isPresent check for structs because some parameters can be missing due to schema evolution.

nezihyigitbasi · 2018-05-22T22:00:33Z

presto-hive/src/main/java/parquet/io/ColumnIOConverter.java

static import OPTIONAL

nezihyigitbasi · 2018-05-22T22:01:04Z

presto-hive/src/main/java/parquet/io/ColumnIOConverter.java

namedTypeSignature.getName() is Optional so get call may fail.

Struct fields are always named (unlike, for instance, array and map inner elements). This is why we can safely call get() here.

nezihyigitbasi · 2018-05-22T22:01:36Z

presto-hive/src/main/java/parquet/io/ColumnIOConverter.java

this else is unnecessary, we can just return.

zhenxiao

a few minor things during my testing

zhenxiao · 2018-05-22T22:09:58Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/predicate/ParquetPredicateUtils.java

following Presto coding style, all parameters in the same line

zhenxiao · 2018-05-22T22:10:23Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/predicate/ParquetPredicateUtils.java

following Presto coding style, all parameters in the same line

zhenxiao · 2018-05-22T22:58:32Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/reader/ParquetReader.java

group final variables and non-final variables

- Fix reading repeated fields, when parquet consists of multiple pages, so the beginning of the field can be on one page and it's ending on the next page. - Support empty arrays read - Determine null values of optional fields - Add tests for hive complex types: arrays, maps and structs - Rewrite tests to read parquets consising of multiple pages - Add TestDataWritableWriter with patch for empty array and empty map because the bag https://issues.apache.org/jira/browse/HIVE-13632 is already fixed in current hive version, so presto should be able to read empty arrays too - Backward-compatibility rules support for arrays https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists - Backward-compatibility rules support for maps https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps

kgalieva · 2018-05-24T01:51:37Z

@nezihyigitbasi @zhenxiao thank you the feedback! I've made the changes you suggested and replied to comments you left.

nezihyigitbasi · 2018-05-24T16:45:42Z

Merged, thanks @kgalieva. Also thanks everyone for their contributions and patience.

zhenxiao · 2018-05-26T18:19:20Z

this patch works good on our highly nested data, it passed 2 day's production workloads, very nice contribution @kgalieva

zhenxiao · 2018-06-15T19:33:05Z

Hi @kgalieva we found a problem for reading a table of schema:

"_hoodie_commit_time","varchar","",""
"_hoodie_commit_seqno","varchar","",""
"_hoodie_record_key","varchar","",""
"_hoodie_partition_path","varchar","",""
"_hoodie_file_name","varchar","",""
"ts","double","",""
"host","varchar","",""
"level","varchar","",""
"dc","varchar","",""
"msg_offset","bigint","",""
"uuid","varchar","",""
"schema_id","integer","",""
"msg","row(menuentityuuid varchar, createdat bigint, updatedat bigint, menuentitytype varchar, title row(defaultvalue varchar, overrides array(row(contexttype varchar, contextvalue varchar, overriddenvalue varchar))), description row(defaultvalue varchar, overrides array(row(contexttype varchar, contextvalue varchar, overriddenvalue varchar))), childmenuentityuuids row(defaultvalue array(varchar), overrides array(row(contexttype varchar, contextvalue varchar, overriddenvalue array(varchar)))), prerequisiteoptionuuids row(defaultvalue array(varchar), overrides array(row(contexttype varchar, contextvalue varchar, overriddenvalue array(varchar)))), tags row(tags array(row(uuid varchar, key varchar, name varchar, type varchar))), vendorinfo row(externalid varchar, externaldata varchar), paymentinfo row(priceinfo row(defaultvalue row(price bigint), overrides array(row(contexttype varchar, contextvalue varchar, overriddenvalue row(price bigint))))), suspensioninfo row(defaultvalue row(suspenduntil bigint, suspendreason varchar), overrides array(row(contexttype varchar, contextvalue varchar, overriddenvalue row(suspenduntil bigint, suspendreason varchar)))), classifications row(cuisineuuid varchar, mealtypeuuid varchar, proteintypeuuid varchar, hasside boolean, ishot boolean, isentree boolean, alcoholicitems bigint), nutritionalinfo row(allergens array(varchar), caloricinfo row(lowerrange bigint, higherrange bigint, displaytype varchar), jouleinfo row(lowerrange bigint, higherrange bigint, displaytype varchar)), badges row(badges array(row(type varchar, iconurl varchar, text varchar))), taxinfo row(taxrate double, vatrate double), options row(options array(row(uuid varchar))), comboentities row(entities array(row(uuid varchar))), image row(imageurl varchar, rawimageurl varchar), quantityinfo row(defaultvalue row(minpermitted bigint, maxpermitted bigint, defaultquantity bigint, chargeabove bigint, refundunder bigint), overrides array(row(contexttype varchar, contextvalue varchar, overriddenvalue row(minpermitted bigint, maxpermitted bigint, defaultquantity bigint, chargeabove bigint, refundunder bigint)))), deletedat bigint, storeuuid varchar, isfrombulkupdate boolean, eventuuid varchar, isinsubsections boolean, insubsectionuuids array(varchar), notes varchar)","",""
"hadoop_row_key","varchar","",""
"hadoop_ref_key","bigint","",""
"hadoop_ref_key_version","integer","",""
"hadoop_data_source","varchar","",""
"hadoop_isdeleted","boolean","",""
"hadoop_isdeleted_uname","varchar","",""
"hadoop_forceharddelete","boolean","",""
"hadoop_forceupdate","boolean","",""
"hadoop_timestamp","bigint","",""
"hadoop_datacenter","varchar","",""
"hadoop_host","varchar","",""
"hadoop_schema_version","integer","",""
"hadoop_message_offset","bigint","",""
"hadoop_row_partition_datetime_str","varchar","",""
"jaeger_context","varbinary","",""
"datestr","varchar","partition key",""

Seems related to reading of: array<structtype:string>

Could you please help take a look?

I sent you an email with attached parquet file.

zhenxiao · 2018-06-16T00:08:08Z

get some clue for it. @nezihyigitbasi @kgalieva Will send to you for review.

zhenxiao · 2018-06-16T01:07:00Z

a fix:
#10849

facebook-github-bot added the CLA Signed label Oct 15, 2017

nezihyigitbasi self-assigned this Oct 16, 2017

nezihyigitbasi mentioned this pull request Oct 23, 2017

Fixing presto native parquet read path for parquet types with repetit… #9110

Closed

nezihyigitbasi reviewed Oct 24, 2017

View reviewed changes

kgalieva closed this Oct 27, 2017

kgalieva reopened this Oct 27, 2017

kgalieva closed this Oct 28, 2017

kgalieva reopened this Oct 28, 2017

kgalieva closed this Oct 28, 2017

kgalieva reopened this Oct 28, 2017

kgalieva force-pushed the complex-hive-types branch 3 times, most recently from e27d714 to d3e02a6 Compare October 28, 2017 22:13

nezihyigitbasi mentioned this pull request Oct 31, 2017

Fix "Reading past RLE/BitPacking stream" when querying array columns from parquets #8289

Closed

nezihyigitbasi requested changes Nov 7, 2017

View reviewed changes

kgalieva force-pushed the complex-hive-types branch from d3e02a6 to 47ecdc7 Compare November 8, 2017 20:24

kgalieva force-pushed the complex-hive-types branch from 47ecdc7 to 520afc6 Compare November 8, 2017 21:46

kgalieva closed this Nov 8, 2017

kgalieva reopened this Nov 8, 2017

kgalieva force-pushed the complex-hive-types branch from 520afc6 to 5dcf82b Compare November 8, 2017 23:17

kgalieva closed this Nov 9, 2017

kgalieva reopened this Nov 9, 2017

nezihyigitbasi reviewed May 22, 2018

View reviewed changes

zhenxiao reviewed May 23, 2018

View reviewed changes

kgalieva force-pushed the complex-hive-types branch 3 times, most recently from 9544e17 to 58d395b Compare May 24, 2018 00:09

kgalieva force-pushed the complex-hive-types branch from 58d395b to 592b74a Compare May 24, 2018 00:31

nezihyigitbasi closed this May 24, 2018

This was referenced May 26, 2018

New Parquet Reader: Error while reading struct with primitive type and complex type #8133

Closed

Parquet reader doesn't support projection pushdown past the first level #2508

Closed

qqibrow mentioned this pull request Jun 1, 2018

pushdown dereference expression #10064

Closed

This was referenced Jul 23, 2018

Parquet optimized reader can't read ARRAY type column #10038

Closed

Parquet predicate pushdown tests are broken. #10009

Closed

bhasudha mentioned this pull request Feb 20, 2020

presto - querying nested object in parquet file created by hudi apache/hudi#1325

Closed

Conversation

kgalieva commented Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kgalieva commented Oct 16, 2017

Uh oh!

nezihyigitbasi commented Oct 16, 2017

Uh oh!

kgalieva commented Oct 23, 2017

Uh oh!

nezihyigitbasi commented Oct 23, 2017

Uh oh!

kgalieva commented Oct 24, 2017

Uh oh!

nezihyigitbasi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgalieva Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgalieva commented Oct 30, 2017

Uh oh!

kgalieva commented Nov 3, 2017

Uh oh!

nezihyigitbasi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgalieva commented Oct 15, 2017 •

edited

Loading

kgalieva Oct 27, 2017 •

edited

Loading