Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Static Analysis #1350

Merged
merged 7 commits into from
Aug 21, 2020
Merged

Update Static Analysis #1350

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec364e7
Update Static Analysis WIP
RussellSpitzer Aug 17, 2020
3bc9cd6
Fix Style Issues that were Previously False Negatives
RussellSpitzer Aug 20, 2020
b78acbc
Fix Optional orElse
RussellSpitzer Aug 20, 2020
1aeaf75
Switch to Effectively Local Scheme in Reader
RussellSpitzer Aug 20, 2020
0f23296
Fix Spacing
RussellSpitzer Aug 21, 2020
e452662
Fix all Single Line Constructors
RussellSpitzer Aug 21, 2020
2ec9ead
Fix Comments without Whitespace
RussellSpitzer Aug 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions .baseline/checkstyle/checkstyle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
<property name="severity" value="error"/>

<module name="FileTabCharacter"/> <!-- Java Style Guide: Whitespace characters -->
<module name="LineLength"> <!-- Java Style Guide: No line-wrapping -->
<property name="max" value="120"/>
<property name="ignorePattern" value="^package.*|^import.*|a href|href|http://|https://|ftp://"/>
</module>
<module name="NewlineAtEndOfFile"> <!-- Java Style Guide: Line ending: LF -->
<property name="lineSeparator" value="lf"/>
</module>
Expand Down Expand Up @@ -212,10 +216,6 @@
</module>
<module name="InnerAssignment"/> <!-- Java Coding Guidelines: Inner assignments: Not used -->
<module name="LeftCurly"/> <!-- Java Style Guide: Nonempty blocks: K & R style -->
<module name="LineLength"> <!-- Java Style Guide: No line-wrapping -->
<property name="max" value="120"/>
<property name="ignorePattern" value="^package.*|^import.*|a href|href|http://|https://|ftp://"/>
</module>
<module name="MemberName"> <!-- Java Style Guide: Non-constant field names -->
<property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
<message key="name.invalidPattern" value="Member name ''{0}'' must match pattern ''{1}''."/>
Expand Down Expand Up @@ -356,7 +356,7 @@
</module>
<module name="RightCurly"> <!-- Java Style Guide: Nonempty blocks: K & R style -->
<property name="option" value="alone"/>
<property name="tokens" value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE, STATIC_INIT, INSTANCE_INIT"/>
<property name="tokens" value="CLASS_DEF, METHOD_DEF, LITERAL_FOR, LITERAL_WHILE, STATIC_INIT, INSTANCE_INIT"/>
</module>
<module name="SeparatorWrap"> <!-- Java Style Guide: Where to break -->
<property name="tokens" value="DOT"/>
Expand Down Expand Up @@ -409,11 +409,9 @@
<module name="JavadocMethod"> <!-- Java Style Guide: Where Javadoc is used -->
<property name="scope" value="public"/>
<property name="allowMissingParamTags" value="true"/>
<property name="allowMissingThrowsTags" value="true"/>
<property name="allowMissingReturnTag" value="true"/>
<property name="minLineCount" value="99999999"/>
<property name="allowedAnnotations" value="Override, Test"/>
<property name="allowThrowsTagsForSubclasses" value="true"/>
<property name="validateThrows" value="false"/>
Copy link
Contributor

@rdblue rdblue Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems more general than "allowThrowsTagsForSubclasses"? Are we sure it does the same thing?

Copy link
Member Author

@RussellSpitzer RussellSpitzer Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit of a complicated story

checkstyle/checkstyle#7329

Long story short, the checks were basically broken in check style so they were removed and are no longer valid configs. We actually don't need to set "validateThrows" to false either since it is false by default. I just wanted to make sure we were aware that we weren't validating any throw related tags.

</module>
<module name="JavadocStyle"> <!-- Java Style Guide: Javadoc -->
<property name="checkFirstSentence" value="false"/>
Expand Down
7 changes: 5 additions & 2 deletions arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,9 @@ public String toString() {
}

@Override
public void setBatchSize(int batchSize) {}
public void setBatchSize(int batchSize) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we require the empty line?

Copy link
Member Author

@RussellSpitzer RussellSpitzer Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we actually did for the Private Constructors as well but we were ignoring that everywhere so I deleted that check

        <module name="RightCurly"> <!-- Java Style Guide: Nonempty blocks: K & R style -->
            <property name="option" value="alone"/>
            <property name="tokens" value="CLASS_DEF, **METHOD_DEF**, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE, STATIC_INIT, INSTANCE_INIT"/>
        </module>

Basically Check-style was just broken before (see my comment on this PR) and didn't work correctly on empty blocks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't suppose it's possible to make it okay to have the right curly on the line just after the method definition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is allowed, I'll change it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that allow us to turn it back on for private constructors without too much trouble?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to have it on for private constructors and I can change all the references to

