PARQUET-2365 : Fixes NPE when rewriting column without column index #1173

ConeyLiu · 2023-10-17T09:39:22Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-XXX
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

The ColumnIndex could be null in some scenes, for example, the float/double column contains NaN or the size has exceeded the expected value. And the page header statistics are not written anymore after we supported ColumnIndex. So we will get NPE when rewriting the column without ColumnIndex due to we will get NULL page statistics when converted from the ColumnIndex(NULL) or page header statistics(NULL). Such as the following:

java.lang.NullPointerException
    at org.apache.parquet.hadoop.ParquetFileWriter.writeDataPage(ParquetFileWriter.java:727)
    at org.apache.parquet.hadoop.ParquetFileWriter.innerWriteDataPage(ParquetFileWriter.java:663)
    at org.apache.parquet.hadoop.ParquetFileWriter.writeDataPage(ParquetFileWriter.java:650)
    at org.apache.parquet.hadoop.rewrite.ParquetRewriter.processChunk(ParquetRewriter.java:453)
    at org.apache.parquet.hadoop.rewrite.ParquetRewriter.processBlocksFromReader(ParquetRewriter.java:317)
    at org.apache.parquet.hadoop.rewrite.ParquetRewriter.processBlocks(ParquetRewriter.java:250)

ConeyLiu · 2023-10-17T09:41:16Z

Hi @wgtmac, please help to review this when you are free. Thanks a lot.

ConeyLiu · 2023-10-17T09:44:23Z

...-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java

