You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Sergio Pena <se...@cloudera.com> on 2014/10/31 00:43:28 UTC
Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/
-----------------------------------------------------------
Review request for hive.
Bugs: HIVE-8359
https://issues.apache.org/jira/browse/HIVE-8359
Repository: hive-git
Description
-------
The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/27404/diff/
Testing
-------
Thanks,
Sergio Pena
Re: Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
Posted by Ryan Blue <bl...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/#review61780
-----------------------------------------------------------
Ship it!
I think we should include a null in the test for array writes, but other than that this looks great. +1
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestDataWritableWriter.java
<https://reviews.apache.org/r/27404/#comment103671>
This test case should have a null value written in the array. I added it locally and the code works, but it should still be in the tests.
- Ryan Blue
On Nov. 14, 2014, 4:06 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27404/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2014, 4:06 p.m.)
>
>
> Review request for hive, Ryan Blue and Brock Noland.
>
>
> Bugs: HIVE-8359
> https://issues.apache.org/jira/browse/HIVE-8359
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
> It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
> ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestDataWritableWriter.java PRE-CREATION
> ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
> ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27404/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/
-----------------------------------------------------------
(Updated Nov. 15, 2014, 12:06 a.m.)
Review request for hive, Ryan Blue and Brock Noland.
Changes
-------
- More code refactoring to make DataWritableWriter.java more readable.
- Remove backward compatibles unit tests as they won't be used.
Bugs: HIVE-8359
https://issues.apache.org/jira/browse/HIVE-8359
Repository: hive-git
Description
-------
The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestDataWritableWriter.java PRE-CREATION
ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/27404/diff/
Testing
-------
Thanks,
Sergio Pena
Re: Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
Posted by Sergio Pena <se...@cloudera.com>.
> On Nov. 11, 2014, 11:02 p.m., Ryan Blue wrote:
> > Great job! I think this is really close. I found one bug and some minor stuff, all of which is noted below. Other than those, I have two major comments:
> >
> > First, the organization seems to duplicate code from writeFields in writeMap and writeArray, where each field of the inner group is written. As it is, this duplicates the code from writeFields and writeFields could be used instead. This also deviates from what the other writer implementations do, which is to require the schema to match a certain pattern and write exactly that pattern. See [1] for an example of what I'm talking about. But I think this was actually structured this way to work with multiple write schemas...
> >
> > The second problem is that this works with multiple write schemas. This was probably a mis-communication on my part, but the backward-compatibility rules in PARQUET-113 are for the read side, not the write side. Hive is already producing the correct structure when writing lists and maps, so we don't need to change that. The problem that PARQUET-113 addresses is reading data written by parquet-avro or parquet-thrift (or similar variants) that uses the 2-level representation. We still need to fix compatibility with that data, but it probably should be done as a separate issue.
> >
> > [1] - https://github.com/rdblue/incubator-parquet-mr/blob/PARQUET-113-nested-types-avro/parquet-avro/src/main/java/parquet/avro/AvroWriteSupport.java#L118
Thanks for the comments. I got confused about writing different schema formats. At some point during coding I treated Hive as Mysql, where you can insert data to tables, so I imagined that schemas would be different. But you're correct, this will never happen.
So, here's my plan for this:
- remove unit-tests that have different schemas that will never be used by Hive when writing
- fix DataWritableWriter.java to detect only one type of schema for the different types
- check if I can call writeFields or writeGroup from writeMap and writeList functions to reuse code. Btw, writeMap can call writeGroup with no problems, but writeList has a slight difference with writeFields. I'll let you know in the next patch
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/#review60863
-----------------------------------------------------------
On Nov. 10, 2014, 7:04 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27404/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2014, 7:04 p.m.)
>
>
> Review request for hive, Ryan Blue and Brock Noland.
>
>
> Bugs: HIVE-8359
> https://issues.apache.org/jira/browse/HIVE-8359
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
> It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
> ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestDataWritableWriter.java PRE-CREATION
> ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
> ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27404/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
Posted by Ryan Blue <bl...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/#review60863
-----------------------------------------------------------
Great job! I think this is really close. I found one bug and some minor stuff, all of which is noted below. Other than those, I have two major comments:
First, the organization seems to duplicate code from writeFields in writeMap and writeArray, where each field of the inner group is written. As it is, this duplicates the code from writeFields and writeFields could be used instead. This also deviates from what the other writer implementations do, which is to require the schema to match a certain pattern and write exactly that pattern. See [1] for an example of what I'm talking about. But I think this was actually structured this way to work with multiple write schemas...
The second problem is that this works with multiple write schemas. This was probably a mis-communication on my part, but the backward-compatibility rules in PARQUET-113 are for the read side, not the write side. Hive is already producing the correct structure when writing lists and maps, so we don't need to change that. The problem that PARQUET-113 addresses is reading data written by parquet-avro or parquet-thrift (or similar variants) that uses the 2-level representation. We still need to fix compatibility with that data, but it probably should be done as a separate issue.
[1] - https://github.com/rdblue/incubator-parquet-mr/blob/PARQUET-113-nested-types-avro/parquet-avro/src/main/java/parquet/avro/AvroWriteSupport.java#L118
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java
<https://reviews.apache.org/r/27404/#comment102276>
Minor: The size of map_values should be accessed with map_values.length, rather than value.get().length
Nit: I'm not sure what variable name conventions are for this project, but java projects usually prefer camelCase, as in mapValues and mapKeyValue
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java
<https://reviews.apache.org/r/27404/#comment102277>
When would key_pair_value be null? If this is something Hive needs support for, then we should note it with a comment. Otherwise, the violation of this assumption should be an exception because Parquet can't handle a null value here. This differs slightly from the behavior for LIST because the list will have null values encoded here. For a map, however, the key-value pair is required and should always be non-null; just the map value could be null, which is inside this layer.
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestDataWritableWriter.java
<https://reviews.apache.org/r/27404/#comment102267>
This doesn't appear to be used in the test. Was this extra group in the record intended to test the fact that Hive ignores extra columns? If so, then can you note it as a comment?
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestDataWritableWriter.java
<https://reviews.apache.org/r/27404/#comment102291>
This test should also verify that null array values are supported by adding a createNull() call in between 1 and 2.
I tried this and the test failed because the null check calls continue, which skips to the next iteration in writeArray and never calls endGroup(). Adding endGroup() there fixes it; good news!
- Ryan Blue
On Nov. 10, 2014, 11:04 a.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27404/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2014, 11:04 a.m.)
>
>
> Review request for hive, Ryan Blue and Brock Noland.
>
>
> Bugs: HIVE-8359
> https://issues.apache.org/jira/browse/HIVE-8359
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
> It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
> ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestDataWritableWriter.java PRE-CREATION
> ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
> ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27404/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/
-----------------------------------------------------------
(Updated Nov. 10, 2014, 7:04 p.m.)
Review request for hive, Ryan Blue and Brock Noland.
Changes
-------
Added more changes. Please review.
Bugs: HIVE-8359
https://issues.apache.org/jira/browse/HIVE-8359
Repository: hive-git
Description
-------
The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestDataWritableWriter.java PRE-CREATION
ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/27404/diff/
Testing
-------
Thanks,
Sergio Pena
Re: Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/
-----------------------------------------------------------
(Updated Nov. 10, 2014, 4:23 p.m.)
Review request for hive.
Changes
-------
Added unit tests for DataWritableWriter.java, and did refactoring based on Ryan comments.
Bugs: HIVE-8359
https://issues.apache.org/jira/browse/HIVE-8359
Repository: hive-git
Description
-------
The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestDataWritableWriter.java PRE-CREATION
ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/27404/diff/
Testing
-------
Thanks,
Sergio Pena
Re: Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
Posted by Ryan Blue <bl...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/#review59546
-----------------------------------------------------------
This is looking good overall, but I have a few comments about how it works below. I'd also like to see unit tests for the writeArray and, if added, writeMap methods. That would be a good way to demonstrate the ArrayWritable-of-ArrayWritables structure this relies on and also expand into some more test cases.
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java
<https://reviews.apache.org/r/27404/#comment100810>
I think this patch would benefit from some slight refactoring. The startGroup/endGroup calls have been changed so that each key-value pair or array element has its own startGroup/endGroup call. That's needed, but it seems awkward that this makes the assumptions of writeData and writeArray not match -- writeData expects startGroup and endGroup to have been called already, while writeArray does not.
Another problem is that this still doesn't check for LIST and MAP annotations. I think it would be much better to make this match the (draft) spec more closely, which would clean up the problem above. By looking for LIST or MAP, the call to writeArray could be moved to the level that wraps the repeated group. Then the startGroup/endGroup calls could be consistent.
This would also allow you to do more validation. First, you could check that the group contained by a LIST or MAP annotation actually is a repeated group and you could check whether LIST and MAP annotate the repeated group itself and adjust accordingly.
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java
<https://reviews.apache.org/r/27404/#comment100812>
It would be much easier to read and understand this section if the names were more descriptive than subValue and subType. There are also validations that should be done at this level, like checking that the key-value type in a map is named "key_value", that the key is required, etc. It would probably be worth breaking writeArray and writeMap into separate methods that made assumptions about the structure of the type, validated those assumptions, and then wrote out values with more understandable code.
- Ryan Blue
On Oct. 30, 2014, 4:43 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27404/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2014, 4:43 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-8359
> https://issues.apache.org/jira/browse/HIVE-8359
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
> It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
> ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
> ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27404/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
Posted by Sergio Pena <se...@cloudera.com>.
> On Nov. 2, 2014, 6:30 p.m., Brock Noland wrote:
> > Hi,
> >
> > Nice work! Have you verified that some of the other parquet tools can read the data generated by this patch? ex. Impala or MapReduce?
I run some tests on Impala, but it does not support complex data types, such as maps or arrays.
See http://www.cloudera.com/content/cloudera/en/documentation/cloudera-impala/latest/topics/impala_datatypes.html
Also, see this error from a query to a parquet with map data types:
ERROR: AnalysisException: Expr 'default.parquet_1.avreau_col_1' in select list of root statement returns a complex type 'MAP<STRING,STRING>'.
Only scalar types are allowed in the select list of the root statement.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/#review59527
-----------------------------------------------------------
On Oct. 30, 2014, 11:43 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27404/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2014, 11:43 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-8359
> https://issues.apache.org/jira/browse/HIVE-8359
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
> It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
> ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
> ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27404/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 27404: HIVE-8359 Map containing null values are not
correctly written in Parquet files
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27404/#review59527
-----------------------------------------------------------
Hi,
Nice work! Have you verified that some of the other parquet tools can read the data generated by this patch? ex. Impala or MapReduce?
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java
<https://reviews.apache.org/r/27404/#comment100799>
Please remove this import change. We typically don't want to do wildcard imports. I am guessing this was done by your IDE
- Brock Noland
On Oct. 30, 2014, 11:43 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27404/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2014, 11:43 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-8359
> https://issues.apache.org/jira/browse/HIVE-8359
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch changes the way DataWritableWriter class writes an array of elements to the Parquet record.
> It wraps each array record into a new startGroup/endGroup block so that Parquet can detect null values on those optional fields.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java c7078efe27482df0a11dd68ac068da27dbcf51b3
> ql/src/test/queries/clientpositive/parquet_map_null.q PRE-CREATION
> ql/src/test/results/clientpositive/parquet_map_null.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27404/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>