Reinitialize the column writers when flushing a row group

[findinpath]

Reinitialize the column writers when flushing a row group

The previous strategy involved in doing the bugfix was:

In the context of closing a row group, when dealing
with FallbackValuesWriter, it is mandatory to call
the resetDictionary() method only after calling
reset() method.

The field firstPage of the FallbackValuesWriter writer
is then correctly set to true to indicate, in case of
dealing with further pages, that the first page is being
processed.

However, this strategy collides with #5518 (comment) reason why now the column writers get reinitialized from scratch.

Fixes #9749

[findinpath]

This bugfix follows the same strategy as in parquet-mr project:

https://github.com/apache/parquet-mr/blob/master/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/InternalParquetRecordWriter.java#L157-L159

After the row group has been flushed, the column writers get reinitialized from scratch.

The FallbackValuesWriter doesn't reset both of its writers within the #reset() method and this behavior leads to the unwanted behavior documented in #5518 (comment)

One concern which I have with this approach is that the reinitialisation of the writers may affect the performance. I'd need in this regard feedback.

[findinpath]

Comparing https://github.com/trinodb/trino/runs/4650149952?check_suite_focus=true (a test run of this branch) and
https://github.com/trinodb/trino/runs/4649720577?check_suite_focus=true (a build on master) on TestFullParquetReader i see differences of about 20% in favor of the master implementation compared to the current bugfix implementation (for thetrino-hive, trino-parquet tests).

I ran again the tests https://github.com/trinodb/trino/runs/4652707273?check_suite_focus=true and found out no major difference between the runs. Actually in most of the cases, the tests were running faster on the branch bugfix/parquet-9749 than on master

The test TestFullParquetReader.testNestedStructs took 1m / 2m on the bugfix branch and 8 seconds on master. I'm not sure whether this is relevant or is it linked to the github build runner.

[martint]

The FallbackValuesWriter doesn't reset both of its writers within the #reset() method and this behavior leads to the unwanted behavior documented in #5518 (comment)

When does the FallbackValuesWriter get used?

One concern which I have with this approach is that the reinitialisation of the writers may affect the performance. I'd need in this regard feedback.

That should not be much of a problem. It happens once per row group and it doesn't seem to be a very expensive operation.

[findinpath]

When does the FallbackValuesWriter get used?

It gets used as a fallback for the situation where the dictionary encoding grows too big.

https://github.com/apache/parquet-format/blob/master/Encodings.md#dictionary-encoding-plain_dictionary--2-and-rle_dictionary--8

[alexjo2144]

What about changing the top three reset calls to re-initialize here https://github.com/trinodb/trino/blob/master/lib/trino-parquet/src/main/java/io/trino/parquet/writer/PrimitiveColumnWriter.java#L313

The rest of the fields should reset/clear fine, it's just the ValueWriters that seem to have a problem.

If that's not worth it, I think the reset method is unused and can be removed.

[findinpath]

What about changing the top three reset calls to re-initialize here https://github.com/trinodb/trino/blob/master/lib/trino-parquet/src/main/java/io/trino/parquet/writer/PrimitiveColumnWriter.java#L313

@alexjo2144 I'm not sure that I follow what you mean.
The PrimitiveColumnWriter doesn't have any knowledge on how to re-initialize/reinstantiate the underlying writers.

https://github.com/trinodb/trino/blob/master/lib/trino-parquet/src/main/java/io/trino/parquet/writer/PrimitiveColumnWriter.java#L100-L102

[alexjo2144]

Sorry, my thought was can we either:

1. Change the fields of PrimitiveColumnWriter so that it has what it needs to re-initialize the underlying writers to minimize the number of objects thrown out on each row group

or if not

2. Remove ColumnWriter#reset, since I think you're removing the one place it was used

[findinpath]

Remove ColumnWriter#reset, since I think you're removing the one place it was used

@alexjo2144 Indeed, this method is now no longer needed.

I'm still investigating also the refactoring possibility:

Change the fields of PrimitiveColumnWriter so that it has what it needs to re-initialize the underlying writers to minimize the number of objects thrown out on each row group

Taken from the Parquet documentation https://parquet.apache.org/documentation/latest/

We recommend large row groups (512MB - 1GB). Since an entire row group might need to be read, we want it to completely fit on one HDFS block.

I'm thinking that in such cases there is no really a big overhead of creating again from scratch the writers in case of dealing with a new row group.

[skrzypo987]

This looks ok.
However, my knowledge about writing Parquet files is non-existent so I can't be the one to approve anything here.

[findepi]

Thanks @skrzypo987 for taking a look