Skip to content

Conversation

@liancheng
Copy link
Contributor

This PR is inspired by #8063 authored by @dguy. Especially, testing Parquet files added here are all taken from that PR.

Committer who merges this PR should attribute it to Damian Guy <damian.guy@gmail.com>.


SPARK-6776 and SPARK-6777 followed parquet-avro to implement backwards-compatibility rules defined in parquet-format spec. However, both Spark SQL and parquet-avro neglected the following statement in parquet-format:

This does not affect repeated fields that are not annotated: A repeated field that is neither contained by a LIST- or MAP-annotated group nor annotated by LIST or MAP should be interpreted as a required list of required elements where the element type is the type of the field.

One of the consequences is that, Parquet files generated by parquet-protobuf containing unannotated repeated fields are not correctly converted to Catalyst arrays.

This PR fixes this issue by

  1. Handling unannotated repeated fields in CatalystSchemaConverter.

  2. Converting this kind of special repeated fields to Catalyst arrays in CatalystRowConverter.

    Two special converters, RepeatedPrimitiveConverter and RepeatedGroupConverter, are added. They delegate actual conversion work to a child elementConverter and accumulates elements in an ArrayBuffer.

    Two extra methods, start() and end(), are added to ParentContainerUpdater. So that they can be used to initialize new ArrayBuffers for unannotated repeated fields, and propagate converted array values to upstream.

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #40299 has finished for PR 8070 at commit a11b7c0.

  • This patch fails RAT tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #40300 has finished for PR 8070 at commit 1c68b55.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@liancheng liancheng changed the title [SPARK-9340] [SQ] Fixes converting unannotated Parquet lists [SPARK-9340] [SQL] Fixes converting unannotated Parquet lists Aug 10, 2015
@dguy
Copy link
Contributor

dguy commented Aug 10, 2015

LGTM - working as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this a CatalystPrimitiveConverter with RepeatedConverter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because CatalystPrimitiveConverter is defined as:

private[parquet] class CatalystPrimitiveConverter(val updater: ParentContainerUpdater)
  extends PrimitiveConverter with HasParentContainerUpdater {
  ...
}

the val updater part has two meanings:

  1. updater is made a constructor argument, and
  2. def updater in HasParentContainerUpdater is overriden since updater is a read-only val.

The 2nd fact prevents subclasses of CatalystPrimitiveConverter to override the updater field.

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #40304 has finished for PR 8070 at commit 450b606.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rdblue
Copy link
Contributor

rdblue commented Aug 10, 2015

This looks good to me overall.

@liancheng liancheng force-pushed the spark-9340/unannotated-parquet-list branch from 450b606 to ace6df7 Compare August 11, 2015 00:03
@liancheng
Copy link
Contributor Author

@dguy @rdblue Thanks for the review! I just rebased this PR to resolve conflicts introduced by #8056. Will merge this pending Jenkins.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #40355 has finished for PR 8070 at commit ace6df7.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

asfgit pushed a commit that referenced this pull request Aug 11, 2015
This PR is inspired by #8063 authored by dguy. Especially, testing Parquet files added here are all taken from that PR.

**Committer who merges this PR should attribute it to "Damian Guy <damian.guygmail.com>".**

----

SPARK-6776 and SPARK-6777 followed `parquet-avro` to implement backwards-compatibility rules defined in `parquet-format` spec. However, both Spark SQL and `parquet-avro` neglected the following statement in `parquet-format`:

> This does not affect repeated fields that are not annotated: A repeated field that is neither contained by a `LIST`- or `MAP`-annotated group nor annotated by `LIST` or `MAP` should be interpreted as a required list of required elements where the element type is the type of the field.

One of the consequences is that, Parquet files generated by `parquet-protobuf` containing unannotated repeated fields are not correctly converted to Catalyst arrays.

This PR fixes this issue by

1. Handling unannotated repeated fields in `CatalystSchemaConverter`.
2. Converting this kind of special repeated fields to Catalyst arrays in `CatalystRowConverter`.

   Two special converters, `RepeatedPrimitiveConverter` and `RepeatedGroupConverter`, are added. They delegate actual conversion work to a child `elementConverter` and accumulates elements in an `ArrayBuffer`.

   Two extra methods, `start()` and `end()`, are added to `ParentContainerUpdater`. So that they can be used to initialize new `ArrayBuffer`s for unannotated repeated fields, and propagate converted array values to upstream.

Author: Cheng Lian <lian@databricks.com>

Closes #8070 from liancheng/spark-9340/unannotated-parquet-list and squashes the following commits:

ace6df7 [Cheng Lian] Moves ParquetProtobufCompatibilitySuite
f1c7bfd [Cheng Lian] Updates .rat-excludes
420ad2b [Cheng Lian] Fixes converting unannotated Parquet lists

(cherry picked from commit 071bbad)
Signed-off-by: Cheng Lian <lian@databricks.com>
@liancheng
Copy link
Contributor Author

Merged to master and branch-1.5.

@asfgit asfgit closed this in 071bbad Aug 11, 2015
@liancheng liancheng deleted the spark-9340/unannotated-parquet-list branch August 11, 2015 04:49
CodingCat pushed a commit to CodingCat/spark that referenced this pull request Aug 17, 2015
This PR is inspired by apache#8063 authored by dguy. Especially, testing Parquet files added here are all taken from that PR.

**Committer who merges this PR should attribute it to "Damian Guy <damian.guygmail.com>".**

----

SPARK-6776 and SPARK-6777 followed `parquet-avro` to implement backwards-compatibility rules defined in `parquet-format` spec. However, both Spark SQL and `parquet-avro` neglected the following statement in `parquet-format`:

> This does not affect repeated fields that are not annotated: A repeated field that is neither contained by a `LIST`- or `MAP`-annotated group nor annotated by `LIST` or `MAP` should be interpreted as a required list of required elements where the element type is the type of the field.

One of the consequences is that, Parquet files generated by `parquet-protobuf` containing unannotated repeated fields are not correctly converted to Catalyst arrays.

This PR fixes this issue by

1. Handling unannotated repeated fields in `CatalystSchemaConverter`.
2. Converting this kind of special repeated fields to Catalyst arrays in `CatalystRowConverter`.

   Two special converters, `RepeatedPrimitiveConverter` and `RepeatedGroupConverter`, are added. They delegate actual conversion work to a child `elementConverter` and accumulates elements in an `ArrayBuffer`.

   Two extra methods, `start()` and `end()`, are added to `ParentContainerUpdater`. So that they can be used to initialize new `ArrayBuffer`s for unannotated repeated fields, and propagate converted array values to upstream.

Author: Cheng Lian <lian@databricks.com>

Closes apache#8070 from liancheng/spark-9340/unannotated-parquet-list and squashes the following commits:

ace6df7 [Cheng Lian] Moves ParquetProtobufCompatibilitySuite
f1c7bfd [Cheng Lian] Updates .rat-excludes
420ad2b [Cheng Lian] Fixes converting unannotated Parquet lists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants