Add Baseline to iceberg-parquet #526

Fokko · 2019-10-09T20:14:03Z

Fixes #155

rdsr · 2019-10-10T02:10:34Z

Seems like some checkstyle errors are remaining. https://travis-ci.org/apache/incubator-iceberg/jobs/595801676

rdsr · 2019-10-10T02:07:44Z

build.gradle

@@ -165,7 +165,7 @@ task deploySite(type: Exec) {
 // Baseline style guide.
 def baselineProjects = [ project("iceberg-api"), project("iceberg-common"), project("iceberg-core"),


Once iceberg-pig and iceberg-parquet are fixed. We should remove the baselineProjects variable

Fokko · 2019-10-11T15:21:54Z

Thanks @rdsr for pointing out, I've fixed the checkstyle issues as well.

parquet/src/main/java/org/apache/iceberg/parquet/ParquetDictionaryRowGroupFilter.java

rdblue · 2019-10-13T18:15:14Z

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

-    public WriteBuilder schema(Schema schema) {
-      this.schema = schema;
+    public WriteBuilder schema(Schema setSchema) {
+      this.schema = setSchema;


The convention used elsewhere to get around this problem is to use "new" as in newSchema. Could you use that instead for consistency?

parquet/src/main/java/org/apache/iceberg/parquet/ParquetDictionaryRowGroupFilter.java

parquet/src/main/java/org/apache/iceberg/parquet/ParquetFilters.java

parquet/src/main/java/org/apache/iceberg/parquet/ParquetSchemaUtil.java

parquet/src/main/java/org/apache/iceberg/parquet/ParquetTypeVisitor.java

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java

rdblue · 2019-10-13T18:27:23Z

parquet/src/main/java/org/apache/iceberg/parquet/TypeWithSchemaVisitor.java

@@ -84,19 +84,18 @@
              }

              return visitor.list(list, group, elementResult);
-
            } finally {
              visitor.fieldNames.pop();
            }

          case MAP:


Instead of suppressing, we could break these into methods by case.

I can do that, but that will mess up the git history. It is up to you.

rdblue · 2019-10-13T18:28:16Z

parquet/src/test/java/org/apache/iceberg/parquet/TestDictionaryRowGroupFilter.java

+  private static final File parquetFile = new File("/tmp/stats-row-group-filter-test.parquet");
+  private static MessageType parquetSchema = null;
+  private static BlockMetaData rowGroupMetadata = null;
+  private static DictionaryPageReadStore dictionaryStore = null;


Wasn't all caps the right format for private static variables?

I think they should be final then as well. I'll check.

Yes:

[ant:checkstyle] [ERROR] /Users/fokkodriesprong/Desktop/incubator-iceberg/parquet/src/test/java/org/apache/iceberg/parquet/TestDictionaryRowGroupFilter.java:107:30: Name 'PARQUET_SCHEMA' must match pattern '^[a-z][a-zA-Z0-9]*$'. [StaticVariableName] [ant:checkstyle] [ERROR] /Users/fokkodriesprong/Desktop/incubator-iceberg/parquet/src/test/java/org/apache/iceberg/parquet/TestDictionaryRowGroupFilter.java:108:32: Name 'ROW_GROUP_METADATA' must match pattern '^[a-z][a-zA-Z0-9]*$'. [StaticVariableName] [ant:checkstyle] [ERROR] /Users/fokkodriesprong/Desktop/incubator-iceberg/parquet/src/test/java/org/apache/iceberg/parquet/TestDictionaryRowGroupFilter.java:109:42: Name 'DICTIONARY_STORE' must match pattern '^[a-z][a-zA-Z0-9]*$'. [StaticVariableName]

I did revert the change for the PARQUET_FILE, since that one is final.

Fokko · 2019-10-18T12:55:11Z

Any pending issues?

Fokko · 2019-10-24T09:03:22Z

Thanks for merging

Add Baseline to iceberg-parquet

a2e0802

rdsr requested changes Oct 10, 2019

View reviewed changes

Fix the checkstyle voilations

6e1af4e