Apply Baseline to iceberg-pig #525
Conversation
Fokko
force-pushed
the
fd-fix-pig
branch
2 times, most recently
from
4773ee2
to
0a8271f
Compare
There was a problem hiding this comment.
LGTM! +1
It seems there are a few more checkstyle errors being reported by iceberg-pig module in the travis build https://travis-ci.org/apache/incubator-iceberg/jobs/595802545
|
Thanks for working on these, @Fokko! Great to see you in this community as well as Avro and Parquet! |
| @@ -73,6 +60,16 @@ private static ResourceFieldSchema convert(Type type) throws IOException { | |||
| return result; | |||
| } | |||
|
|
|||
| private static ResourceFieldSchema [] convertFields(List<Types.NestedField> fields) throws IOException { | |||
There was a problem hiding this comment.
seems like a non functional change? [method moved]
There was a problem hiding this comment.
Overloaded methods need to be grouped together.
| @@ -29,7 +29,7 @@ | |||
| import java.util.List; | |||
| import java.util.Map; | |||
| import java.util.Set; | |||
| import org.apache.commons.lang.SerializationUtils; | |||
| import org.apache.commons.lang3.SerializationUtils; | |||
There was a problem hiding this comment.
Why did we update the to this api?
There was a problem hiding this comment.
lang is being considered as deprecated, and the linter complaints that we should move to lang3.
| @@ -451,6 +451,8 @@ project(':iceberg-pig') { | |||
| compile project(':iceberg-core') | |||
| compile project(':iceberg-parquet') | |||
|
|
|||
| compile "org.apache.commons:commons-lang3" | |||
There was a problem hiding this comment.
Is there more context on the update to commons-lang3? Was this just an undeclared dependency? Why not use commons-lang?
There was a problem hiding this comment.
lang is being considered as deprecated, and the linter complaints that we should move to lang3. Therefore I've added it as a dependency. We only use it for serializing, de-serializing, so I'm fine to suppress the error as well. WDYT?
There was a problem hiding this comment.
Sounds fine to me, but we'll need to make sure this doesn't affect anything in the runtime Jar.
There was a problem hiding this comment.
Confirmed that this is provided by Spark and is excluded from the runtime Jar.
|
Any pending issues? |
|
Since this changes dependencies, I'm reluctant to merge it before the 0.7.0-incubating release is final. I plan to merge this and the one for iceberg-parquet shortly after that release. |
|
@Fokko, I think this is ready to be merged, but it conflicts with the iceberg-parquet commit. Could you fix it? Thanks! |
|
Sure @rdblue, fixed the merge conflicts |
|
|
||
| // We enable baseline-idea everywhere so that everyone can use IntelliJ to build code against the | ||
| // Baseline style guide. | ||
| def baselineProjects = [ project("iceberg-api"), project("iceberg-common"), project("iceberg-core"), |
|
|
|
+1, but it looks like the last push didn't get tested. I'm going to close this and re-open to trigger a build. |
Fixes #159