private Constructor() {
}

Because I can do that no problem

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's what we do in most places isn't it? I was hoping that with this update it wouldn't be too many places to change. Probably good to update them to all be standard, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we can do that in a separate PR if you want to keep this one small.

}
}

/**
Expand Down Expand Up @@ -377,8 +379,9 @@ public String toString() {
}

@Override
public void setBatchSize(int batchSize) {}
public void setBatchSize(int batchSize) {

}
}

}
Expand Down
7 changes: 4 additions & 3 deletions baseline.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ subprojects {

pluginManager.withPlugin('com.palantir.baseline-error-prone') {
tasks.withType(JavaCompile).configureEach {
options.errorprone.errorproneArgs += [
options.errorprone.errorproneArgs.addAll (
// error-prone is slow, don't run on tests
'-XepExcludedPaths:.*/test/.*',
// specific to Palantir
Expand All @@ -71,11 +71,12 @@ subprojects {
// subclasses are not equal
'-Xep:EqualsGetClass:OFF',
// patterns that are allowed
'-Xep:SwitchStatementDefaultCase:OFF',
'-Xep:MissingCasesInEnumSwitch:OFF',
//Added because it errors out compile, but we need to figure out if we want it
'-Xep:StrictUnusedVariable:OFF',
'-Xep:TypeParameterShadowing:OFF',
'-Xep:TypeParameterUnusedInFormals:OFF',
]
)
}
}
}
4 changes: 2 additions & 2 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@ buildscript {
}
dependencies {
classpath 'com.github.jengelman.gradle.plugins:shadow:5.0.0'
classpath 'com.palantir.baseline:gradle-baseline-java:0.55.0'
classpath 'com.palantir.baseline:gradle-baseline-java:3.36.2'
classpath 'com.diffplug.spotless:spotless-plugin-gradle:3.14.0'
classpath 'gradle.plugin.org.inferred:gradle-processors:2.1.0'
classpath 'me.champeau.gradle:jmh-gradle-plugin:0.4.8'
}
}

plugins {
id 'com.palantir.git-version' version '0.9.1'
id 'com.palantir.git-version' version '0.12.3'
id 'nebula.dependency-recommender' version '9.0.2'
}

Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,8 @@ private void keepActiveManifests(List<ManifestFile> currentManifests) {
// keep any existing manifests as-is that were not processed
keptManifests.clear();
currentManifests.stream()
.filter(manifest -> !rewrittenManifests.contains(manifest) && !deletedManifests.contains(manifest))
.forEach(manifest -> keptManifests.add(manifest));
.filter(manifest -> !rewrittenManifests.contains(manifest) && !deletedManifests.contains(manifest))
.forEach(manifest -> keptManifests.add(manifest));
}

