You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/03/23 14:41:07 UTC
[GitHub] [iceberg] felixYyu opened a new pull request #4376: ALL：fixing static analysis warnings
felixYyu opened a new pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376
this PR fixed these grouping parentheses are unnecessary. cc @rdblue @RussellSpitzer
```
iceberg\core\src\main\java\org\apache\iceberg\BaseFileScanTask.java:224: [UnnecessaryParentheses] These grouping parentheses are unnecessary; it is unlikely the code will be misinterpreted without them
(this.file().equals(other.file())) &&
^
(see https://errorprone.info/bugpattern/UnnecessaryParentheses)
Did you mean 'this.file().equals(other.file()) &&'?
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833861603
##########
File path: core/src/main/java/org/apache/iceberg/ScanSummary.java
##########
@@ -295,7 +295,7 @@ private PartitionMetrics updateFromFile(ContentFile<?> file, Long timestampMilli
@Override
public String toString() {
String dataTimestamp = dataTimestampMillis != null ?
- new Date(dataTimestampMillis).toString() : null;
+ Instant.ofEpochMilli(dataTimestampMillis).toString() : null;
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1083856002
The task was not fully completed，It will be finished today. @RussellSpitzer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838067187
##########
File path: hive3/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergTimestampObjectInspectorHive3.java
##########
@@ -50,6 +50,7 @@ public LocalDateTime convert(Object o) {
}
@Override
+ @SuppressWarnings("JavaLocalDateTimeGetNano")
Review comment:
fixed with Timestamp.valueOf(LocalDateTime time)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833372101
##########
File path: api/src/main/java/org/apache/iceberg/util/CharSequenceSet.java
##########
@@ -172,7 +172,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- return Objects.hash(wrapperSet);
+ return Objects.hashCode(wrapperSet);
Review comment:
I'm not very comfortable changing this because it's a public api. While I don't think we actually serialize this anywhere, if someone did this would probably cause an issue. That said if as a community we feel confident this isn't going to be breaking we can change it.
Objects.hash will hash the hash of wrapperSet
Objects.hashCode will hash wrapperSet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
changelist
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
SnapshotRef.java ObjectEqualsForPrimitives
CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
MappedFields.java ObjectsHashCodeUnnecessaryVarargs
MetricsModes.java ObjectsHashCodeUnnecessaryVarargs
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838040974
##########
File path: arrow/src/main/java/org/apache/iceberg/arrow/vectorized/ArrowReader.java
##########
@@ -268,7 +268,7 @@ public void close() throws IOException {
.map(entry -> EncryptedFiles.encryptedInput(io.newInputFile(entry.getKey()), entry.getValue()));
// decrypt with the batch call to avoid multiple RPCs to a key server, if possible
- Iterable<InputFile> decryptedFiles = encryptionManager.decrypt(encrypted::iterator);
+ Iterable<InputFile> decryptedFiles = encryptionManager.decrypt(encrypted.collect(Collectors.toList()));
Review comment:
revert this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
changelist-v2 cc @RussellSpitzer
```
Object Warnings
------------------------------------------------------------------------------------
Tasks.java StreamToIterable--**revert**/new Runnable() lambda
Avro.java UnnecessaryLambda
--InputFilesDecryptor.java StreamToIterable--**revert**
--ArrowReader.java StreamToIterable--**revert**
OrcSplit.java MissingSummary
IcebergTimestampObjectInspectorHive3.java JavaLocalDateTimeGetNano
HiveTableOperations.java ReverseDnsLookup
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833374276
##########
File path: core/src/main/java/org/apache/iceberg/MetricsModes.java
##########
@@ -134,7 +134,7 @@ public boolean equals(Object other) {
@Override
public int hashCode() {
- return Objects.hash(length);
+ return Integer.hashCode(length);
Review comment:
I'm not as worried about this change since it's in Core, although this will change the underlying value
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r832768046
##########
File path: core/src/main/java/org/apache/iceberg/BaseFileScanTask.java
##########
@@ -220,12 +220,13 @@ public Expression residual() {
}
public boolean isAdjacent(SplitScanTask other) {
- return (other != null) &&
- (this.file().equals(other.file())) &&
- (this.offset + this.len == other.offset);
+ return other != null &&
+ this.file().equals(other.file()) &&
+ this.offset + this.len == other.offset;
}
}
+ @SuppressWarnings("MixedMutabilityReturnType")
Review comment:
I will fixing all static analysis warnings with this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
changelist
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
--SnapshotRef.java ObjectEqualsForPrimitives--**revert**
--CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs--**revert**
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
MappedFields.java ObjectsHashCodeUnnecessaryVarargs
--MetricsModes.java ObjectsHashCodeUnnecessaryVarargs--**revert**
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833854299
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -86,7 +86,7 @@ public boolean equals(Object other) {
}
SnapshotRef ref = (SnapshotRef) other;
- return Objects.equals(ref.snapshotId(), snapshotId) &&
+ return ref.snapshotId() == snapshotId &&
Review comment:
revert
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838060626
##########
File path: flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
for (PartitionField field : spec.fields()) {
if (field.transform().isIdentity()) {
- partitionKeys.add(icebergSchema.findColumnName(field.sourceId()));
+ partitionKeysBuilder.add(icebergSchema.findColumnName(field.sourceId()));
} else {
// Not created by Flink SQL.
// For compatibility with iceberg tables, return empty.
// TODO modify this after Flink support partition transform.
- return Collections.emptyList();
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837630103
##########
File path: arrow/src/main/java/org/apache/iceberg/arrow/vectorized/ArrowReader.java
##########
@@ -268,7 +268,7 @@ public void close() throws IOException {
.map(entry -> EncryptedFiles.encryptedInput(io.newInputFile(entry.getKey()), entry.getValue()));
// decrypt with the batch call to avoid multiple RPCs to a key server, if possible
- Iterable<InputFile> decryptedFiles = encryptionManager.decrypt(encrypted::iterator);
+ Iterable<InputFile> decryptedFiles = encryptionManager.decrypt(encrypted.collect(Collectors.toList()));
Review comment:
This will fully materialize the list so I'm not sure we want to change the approach here. Currently the only usages do single passes over the iterable (see the forEach below) ... It is probably fine to suppress here but I also don't think we lose to much to much by materializing right here since we immediately put our results into a materialized map anyway.
I guess i'm leaning towards this being ok at the moment although I don't think it materially improves the code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
changelist
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
##SnapshotRef.java ObjectEqualsForPrimitives
##CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
MappedFields.java ObjectsHashCodeUnnecessaryVarargs
MetricsModes.java ObjectsHashCodeUnnecessaryVarargs
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on pull request #4376: Core：fixed unnecessary parentheses in build
Posted by GitBox <gi...@apache.org>.
felixYyu commented on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1075822276
Can you combine some of these style prs and perhaps label them as fixing static analysis warnings
is a good idea, I try do it. @RussellSpitzer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833381491
##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetricsRowGroupFilter.java
##########
@@ -565,6 +565,7 @@ private static boolean mayContainNull(Statistics statistics) {
return !statistics.isNumNullsSet() || statistics.getNumNulls() > 0;
}
+ @SuppressWarnings("UnusedMethod")
Review comment:
If this is really unused shouldn't we be removing it? Is it needed for something via reflection?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838060791
##########
File path: flink/v1.13/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837647822
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java
##########
@@ -32,20 +32,20 @@
final class JdbcUtil {
// Catalog Table
- protected static final String CATALOG_TABLE_NAME = "iceberg_tables";
- protected static final String CATALOG_NAME = "catalog_name";
- protected static final String TABLE_NAMESPACE = "table_namespace";
- protected static final String TABLE_NAME = "table_name";
- protected static final String METADATA_LOCATION = "metadata_location";
- protected static final String PREVIOUS_METADATA_LOCATION = "previous_metadata_location";
+ public static final String CATALOG_TABLE_NAME = "iceberg_tables";
Review comment:
I forgot if we checked this, but could these all be package private?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837740386
##########
File path: flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
for (PartitionField field : spec.fields()) {
if (field.transform().isIdentity()) {
- partitionKeys.add(icebergSchema.findColumnName(field.sourceId()));
+ partitionKeysBuilder.add(icebergSchema.findColumnName(field.sourceId()));
} else {
// Not created by Flink SQL.
// For compatibility with iceberg tables, return empty.
// TODO modify this after Flink support partition transform.
- return Collections.emptyList();
+ return partitionKeys;
}
}
+ partitionKeys = partitionKeysBuilder.build();
return partitionKeys;
Review comment:
This could return partitionKeysBuilder.build()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837746097
##########
File path: hive3/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergTimestampObjectInspectorHive3.java
##########
@@ -50,6 +50,7 @@ public LocalDateTime convert(Object o) {
}
@Override
+ @SuppressWarnings("JavaLocalDateTimeGetNano")
Review comment:
Why is this ok to surpress, it does look like we are doing this incorrectly here, shouldn't this be Timestamp.valueOf(LocalDateTime time)?
I would separate this out into another PR since we could be doing a semantic change here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer closed pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer closed pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833884938
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -486,6 +486,7 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
return builder.build();
}
+ @SuppressWarnings("MixedMutabilityReturnType")
Review comment:
fix using an immutable list builder
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
updated list:
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
SnapshotRef.java ObjectEqualsForPrimitives
CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
MappedFields.java ObjectsHashCodeUnnecessaryVarargs
MetricsModes.java ObjectsHashCodeUnnecessaryVarargs
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833375085
##########
File path: core/src/main/java/org/apache/iceberg/ScanSummary.java
##########
@@ -295,7 +295,7 @@ private PartitionMetrics updateFromFile(ContentFile<?> file, Long timestampMilli
@Override
public String toString() {
String dataTimestamp = dataTimestampMillis != null ?
- new Date(dataTimestampMillis).toString() : null;
+ Instant.ofEpochMilli(dataTimestampMillis).toString() : null;
Review comment:
nit: Indentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
changelist-v2
```
Object Warnings
------------------------------------------------------------------------------------
Tasks.java StreamToIterable/new Runnable() lambda
Avro.java UnnecessaryLambda
InputFilesDecryptor.java StreamToIterable
ArrowReader.java StreamToIterable
OrcSplit.java MissingSummary
IcebergTimestampObjectInspectorHive3.java JavaLocalDateTimeGetNano
HiveTableOperations.java ReverseDnsLookup
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
changelist-v2
```
Object Warnings
------------------------------------------------------------------------------------
Tasks.java StreamToIterable/new Runnable() lambda
Avro.java UnnecessaryLambda
InputFilesDecryptor.java StreamToIterable
ArrowReader.java StreamToIterable
OrcSplit.java MissingSummary
IcebergTimestampObjectInspectorHive3.java JavaLocalDateTimeGetNano
HiveTableOperations.java ReverseDnsLookup
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
changelist-v2
```
Object Warnings
------------------------------------------------------------------------------------
Tasks.java StreamToIterable/new Runnable() lambda
Avro.java UnnecessaryLambda
InputFilesDecryptor.java StreamToIterable
ArrowReader.java StreamToIterable
OrcSplit.java MissingSummary
IcebergTimestampObjectInspectorHive3.java JavaLocalDateTimeGetNano
HiveTableOperations.java ReverseDnsLookup
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838067187
##########
File path: hive3/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergTimestampObjectInspectorHive3.java
##########
@@ -50,6 +50,7 @@ public LocalDateTime convert(Object o) {
}
@Override
+ @SuppressWarnings("JavaLocalDateTimeGetNano")
Review comment:
I change to `return o == null ? null : Timestamp.valueOf((LocalDateTime) o);` ,but the check were not successful
```
/home/runner/work/iceberg/iceberg/hive3/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergTimestampObjectInspectorHive3.java:54: error: incompatible types: java.time.LocalDateTime cannot be converted to java.lang.String
return o == null ? null : Timestamp.valueOf((LocalDateTime) o);
```
so revert this, but why IcebergTimestampObjectInspector.getPrimitiveJavaObject is ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833378062
##########
File path: core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java
##########
@@ -27,7 +27,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
/**
- *
+ * Table metadata result when loading a table
Review comment:
I believe @rdblue is in the middle of following up here as well, I think in this case I would just drop the empty javadoc in anticipation of a commit which adds a more specific doc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833376499
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java
##########
@@ -30,6 +30,7 @@
import org.apache.iceberg.relocated.com.google.common.base.Splitter;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+@SuppressWarnings("ProtectedMembersInFinalClass")
Review comment:
Why are we suppressing here, shouldn't we be changing the members to private?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
changelist
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
> SnapshotRef.java ObjectEqualsForPrimitives
> CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
MappedFields.java ObjectsHashCodeUnnecessaryVarargs
MetricsModes.java ObjectsHashCodeUnnecessaryVarargs
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
changelist
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
--SnapshotRef.java ObjectEqualsForPrimitives--**revert**
--CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs--**revert**
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
--MappedFields.java ObjectsHashCodeUnnecessaryVarargs--**revert**
--MetricsModes.java ObjectsHashCodeUnnecessaryVarargs--**revert**
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
--StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs--**revert**
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
--ParquetMetricsRowGroupFilter.java UnusedMethod--**revert**
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
changelist
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
--SnapshotRef.java ObjectEqualsForPrimitives--**revert**
--CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs--**revert**
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
--MappedFields.java ObjectsHashCodeUnnecessaryVarargs--**revert**
--MetricsModes.java ObjectsHashCodeUnnecessaryVarargs--**revert**
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
--StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs--**revert**
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833884938
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -486,6 +486,7 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
return builder.build();
}
+ @SuppressWarnings("MixedMutabilityReturnType")
Review comment:
fix using an immutable list builder contains v1.14 v1.13 v1.12
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833887958
##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetricsRowGroupFilter.java
##########
@@ -565,6 +565,7 @@ private static boolean mayContainNull(Statistics statistics) {
return !statistics.isNumNullsSet() || statistics.getNumNulls() > 0;
}
+ @SuppressWarnings("UnusedMethod")
Review comment:
not sure via reflection, so revert
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
changelist-v2
```
Object Warnings
------------------------------------------------------------------------------------
Tasks.java StreamToIterable/new Runnable() lambda
Avro.java UnnecessaryLambda
InputFilesDecryptor.java StreamToIterable
ArrowReader.java StreamToIterable
OrcSplit.java MissingSummary
IcebergTimestampObjectInspectorHive3.java JavaLocalDateTimeGetNano
HiveTableOperations.java ReverseDnsLookup
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
changelist
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
_SnapshotRef.java ObjectEqualsForPrimitives
CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs_
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
MappedFields.java ObjectsHashCodeUnnecessaryVarargs
MetricsModes.java ObjectsHashCodeUnnecessaryVarargs
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
changelist-v2
```
Object Warnings
------------------------------------------------------------------------------------
Tasks.java StreamToIterable/new Runnable() lambda
Avro.java UnnecessaryLambda
InputFilesDecryptor.java StreamToIterable
ArrowReader.java StreamToIterable
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833858885
##########
File path: core/src/main/java/org/apache/iceberg/MetricsModes.java
##########
@@ -134,7 +134,7 @@ public boolean equals(Object other) {
@Override
public int hashCode() {
- return Objects.hash(length);
+ return Integer.hashCode(length);
Review comment:
to maintain hash consistency, so revert
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838060561
##########
File path: flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu closed pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu closed pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
updated list:
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
SnapshotRef.java ObjectEqualsForPrimitives
CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
MappedFields.java ObjectsHashCodeUnnecessaryVarargs
MetricsModes.java ObjectsHashCodeUnnecessaryVarargs
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833867498
##########
File path: core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java
##########
@@ -27,7 +27,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
/**
- *
+ * Table metadata result when loading a table
Review comment:
fix via drop the empty javadoc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
changelist-v2
```
Object Warnings
------------------------------------------------------------------------------------
Tasks.java StreamToIterable/new Runnable() lambda
Avro.java UnnecessaryLambda
InputFilesDecryptor.java StreamToIterable
ArrowReader.java StreamToIterable
OrcSplit.java MissingSummary
IcebergTimestampObjectInspectorHive3.java JavaLocalDateTimeGetNano
HiveTableOperations.java ReverseDnsLookup
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
changelist-v2 cc @RussellSpitzer
```
Object Warnings
------------------------------------------------------------------------------------
Tasks.java StreamToIterable/new Runnable() lambda
Avro.java UnnecessaryLambda
InputFilesDecryptor.java StreamToIterable
ArrowReader.java StreamToIterable
OrcSplit.java MissingSummary
IcebergTimestampObjectInspectorHive3.java JavaLocalDateTimeGetNano
HiveTableOperations.java ReverseDnsLookup
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838060715
##########
File path: flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
for (PartitionField field : spec.fields()) {
if (field.transform().isIdentity()) {
- partitionKeys.add(icebergSchema.findColumnName(field.sourceId()));
+ partitionKeysBuilder.add(icebergSchema.findColumnName(field.sourceId()));
} else {
// Not created by Flink SQL.
// For compatibility with iceberg tables, return empty.
// TODO modify this after Flink support partition transform.
- return Collections.emptyList();
+ return partitionKeys;
}
}
+ partitionKeys = partitionKeysBuilder.build();
return partitionKeys;
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838045351
##########
File path: core/src/main/java/org/apache/iceberg/BaseFilesTable.java
##########
@@ -108,7 +108,8 @@ public TableScan appendsAfter(long fromSnapshotId) {
}
/**
- * @return list of manifest files to explore for this files metadata table scan
+ * Returns list of manifest files to explore for this file's metadata table scan
Review comment:
thanks，I got it and fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837739894
##########
File path: flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
Review comment:
If we are using the builder we don't need the list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837632937
##########
File path: core/src/main/java/org/apache/iceberg/BaseFilesTable.java
##########
@@ -108,7 +108,8 @@ public TableScan appendsAfter(long fromSnapshotId) {
}
/**
- * @return list of manifest files to explore for this files metadata table scan
+ * Returns list of manifest files to explore for this file's metadata table scan
Review comment:
For this kind of one line doc, we just do something like
```
/**
* Returns the {@link Schema} for this sort order
*/
```
If you don't flag Return as `@return` then you'll make the Javadoc with a summary and the function's return type will be clear
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #4376: Core：fixed unnecessary parentheses in build
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1075234554
Can you combine some of these style prs and perhaps label them as fixing static analysis warnings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833366740
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -86,7 +86,7 @@ public boolean equals(Object other) {
}
SnapshotRef ref = (SnapshotRef) other;
- return Objects.equals(ref.snapshotId(), snapshotId) &&
+ return ref.snapshotId() == snapshotId &&
Review comment:
I think it's fine if we disable the static analysis rule for this, just because I like it when all the comparisons look the same but if we want to follow this rule that's also ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
changelist
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
--SnapshotRef.java ObjectEqualsForPrimitives--**revert**
--CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs--**revert**
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
MappedFields.java ObjectsHashCodeUnnecessaryVarargs
MetricsModes.java ObjectsHashCodeUnnecessaryVarargs
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833865516
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java
##########
@@ -30,6 +30,7 @@
import org.apache.iceberg.relocated.com.google.common.base.Splitter;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+@SuppressWarnings("ProtectedMembersInFinalClass")
Review comment:
because the JdbcUtil's vars are used in orther class, so i want to change protected members to public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1081476418
please review again，thanks @RussellSpitzer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837740094
##########
File path: flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
for (PartitionField field : spec.fields()) {
if (field.transform().isIdentity()) {
- partitionKeys.add(icebergSchema.findColumnName(field.sourceId()));
+ partitionKeysBuilder.add(icebergSchema.findColumnName(field.sourceId()));
} else {
// Not created by Flink SQL.
// For compatibility with iceberg tables, return empty.
// TODO modify this after Flink support partition transform.
- return Collections.emptyList();
Review comment:
This could stay Collections.emptyList()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837740833
##########
File path: flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
Review comment:
Since we don't need `partitionKeys` it may be ok to just call this builder `partitionKeys`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1082564173
I will separate out the changes which make semantic changes to the code into their own PR's.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838048630
##########
File path: core/src/main/java/org/apache/iceberg/util/Tasks.java
##########
@@ -570,7 +566,7 @@ public Integer next() {
}
public static <I> Builder<I> foreach(Stream<I> items) {
- return new Builder<>(items::iterator);
+ return new Builder<>(items.collect(Collectors.toList()));
Review comment:
as ArrowReader，revert this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1083856002
The task was not fully completed，don't turn it off. It will be finished today. @RussellSpitzer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1077285033
changelist-v2 cc @RussellSpitzer
```
Object Warnings
------------------------------------------------------------------------------------
Tasks.java StreamToIterable--**revert**/new Runnable() lambda
Avro.java UnnecessaryLambda
--InputFilesDecryptor.java StreamToIterable--**revert**
--ArrowReader.java StreamToIterable--**revert**
OrcSplit.java MissingSummary
--IcebergTimestampObjectInspectorHive3.java JavaLocalDateTimeGetNano--**revert**
HiveTableOperations.java ReverseDnsLookup
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838047860
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java
##########
@@ -32,20 +32,20 @@
final class JdbcUtil {
// Catalog Table
- protected static final String CATALOG_TABLE_NAME = "iceberg_tables";
- protected static final String CATALOG_NAME = "catalog_name";
- protected static final String TABLE_NAMESPACE = "table_namespace";
- protected static final String TABLE_NAME = "table_name";
- protected static final String METADATA_LOCATION = "metadata_location";
- protected static final String PREVIOUS_METADATA_LOCATION = "previous_metadata_location";
+ public static final String CATALOG_TABLE_NAME = "iceberg_tables";
Review comment:
both JdbcCatalog and JdbcTableOperations need the var of JdbcUtil, so I think the vars of JdbcUtil need to be public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r838067187
##########
File path: hive3/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergTimestampObjectInspectorHive3.java
##########
@@ -50,6 +50,7 @@ public LocalDateTime convert(Object o) {
}
@Override
+ @SuppressWarnings("JavaLocalDateTimeGetNano")
Review comment:
I change to `return o == null ? null : Timestamp.valueOf((LocalDateTime) o);` ,but the check were not successful ```
/home/runner/work/iceberg/iceberg/hive3/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergTimestampObjectInspectorHive3.java:54: error: incompatible types: java.time.LocalDateTime cannot be converted to java.lang.String
return o == null ? null : Timestamp.valueOf((LocalDateTime) o);
```
so revert this, but why IcebergTimestampObjectInspector.getPrimitiveJavaObject is ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837740833
##########
File path: flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
Review comment:
Since we don't need `partitionKeys` the list, it may be ok to just call this builder `partitionKeys`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837738807
##########
File path: core/src/main/java/org/apache/iceberg/util/Tasks.java
##########
@@ -570,7 +566,7 @@ public Integer next() {
}
public static <I> Builder<I> foreach(Stream<I> items) {
- return new Builder<>(items::iterator);
+ return new Builder<>(items.collect(Collectors.toList()));
Review comment:
Our "iterator<I> items" already makes an iterable out of an iterator so I don't think we should avoid it here. This change also forces a materialization so we should avoid it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r837741368
##########
File path: flink/v1.13/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -488,16 +489,18 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
private static List<String> toPartitionKeys(PartitionSpec spec, Schema icebergSchema) {
List<String> partitionKeys = Lists.newArrayList();
+ ImmutableList.Builder<String> partitionKeysBuilder = ImmutableList.builder();
Review comment:
same comments as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: Core：fixed unnecessary parentheses in build
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1075822276
> Can you combine some of these style prs and perhaps label them as fixing static analysis warnings
is a good idea, I try do it. @RussellSpitzer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4376: Core：fixed unnecessary parentheses in build
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r832549419
##########
File path: core/src/main/java/org/apache/iceberg/BaseFileScanTask.java
##########
@@ -220,12 +220,13 @@ public Expression residual() {
}
public boolean isAdjacent(SplitScanTask other) {
- return (other != null) &&
- (this.file().equals(other.file())) &&
- (this.offset + this.len == other.offset);
+ return other != null &&
+ this.file().equals(other.file()) &&
+ this.offset + this.len == other.offset;
}
}
+ @SuppressWarnings("MixedMutabilityReturnType")
Review comment:
This seems unrelated to the parentheses change. What is the purpose of this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833379640
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -486,6 +486,7 @@ private static PartitionSpec toPartitionSpec(List<String> partitionKeys, Schema
return builder.build();
}
+ @SuppressWarnings("MixedMutabilityReturnType")
Review comment:
Not a huge deal, but couldn't we fix this by just using an immutable list builder? That way we would have the same mutability in the return type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu edited a comment on pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu edited a comment on pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#issuecomment-1076456495
changelist
```
Object Warnings
------------------------------------------------------------------------------------
BaseFileScanTask.java UnnecessaryParentheses/MixedMutabilityReturnType
BaseFilesTable.java SummaryFragment
FlinkCatalog.java MixedMutabilityReturnType
FlinkSplitPlanner.java LambdaMethodReference
--SnapshotRef.java ObjectEqualsForPrimitives--**revert**
--CharSequenceSet.java ObjectsHashCodeUnnecessaryVarargs--**revert**
DynClasses.java ImplicitPublicBuilderConstructor
DynFields.java ImplicitPublicBuilderConstructor
MappedFields.java ObjectsHashCodeUnnecessaryVarargs
--MetricsModes.java ObjectsHashCodeUnnecessaryVarargs--**revert**
BuildAvroProjection.java UnnecessarilyQualified
SnapshotSummary.java ImplicitPublicBuilderConstructor
StructLikeSet.java ObjectsHashCodeUnnecessaryVarargs
JdbcUtil.java ProtectedMembersInFinalClass
RemoveSnapshots.java JavaUtilDate
LoadTableResponse.java MissingSummary
ScanSummary.java JavaUtilDate
ParquetMetricsRowGroupFilter.java UnusedMethod
IcebergSplit.java MutablePublicArray
TestIcebergInputFormats.java MutablePublicArray
------------------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] felixYyu commented on a change in pull request #4376: ALL：fixing static analysis warnings
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4376:
URL: https://github.com/apache/iceberg/pull/4376#discussion_r833857068
##########
File path: api/src/main/java/org/apache/iceberg/util/CharSequenceSet.java
##########
@@ -172,7 +172,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- return Objects.hash(wrapperSet);
+ return Objects.hashCode(wrapperSet);
Review comment:
revert
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org