@@ -543,6 +546,11 @@ public static ColumnIndex build(
   *          the statistics to be added
   */
  public void add(Statistics<?> stats) {
+    if (stats.isEmpty()) {


It should be invalid if the stats are empty. Previously we set it as a null page. @gszadovszky please correct me if I am wrong.

I am not sure how column index writing worked for the "rewriter". This rewriting thing is newer than the column index. In the normal writing scenario stats cannot be empty since we are just creating these objects during the write path.

This is especially used when the ColumnIndex is null during rewriting. And we pass empty statistics to the ColumnIndexBuilder to avoid NPE.

Let me try to understand what happens here. convertStatistics is used to recover page statistics from ColumnIndex or original page header if the ColumnIndex is unavailable. The problem emerges when ColumnIndex is unavailable. Am I correct? If true, then why do we need those changes in the ColumnIndexBuilder?

The problem happens when both the ColumnIndex and the page header Statistics are null. Because we get null returned from the convertStatistics. However, the ParquetFileWriter.writeDataPage needs the page statistics. So here we pass invalid page statistics to avoid the NPE and overwrite the column statistics in the end. Otherwise, we need to add some methods that don't need page statistics.

From what you have said, it seems that the problem comes from the input file which has valid aggregate statistics for the column chunk but does not write page statistics in the page header. Should we just fix the NPE in the page header and leave other parts as is?

Should we just fix the NPE in the page header and leave other parts as is?

Updated the implementation.

ConeyLiu · 2023-10-17T09:48:33Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java

+   * @param totalStatistics the column total statistics
+   * @throws IOException if there is an error while writing
+   */
+  public void endColumn(Statistics<?> totalStatistics) throws IOException {


Exposing this to public is not a good idea. Other good suggestions are welcome.

Since this is used only for invalid stats, is it better to add a void invalidateStatistics() which simply include line 988 and 990? Then you just need to call invalidateStatistics() and endColumn() in this case.

This is used to set the column aggregated statistics. Do you mean to modify the public void endColumn(Statistics<?> totalStatistics) to public void invalidateStatistics(Statistics<?> totalStatistics)?

Replaced with invalidateStatistics

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/rewrite/ParquetRewriter.java

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java

wgtmac · 2023-10-19T03:27:22Z

...-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java

@@ -543,6 +546,11 @@ public static ColumnIndex build(
   *          the statistics to be added
   */
  public void add(Statistics<?> stats) {
+    if (stats.isEmpty()) {


Let me try to understand what happens here. convertStatistics is used to recover page statistics from ColumnIndex or original page header if the ColumnIndex is unavailable. The problem emerges when ColumnIndex is unavailable. Am I correct? If true, then why do we need those changes in the ColumnIndexBuilder?

...-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java

wgtmac · 2023-10-25T15:14:21Z

...-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java

@@ -543,6 +546,11 @@ public static ColumnIndex build(
   *          the statistics to be added
   */
  public void add(Statistics<?> stats) {
+    if (stats.isEmpty()) {


From what you have said, it seems that the problem comes from the input file which has valid aggregate statistics for the column chunk but does not write page statistics in the page header. Should we just fix the NPE in the page header and leave other parts as is?

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java

wgtmac · 2023-11-03T09:30:59Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java

+   * @param totalStatistics the column total statistics
+   * @throws IOException if there is an error while writing
+   */
+  public void endColumn(Statistics<?> totalStatistics) throws IOException {


Since this is used only for invalid stats, is it better to add a void invalidateStatistics() which simply include line 988 and 990? Then you just need to call invalidateStatistics() and endColumn() in this case.

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/rewrite/ParquetRewriter.java

wgtmac

Thanks. LGTM

wgtmac · 2023-11-03T16:00:11Z

It seems that a rebase is required to make CIs happy.

…/ParquetRewriter.java Co-authored-by: Gang Wu <[email protected]>

ConeyLiu · 2023-11-03T16:12:50Z

Just rebased

ConeyLiu · 2023-11-04T13:17:33Z

Thanks @wgtmac @gszadovszky

ConeyLiu · 2023-11-10T03:20:04Z

Hi, @wgtmac @gszadovszky should we port this to 1.13.x branch? And is there any plan for a new release?

wgtmac · 2023-11-10T03:29:31Z

IMO, there isn't any plan for 1.13.2 release. But you may port it to 1.13.x branch just in case.

ConeyLiu · 2023-11-10T03:39:24Z

OK, I will port it.

ConeyLiu commented Oct 17, 2023

View reviewed changes

wgtmac reviewed Oct 19, 2023

View reviewed changes

wgtmac reviewed Oct 25, 2023

View reviewed changes

wgtmac reviewed Nov 3, 2023

View reviewed changes

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java Outdated Show resolved Hide resolved

ConeyLiu force-pushed the column-index branch from 94b0cd1 to 7019e74 Compare November 3, 2023 06:58

wgtmac reviewed Nov 3, 2023

View reviewed changes

wgtmac approved these changes Nov 3, 2023

View reviewed changes

ConeyLiu and others added 6 commits November 4, 2023 00:09

Fixes NPE when rewriting column without column index

54eba47

Update parquet-hadoop/src/main/java/org/apache/parquet/hadoop/rewrite…

492f917

…/ParquetRewriter.java Co-authored-by: Gang Wu <[email protected]>

revert unrelated changes

3e2f8dd

address comments

039ae2a

address comments

b945b9a

address comments

8da6804

ConeyLiu force-pushed the column-index branch from adfd647 to 8da6804 Compare November 3, 2023 16:12

wgtmac approved these changes Nov 4, 2023

View reviewed changes

wgtmac merged commit ff36d6b into apache:master Nov 4, 2023
9 checks passed

ConeyLiu deleted the column-index branch November 4, 2023 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-2365 : Fixes NPE when rewriting column without column index #1173

PARQUET-2365 : Fixes NPE when rewriting column without column index #1173

ConeyLiu commented Oct 17, 2023 •

edited

Loading

ConeyLiu commented Oct 17, 2023

ConeyLiu Oct 17, 2023

gszadovszky Oct 17, 2023

ConeyLiu Oct 17, 2023

wgtmac Oct 19, 2023

ConeyLiu Oct 19, 2023

wgtmac Oct 25, 2023

ConeyLiu Oct 27, 2023

ConeyLiu Oct 17, 2023

wgtmac Nov 3, 2023

ConeyLiu Nov 3, 2023

ConeyLiu Nov 3, 2023

wgtmac Oct 19, 2023

wgtmac Oct 25, 2023

wgtmac Nov 3, 2023

wgtmac left a comment

wgtmac commented Nov 3, 2023

ConeyLiu commented Nov 3, 2023

ConeyLiu commented Nov 4, 2023

ConeyLiu commented Nov 10, 2023

wgtmac commented Nov 10, 2023

ConeyLiu commented Nov 10, 2023

PARQUET-2365 : Fixes NPE when rewriting column without column index #1173

PARQUET-2365 : Fixes NPE when rewriting column without column index #1173

Conversation

ConeyLiu commented Oct 17, 2023 • edited Loading

Jira

Tests

Commits

Documentation

ConeyLiu commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgtmac left a comment

Choose a reason for hiding this comment

wgtmac commented Nov 3, 2023

ConeyLiu commented Nov 3, 2023

ConeyLiu commented Nov 4, 2023

ConeyLiu commented Nov 10, 2023

wgtmac commented Nov 10, 2023

ConeyLiu commented Nov 10, 2023

ConeyLiu commented Oct 17, 2023 •

edited

Loading