private void reset() {
Expand Down
100 changes: 50 additions & 50 deletions core/src/test/java/org/apache/iceberg/TestRewriteManifests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,18 +136,18 @@ public void testRewriteManifestsGeneratedAndAppendedDirectly() throws IOExceptio
public void testReplaceManifestsSeparate() {
Table table = load();
table.newFastAppend()
.appendFile(FILE_A)
.appendFile(FILE_B)
.commit();
.appendFile(FILE_A)
.appendFile(FILE_B)
.commit();
long appendId = table.currentSnapshot().snapshotId();

Assert.assertEquals(1, table.currentSnapshot().allManifests().size());

// cluster by path will split the manifest into two

table.rewriteManifests()
.clusterBy(file -> file.path())
.commit();
.clusterBy(file -> file.path())
.commit();

List<ManifestFile> manifests = table.currentSnapshot().allManifests();
Assert.assertEquals(2, manifests.size());
Expand All @@ -168,21 +168,21 @@ public void testReplaceManifestsConsolidate() throws IOException {
Table table = load();

table.newFastAppend()
.appendFile(FILE_A)
.commit();
.appendFile(FILE_A)
.commit();
long appendIdA = table.currentSnapshot().snapshotId();
table.newFastAppend()
.appendFile(FILE_B)
.commit();
.appendFile(FILE_B)
.commit();
long appendIdB = table.currentSnapshot().snapshotId();

Assert.assertEquals(2, table.currentSnapshot().allManifests().size());

// cluster by constant will combine manifests into one

table.rewriteManifests()
.clusterBy(file -> "file")
.commit();
.clusterBy(file -> "file")
.commit();

List<ManifestFile> manifests = table.currentSnapshot().allManifests();
Assert.assertEquals(1, manifests.size());
Expand Down Expand Up @@ -211,34 +211,34 @@ public void testReplaceManifestsWithFilter() throws IOException {
Table table = load();

table.newFastAppend()
.appendFile(FILE_A)
.commit();
.appendFile(FILE_A)
.commit();
long appendIdA = table.currentSnapshot().snapshotId();

table.newFastAppend()
.appendFile(FILE_B)
.commit();
.appendFile(FILE_B)
.commit();
long appendIdB = table.currentSnapshot().snapshotId();

table.newFastAppend()
.appendFile(FILE_C)
.commit();
.appendFile(FILE_C)
.commit();
long appendIdC = table.currentSnapshot().snapshotId();

Assert.assertEquals(3, table.currentSnapshot().allManifests().size());

//keep the file A manifest, combine the other two

table.rewriteManifests()
.clusterBy(file -> "file")
.rewriteIf(manifest -> {
try (ManifestReader<DataFile> reader = ManifestFiles.read(manifest, table.io())) {
return !reader.iterator().next().path().equals(FILE_A.path());
} catch (IOException x) {
throw new RuntimeIOException(x);
}
})
.commit();
.clusterBy(file -> "file")
.rewriteIf(manifest -> {
try (ManifestReader<DataFile> reader = ManifestFiles.read(manifest, table.io())) {
return !reader.iterator().next().path().equals(FILE_A.path());
} catch (IOException x) {
throw new RuntimeIOException(x);
}
})
.commit();

List<ManifestFile> manifests = table.currentSnapshot().allManifests();
Assert.assertEquals(2, manifests.size());
Expand Down Expand Up @@ -270,9 +270,9 @@ public void testReplaceManifestsWithFilter() throws IOException {
public void testReplaceManifestsMaxSize() {
Table table = load();
table.newFastAppend()
.appendFile(FILE_A)
.appendFile(FILE_B)
.commit();
.appendFile(FILE_A)
.appendFile(FILE_B)
.commit();
long appendId = table.currentSnapshot().snapshotId();

Assert.assertEquals(1, table.currentSnapshot().allManifests().size());
Expand Down Expand Up @@ -300,12 +300,12 @@ public void testReplaceManifestsMaxSize() {
public void testConcurrentRewriteManifest() throws IOException {
Table table = load();
table.newFastAppend()
.appendFile(FILE_A)
.commit();
.appendFile(FILE_A)
.commit();
long appendIdA = table.currentSnapshot().snapshotId();
table.newFastAppend()
.appendFile(FILE_B)
.commit();
.appendFile(FILE_B)
.commit();
long appendIdB = table.currentSnapshot().snapshotId();

// start a rewrite manifests that involves both manifests
Expand All @@ -314,15 +314,15 @@ public void testConcurrentRewriteManifest() throws IOException {

// commit a rewrite manifests that only involves one manifest
table.rewriteManifests()
.clusterBy(file -> "file")
.rewriteIf(manifest -> {
try (ManifestReader<DataFile> reader = ManifestFiles.read(manifest, table.io())) {
return !reader.iterator().next().path().equals(FILE_A.path());
} catch (IOException x) {
throw new RuntimeIOException(x);
}
})
.commit();
.clusterBy(file -> "file")
.rewriteIf(manifest -> {
try (ManifestReader<DataFile> reader = ManifestFiles.read(manifest, table.io())) {
return !reader.iterator().next().path().equals(FILE_A.path());
} catch (IOException x) {
throw new RuntimeIOException(x);
}
})
.commit();

Assert.assertEquals(2, table.currentSnapshot().allManifests().size());

Expand Down Expand Up @@ -356,8 +356,8 @@ public void testConcurrentRewriteManifest() throws IOException {
public void testAppendDuringRewriteManifest() {
Table table = load();
table.newFastAppend()
.appendFile(FILE_A)
.commit();
.appendFile(FILE_A)
.commit();
long appendIdA = table.currentSnapshot().snapshotId();

// start the rewrite manifests
Expand All @@ -366,8 +366,8 @@ public void testAppendDuringRewriteManifest() {

// append a file
table.newFastAppend()
.appendFile(FILE_B)
.commit();
.appendFile(FILE_B)
.commit();
long appendIdB = table.currentSnapshot().snapshotId();

Assert.assertEquals(2, table.currentSnapshot().allManifests().size());
Expand Down Expand Up @@ -395,8 +395,8 @@ public void testAppendDuringRewriteManifest() {
public void testRewriteManifestDuringAppend() {
Table table = load();
table.newFastAppend()
.appendFile(FILE_A)
.commit();
.appendFile(FILE_A)
.commit();
long appendIdA = table.currentSnapshot().snapshotId();

// start an append
Expand All @@ -405,8 +405,8 @@ public void testRewriteManifestDuringAppend() {

// rewrite the manifests - only affects the first
table.rewriteManifests()
.clusterBy(file -> "file")
.commit();
.clusterBy(file -> "file")
.commit();

Assert.assertEquals(1, table.currentSnapshot().allManifests().size());

Expand Down
4 changes: 2 additions & 2 deletions mr/src/main/java/org/apache/iceberg/mr/mapred/MapredIcebergInputFormat.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,14 @@ public void close() throws IOException {

private static JobContext newJobContext(JobConf job) {
JobID jobID = Optional.ofNullable(JobID.forName(job.get(JobContext.ID)))
.orElse(new JobID());
.orElseGet(JobID::new);

return new JobContextImpl(job, jobID);
}

private static TaskAttemptContext newTaskAttemptContext(JobConf job, Reporter reporter) {
TaskAttemptID taskAttemptID = Optional.ofNullable(TaskAttemptID.forName(job.get(JobContext.TASK_ATTEMPT_ID)))
.orElse(new TaskAttemptID());
.orElseGet(TaskAttemptID::new);

return new TaskAttemptContextImpl(job, taskAttemptID, toStatusReporter(reporter));
}
Expand Down
2 changes: 1 addition & 1 deletion orc/src/main/java/org/apache/iceberg/orc/ORCSchemaUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ private static TypeDescription buildOrcProjection(Integer fieldId, Type type, bo
// e.g. renaming column c -> d and adding new column d
String name = Optional.ofNullable(mapping.get(nestedField.fieldId()))
.map(OrcField::name)
.orElse(nestedField.name() + "_r" + nestedField.fieldId());
.orElseGet(() -> nestedField.name() + "_r" + nestedField.fieldId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that it catches these.

TypeDescription childType = buildOrcProjection(nestedField.fieldId(), nestedField.type(),
isRequired && nestedField.isRequired(), mapping);
orcType.addField(name, childType);
Expand Down
7 changes: 4 additions & 3 deletions spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,9 @@ class Reader implements DataSourceReader, SupportsScanColumnarBatch, SupportsPus
} catch (IOException ioe) {
LOG.warn("Failed to get Hadoop Filesystem", ioe);
}
Boolean localityFallback = LOCALITY_WHITELIST_FS.contains(scheme);
this.localityPreferred = options.get("locality").map(Boolean::parseBoolean)
.orElse(LOCALITY_WHITELIST_FS.contains(scheme));
.orElse(localityFallback);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because schema is non-final we can't just switch this to a lambda

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think actually this would actually be fine, but to make the rules happy we would have to reassign Scheme anyway so I figured we may as well just put the result in a local var

Copy link
Contributor

@rdblue rdblue Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could refactor scheme detection into a method to make scheme effectively final here.

} else {
this.localityPreferred = false;
}
Expand All @@ -154,10 +155,10 @@ class Reader implements DataSourceReader, SupportsScanColumnarBatch, SupportsPus
this.encryptionManager = encryptionManager;
this.caseSensitive = caseSensitive;

this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElse(
this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
PropertyUtil.propertyAsBoolean(table.properties(),
TableProperties.PARQUET_VECTORIZATION_ENABLED, TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
this.batchSize = options.get("batch-size").map(Integer::parseInt).orElse(
this.batchSize = options.get("batch-size").map(Integer::parseInt).orElseGet(() ->
PropertyUtil.propertyAsInt(table.properties(),
TableProperties.PARQUET_BATCH_SIZE, TableProperties.PARQUET_BATCH_SIZE_DEFAULT));
}
Expand Down
2 changes: 1 addition & 1 deletion spark2/src/main/java/org/apache/iceberg/spark/source/Writer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class Writer implements DataSourceWriter {
private FileFormat getFileFormat(Map<String, String> tableProperties, DataSourceOptions options) {
Optional<String> formatOption = options.get("write-format");
String formatString = formatOption
.orElse(tableProperties.getOrDefault(DEFAULT_FILE_FORMAT, DEFAULT_FILE_FORMAT_DEFAULT));
.orElseGet(() -> tableProperties.getOrDefault(DEFAULT_FILE_FORMAT, DEFAULT_FILE_FORMAT_DEFAULT));
return FileFormat.valueOf(formatString.toUpperCase(Locale.ENGLISH));
}

Expand Down
2 changes: 1 addition & 1 deletion spark3/src/main/java/org/apache/iceberg/spark/source/SparkBatchWrite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class SparkBatchWrite implements BatchWrite {
protected FileFormat getFileFormat(Map<String, String> tableProperties, Map<String, String> options) {
Optional<String> formatOption = Optional.ofNullable(options.get("write-format"));
String formatString = formatOption
.orElse(tableProperties.getOrDefault(DEFAULT_FILE_FORMAT, DEFAULT_FILE_FORMAT_DEFAULT));
.orElseGet(() -> tableProperties.getOrDefault(DEFAULT_FILE_FORMAT, DEFAULT_FILE_FORMAT_DEFAULT));
return FileFormat.valueOf(formatString.toUpperCase(Locale.ENGLISH));
}

Expand Down