nastra commented on code in PR #7988: URL: https://github.com/apache/iceberg/pull/7988#discussion_r1255741969
########## gcp/src/test/java/org/apache/iceberg/gcp/gcs/TestUtils.java: ########## @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.gcp.gcs; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.EOFException; +import java.io.IOException; +import java.io.InputStream; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; +import java.io.OutputStream; +import java.io.Serializable; +import org.testcontainers.shaded.com.google.common.base.Preconditions; + +public class TestUtils { Review Comment: `TestUtils` indicates that this is a class containing tests for a `Utils` class, so maybe rename this to something else. Also is this a 1:1 copy of the `TestUtils` in aws? ########## hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java: ########## @@ -21,7 +21,6 @@ import java.nio.charset.StandardCharsets; import java.sql.Date; import java.sql.Timestamp; -import org.apache.commons.lang3.StringUtils; Review Comment: seems to be used here: https://github.com/apache/iceberg/blob/c07f2aabc0a1d02f068ecf1514d2479c0fbdd3b0/hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java#L70 ########## aliyun/src/test/java/org/apache/iceberg/aliyun/oss/mock/AliyunOSSMockLocalController.java: ########## @@ -276,4 +277,246 @@ public void setMessage(String message) { this.message = message; } } + + /** + * Reads bytes up to a maximum length, if its count goes above that, it stops. + * + * <p>This is useful to wrap ServletInputStreams. The ServletInputStream will block if you try to + * read content from it that isn't there, because it doesn't know whether the content hasn't + * arrived yet or whether the content has finished. So, one of these, initialized with the + * Content-length sent in the ServletInputStream's header, will stop it blocking, providing it's + * been sent with a correct content length. + * + * <p>This code is borrowed from `org.apache.commons:commons-io` + */ + public class BoundedInputStream extends FilterInputStream { Review Comment: maybe we should rewrite that one single place where `BoundedInputStream` is used in this file rather than introducing the entire class here? ########## dell/src/test/java/org/apache/iceberg/dell/mock/ecs/ObjectData.java: ########## @@ -20,18 +20,25 @@ import com.emc.object.Range; import com.emc.object.s3.S3ObjectMetadata; -import com.emc.object.shadow.org.apache.commons.codec.digest.DigestUtils; Review Comment: isn't that using a shaded version, meaning that we wouldn't have to change this file? ########## hive3/src/main/java/org/apache/iceberg/mr/hive/vector/HiveVectorizedReader.java: ########## @@ -21,7 +21,6 @@ import java.io.IOException; import java.util.List; import java.util.Map; -import org.apache.commons.lang3.ArrayUtils; Review Comment: seems to be used here: https://github.com/apache/iceberg/blob/c07f2aabc0a1d02f068ecf1514d2479c0fbdd3b0/hive3/src/main/java/org/apache/iceberg/mr/hive/vector/HiveVectorizedReader.java#L104 ########## gcp/src/test/java/org/apache/iceberg/gcp/gcs/TestUtils.java: ########## @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.gcp.gcs; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.EOFException; +import java.io.IOException; +import java.io.InputStream; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; +import java.io.OutputStream; +import java.io.Serializable; +import org.testcontainers.shaded.com.google.common.base.Preconditions; + +public class TestUtils { Review Comment: I'm not sure it's a good idea to have multiple copies of the exact same file, maybe that should live under tests in `iceberg-core`? ########## delta-lake/src/integration/java/org/apache/iceberg/delta/TestSnapshotDeltaLakeTable.java: ########## @@ -532,13 +530,14 @@ private void addExternalDatafiles( private static String getFullFilePath(String path, String tableRoot) { URI dataFileUri = URI.create(path); try { - String decodedPath = new URLCodec().decode(path); + String decodedPath = java.net.URLDecoder.decode(path, StandardCharsets.UTF_8.name()); Review Comment: ```suggestion String decodedPath = URLDecoder.decode(path, StandardCharsets.UTF_8.name()); ``` and then please import ``` import java.net.URLDecoder; import java.nio.charset.StandardCharsets; ``` to fix CI failures. Also please run `./gradlew spotlessApply` to fix any formatting issues ########## aws/src/test/java/org/apache/iceberg/aws/s3/TestUtils.java: ########## @@ -0,0 +1,201 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.aws.s3; + +import org.testcontainers.shaded.com.google.common.base.Preconditions; Review Comment: wrong import, should be `org.apache.iceberg.relocated.com.google....` ########## gcp/src/test/java/org/apache/iceberg/gcp/gcs/TestUtils.java: ########## @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.gcp.gcs; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.EOFException; +import java.io.IOException; +import java.io.InputStream; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; +import java.io.OutputStream; +import java.io.Serializable; +import org.testcontainers.shaded.com.google.common.base.Preconditions; Review Comment: wrong import, should be `org.apache.iceberg.relocated.com.google....` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
