PARQUET-1685: Truncate Min/Max for Statistics by shangxinli · Pull Request #696 · apache/parquet-java

shangxinli · 2019-10-30T01:29:34Z

Make sure you have checked all steps below.

Jira

PARQUET-1685: Truncate Min/Max for Statistics

Tests

Added unit test
Write a Spark application test and it passed

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

gszadovszky

I would not add the truncate mechanism to the Statistics objects (see details in the code comments). ParquetMetadataConverter might be a better place.

parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java

shangxinli · 2019-10-30T21:32:39Z

Yes. Moved it to ParquetMetadataConverter for the write path only.

gszadovszky · 2019-10-31T07:31:47Z

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

    return formatStats;
  }

+  public static void setStatisticsTruncateLength(int statisticsTruncateLength) {


This is not correct this way. Writer has a constructor argument so you shall not set here statically. I would suggest creating a new toParquetStatistics method with this argument instead.

Yes, I know setting it here is a little hacky. The reasons for that are: 1) ParquetMetadataConverter is created as static inside ParquetFileWriter(don't' understand why). ParquetFileWriter has no chance to pass the parameter that it gets from it's constructor. 2) 'toParquetStatistics()' is defined as public static, so the constructor parameter doesn't work for it. But you suggested rewriting my own toParquetStatistics(). The concern is code duplicate.

The #2 is less concern that I can refactor the code a bit to reduce the duplicate. Any suggestions for #1? Maybe change metadataConverter to non-static? Concerns?

Just created another iteration by changing the metadataConverter to non-static and create my own 'toParquetStatistics'. Please have a look.

Sorry for the late response. I didn't want to recommend refactoring the usage of MetadataConverter for your change. I've just ment to have the truncateLength as a parameter of toParquetStatistics. You may keep the original method header as well so you do not need to rewrite all the calls only the ones are required for this change.

Got it. Thanks.

gszadovszky · 2019-10-31T07:41:52Z

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

+   * @param pageWriteChecksumEnabled whether to write out page level checksums
+   * @throws IOException if the file can not be created
+   */
+  public ParquetFileWriter(OutputFile file, MessageType schema, Mode mode,


Please, check where the previous constructor is invoked and update those code parts as well.
In addition, if this change is targeted to the next release (1.11.0) and the previous constructor was not released yet, you may simply add the new argument to it. The key is to not break API compatibility between releases.

Good point. I found I forgot to change the new ParquetFileWriter() inside ParquetWriter to the new signature. I will add it soon.

Since the 1.11.0 tag has been released and the current version is 1.12.0-SNAPSHOT, I would assume this will be in 1.12.0.

This is because or faulty releasing process. 1.11.0 is not release yet. 1.11.0 tag will be moved to the finally released RC.

Cool. Let's just add one parameter instead of creating a new signature.

gszadovszky · 2019-11-06T09:12:41Z

...t-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java

+  @Test
+  public void testBinaryStatsWithTruncation() {
+    int defaultTruncLen = ParquetProperties.DEFAULT_STATISTICS_TRUNCATE_LENGTH;
+    int[] validLengths = {1, 2, 16, 64, defaultTruncLen - 1, defaultTruncLen - 1};


Why do you test defaultTruncLen -1 twice? I guess, defaultTrundLen itself shall also be tested.

Good catch. That is a typo, I meant to test defaultTruncLen.

- 1 is still there. :)

Don't know how it is slipped. Fix it now.

gszadovszky · 2019-11-06T09:15:02Z

...t-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java

+      testBinaryStatsWithTruncation(len);
+    }
+
+    int[] invalidLengths = {-1, 0, defaultTruncLen + 1};


defaultTruncLen + 1 is invalid only because ParquetProperties.DEFAULT_STATISTICS_TRUNCATE_LENGTH is Integer.MAX_VALUE so it overflows. Meanwhile, semantically a larger value of the default one shall be supported.

Make sense. I will make it as "-1, 0, Integer.MAX_VALUE + 1"

Still not fixed.

