[SPARK-9876][SQL]: Update Parquet to 1.8.1.

[rdblue]

What changes were proposed in this pull request?

This includes minimal changes to get Spark using the current release of Parquet, 1.8.1.

How was this patch tested?

This uses the existing Parquet tests.

[rxin]

in the past parquet upgrades brought perf regressions. Any idea about this release?

[SparkQA]

Test build #59214 has finished for PR 13280 at commit 022dd6b.

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

[SparkQA]

Test build #3015 has finished for PR 13280 at commit 022dd6b.

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

[rdblue]

@rxin, I agree that we shouldn't upgrade if there are perf regressions. I would like to know what they are so we can fix them in Parquet upstream though. This should be a big performance improvement for selective queries on sorted data because we can't currently push down any string predicates. It seems like a good thing to pair with the new DataFrameWriter#sortBy feature.

[rdblue]

I'm not sure what should be done to fix the dependency test failure. Looks like there's a list of dependencies that needs to be updated. Is that something I should include in this PR?

[rxin]

Yea you would need to explicitly update the dependency list. We added that as a safe-guard to accidentally changing dependencies.

[rxin]

cc @liancheng who might have idea about past parquet perf regressions.

[srowen]

The dev/test-dependencies script can auto update the deps files for this purpose.

One thing we ask people to investigate are changes between old and new version so we can think through benefits vs potential incompatibilities

[liancheng]

(Updated the performance regression discussion link below.)

I had once tried to upgrade Parquet to 1.8.1, and one more change needs to be done for the upgrade: https://github.com/apache/spark/pull/9225/files#diff-b4108187503e0f3ac64c1630d266b122R115

For the performance regression, here is the full thread of previous discussion. We observed that 1.8.2-SNAPSHOT didn't have the regression any more.

I had tried to bisect, but failed to find anything useful. I thought 1.8.2 would probably be released soon at that time, so didn't try hard to dig it...

[yhuai]

@rdblue Is there any perf evaluation of this new version that we can refer to ?

[liancheng]

I think we should upgrade Parquet to 1.8.1 in Spark 2.0 due to the following reasons:

1. Get PARQUET-251 fixed so that we no longer write corrupted statistics for string/binary columns, and can re-enable filter push-down for these columns (1.8.1 ignores corrupted statistics automatically).
2. The performance regression was discovered by a micro benchmark that simply scans the flat TPC-DS store_sales table (scale factor 15). However, we are using our own vectorized Parquet reader for flat tables now. So at least table scan for flat schemata shouldn't be affected at the moment.

My only concern for this PR is that we should add the hack done in PR #9225. Otherwise there would be noticeable performance regression for queries like SELECT COUNT(*) FROM t.

[rdblue]

@liancheng, thanks for pointing out that fix, I've added it. I thought that was already committed since it has been a while since we fixed the Parquet side.

I've also updated the dependency files, thanks @srowen.

I think given the fix for PARQUET-251, updating is a good idea. There isn't a perf evaluation for 1.8.1 that I can point to, but we've been using it without a noticeable change for months.

[liancheng]

@rdblue LGTM pending rebasing and Jenkins. Thanks for fixing this!

[rdblue]

@liancheng: rebased. Sorry I missed that earlier.

[SparkQA]

Test build #59422 has finished for PR 13280 at commit 30769bd.

• This patch fails Spark unit tests.
• This patch does not merge cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #59430 has finished for PR 13280 at commit d1c79c7.

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

[SparkQA]

Test build #59441 has finished for PR 13280 at commit af3957f.

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

[SparkQA]

Test build #59500 has finished for PR 13280 at commit 85e03f9.

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

[liancheng]

Nit: Could you please revert this indentation change? (IntelliJ's bad I guess...)

[rdblue]

@liancheng, fixed. Yeah, IntelliJ has a few annoyances like that with scala. Imports are a mess.

[liancheng]

Maybe move emptySchema into object CatalystSchemaConverter as a method? Now the same "HACK ALERT" appears 3 times in total:

    def emptyMessageType(name: String): MessageType = {
      // (Add the HACK ALERT here.)
      val messageType = Types.buildMessage()
        .required(PrimitiveType.PrimitiveTypeName.INT32).named("dummy")
        .named(name)
      messageType.getFields.clear()
      messageType
    }

Also, let's make sure the hack alert message mentions the specific parquet-mr version (1.8.2-SNAPSHOT) that fixes PARQUET-363. So that people clearly know when we can remove this hack.

[liancheng]

Just a double check, so the following two are equivalent, right? I wasn't aware of the first approach (which is a little bit confusing since there are two consecutive named calls).

    // (1)
    Types
      .buildMessage()
      .required(PrimitiveType.PrimitiveTypeName.INT32).named("dummy")
      .named("root")

    // (2)
    Types
      .buildMessage()
      .addField(
        Types.required(PrimitiveType.PrimitiveTypeName.INT32).named("dummy")
      )
      .named("root")

[rdblue]

Yes, those are equivalent. The builders nest so that it reads like the schema definition and the parent builder is returned by named.

[rdblue]

Fixed the duplicates by adding CatalystSchemaConverter.EMPTY_MESSAGE.

[rdblue]

Thanks @liancheng! It will be great to have predicate push-down for strings in 2.0!

[yhuai]

Hello @rdblue, we are pretty late in this release cycle. I am afraid that we cannot actually upgrade Parquet to 1.8.1 because of the following two reasons:

1. Since this change was merged pretty late, I am not sure this change can be thoroughly tested. All of our previous testing was based on the Parquet 1.7.0. Also, some of our internal jobs started to fail after this upgrade.
2. Parquet 1.8.1 potentially introduces performance regression. I am not sure we can upgrade Parquet before finish the investigation of this regression.

So, I'd like to propose to revert this upgrade. We can try to upgrade Parquet in the early development cycle of 2.1 (assuming we have figured out the regression). So, we can have more time to test this change.

[rxin]

+1 on reverting for the reasons Yin mentioned. It's very risky to do dep updates at this point for 2.0, and I was surprised this got merged without at least verifying the prior performance regression we found went away.

[markhamstra]

@rxin Huh? The merge was to master, not branch-2.0. Doesn't that put it on the 2.1 track and not into 2.0.0? I think that is all that Yin was saying, that @rdblue was mistaken about this change going into 2.0.

[markhamstra]

Oh, wait... sorry, I just realized that @liancheng said he also merged to branch-2.0. +1 on reverting that.

[liancheng]

I'm working on verifying the regression using the micro benchmark I did before. Sorry for the troubles.

[rdblue]

As I said on PR #13445: It sounds reasonable, but we should follow up on this. If we revert the change I suggest that we only revert it in 2.0 or add it to master as soon as 2.0 is branched. That way we don't adversely affect 2.0, but we do get this addressed. Is that a reasonable path forward?

[rdblue]

@yhuai, what started failing?

[yhuai]

@rdblue When will Parquet 1.8.2 be released?

[rdblue]

We're close to a 1.9.0 release, just working through some performance issues with the switch to ByteBuffer APIs. No estimate on that yet. We can do a 1.8.2 if there's interest so that we can fix some things like this without pulling in all those changes.