PARQUET-363: Allow empty schema groups.

[rdblue]

This removes the check added in PARQUET-278 that rejects schema groups
that have no fields. Selecting 0 columns from a file is allowed and used
by Hive and SparkSQL to implement queries like select count(1) ...

[liancheng]

LGTM

[isnotinvain]

Before we ship this, can we add back in some validation to cover what PARQUET-278 intended to cover?

[isnotinvain]

I think maybe special casing a statistics-only read path would make sense, but we do need a runtime check that there are no nested empty structs inside a schema at write time.

[isnotinvain]

could be as simple as moving the check from the GroupType constructor to some method in the write path that just asserts (recursively) that the schema being written contains no empty GroupTypes

[rdblue]

@isnotinvain PARQUET-278 doesn't state what the underlying problem was, just that we "shouldn't" allow empty groups. Was it the problem you're talking about above, where something breaks in the write path if you pass in an empty schema?

We have two options in that case, to update the write path to write nothing for empty groups (fine with me) or to add a check as you suggest when instantiating a writer. I'm fine with the latter case, since I see no value in being able to write a schema with empty groups. I'd rather not block this fix on it, though.

[isnotinvain]

Parquet will write corrupt files if the write path accepts schemas with empty Groups, the file won't be readable after its written because of the empty groups, IIRC.

That's why the exception was added to to the constructor. I think it's important we preserve that guard, but we can certainly move it to be write-path only and out of the Group constructor. I feel like that would belong in this PR, instead of removing an important check without replacing it. What do you think?

[rdblue]

Since it is a file corruption issue, we should definitely fix it in this PR. I didn't realize that files were corrupted... is there something that documents what happens and whether you can recover the files?

I'll add a write-side guard to this issue before we commit it.

[isnotinvain]

Maybe corruption is too strong of a word. The data is all in tact, but you can't read the full records. You are probably fine if you project away the field w/ with the empty group in it, is that right @tsdeng?

[rdblue]

Okay, I've updated this PR:

• ParquetFileReader now rejects schemas with empty groups
• Types will now construct schemas with empty groups because there may be data files where there are empty groups and this was preventing them from being read (I believe this is likely the error that @tsdeng hit).
• I've added a few minor classes to make this easily tested, like ExampleParquetWriter.

[isnotinvain]

Thanks for updating. The issue we had and that @tsdeng fixed was that the write path was allowing these invalid schemas through which it shouldn't. Your fix looks good to me.

@tsdeng you had some tests for each kind of group (required, optional, repeated) was there any specific reason for that? I don't think it's needed so I think we are OK.

+1

[isnotinvain]

oh, we may also want to put this check in the outputformat so that it is checked client-side, before launching a hadoop job, right?

[tsdeng]

Can we add comment on why this should be supported so next time it will not be accidentally removed

[rdblue]

Done.

[rdblue]

@isnotinvain, I thought about adding it to the OutputFormat, but that would require calling WriteContext.init on the client side, which isn't currently done. If we want to change the output initialization steps to catch stuff like this, I think we should do it in a separate issue.

[isnotinvain]

+1, yeah if we don't have access to the schema on the client without changing the lifecycle, lets do that separately if needed.