Don't know how it is slipped. Fix it now.

gszadovszky · 2019-11-06T09:19:53Z

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

@@ -591,9 +609,17 @@ public static Statistics toParquetStatistics(
    if (!stats.isEmpty() && stats.isSmallerThan(MAX_STATS_SIZE)) {


So, we are not truncating if the size is too large? It should be the other way around: check the size after the truncate.

gszadovszky · 2019-11-06T09:20:55Z

...t-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java

+    }
+  }
+
+  private void testBinaryStatsWithTruncation(int truncateLen) {


Please add test data where we would reach the size limit of statistics.

gszadovszky · 2019-11-11T09:10:58Z

...t-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java

+  @Test
+  public void testBinaryStatsWithTruncation() {
+    int defaultTruncLen = ParquetProperties.DEFAULT_STATISTICS_TRUNCATE_LENGTH;
+    int[] validLengths = {1, 2, 16, 64, defaultTruncLen - 1, defaultTruncLen - 1};


- 1 is still there. :)

gszadovszky · 2019-11-11T09:11:36Z

...t-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java

+      testBinaryStatsWithTruncation(len);
+    }
+
+    int[] invalidLengths = {-1, 0, defaultTruncLen + 1};


Still not fixed.

...t-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

gszadovszky

One more finding.
The build/test fails because of PARQUET-1691. Please, wait with your last change for the PR #698 to get in so we can re-trigger the build and we will have all green lights.

gszadovszky · 2019-11-12T09:46:12Z

...t-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java

+    for (int len : invalidLengths) {
+      try {
+        testBinaryStatsWithTruncation(len, 80, 20);
+      } catch (IllegalArgumentException e) {


This test would not fail if no exception is thrown.

Suggested change

} catch (IllegalArgumentException e) {

Assert.fail("...");

} catch (IllegalArgumentException e) {

Remove unnecessary string converting in readFooter method

fe147cc

gszadovszky requested changes Oct 30, 2019

View reviewed changes

parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java Outdated Show resolved Hide resolved

parquet-column/src/main/java/org/apache/parquet/column/statistics/BinaryStatistics.java Outdated Show resolved Hide resolved

shangxinli force-pushed the master branch from cb89fa6 to 562840a Compare October 30, 2019 21:26

gszadovszky requested changes Oct 31, 2019

View reviewed changes

shangxinli force-pushed the master branch 5 times, most recently from 18f938c to c33cd36 Compare November 5, 2019 18:36

gszadovszky requested changes Nov 6, 2019

View reviewed changes

shangxinli force-pushed the master branch from 1203348 to 8c017b6 Compare November 10, 2019 17:44

gszadovszky requested changes Nov 11, 2019

View reviewed changes

shangxinli force-pushed the master branch from 7c7700c to a7056ed Compare November 11, 2019 17:06

gszadovszky requested changes Nov 12, 2019

View reviewed changes

PARQUET-1685: Truncate Min/Max for Statistics

6d9d691

shangxinli force-pushed the master branch from f3dc062 to 6d9d691 Compare November 12, 2019 17:56

gszadovszky approved these changes Nov 13, 2019

View reviewed changes

gszadovszky merged commit 76f9010 into apache:master Nov 13, 2019

emcake mentioned this pull request Nov 5, 2023

Binary columns do not receive truncated statistics apache/arrow-rs#5037

Closed

emcake mentioned this pull request Nov 14, 2023

Enable truncation of binary statistics columns apache/arrow-rs#5076

Merged

		@@ -591,9 +609,17 @@ public static Statistics toParquetStatistics(
		if (!stats.isEmpty() && stats.isSmallerThan(MAX_STATS_SIZE)) {

	} catch (IllegalArgumentException e) {
	Assert.fail("...");
	} catch (IllegalArgumentException e) {

Comments

Conversation

shangxinli commented Oct 30, 2019

Jira

Tests

Commits

Documentation

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shangxinli commented Oct 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shangxinli Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

shangxinli Oct 31, 2019 •

edited

Loading