You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Neuw84 (via GitHub)" <gi...@apache.org> on 2023/07/05 07:40:44 UTC
[GitHub] [iceberg] Neuw84 opened a new pull request, #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Neuw84 opened a new pull request, #7988:
URL: https://github.com/apache/iceberg/pull/7988
The followed approach is to use native Java 8 code when the alternative was clear or replicate/borrow when not following iceberg-core project practice.
Flink and Spark projects declare Apache Commons as direct dependency and therefore on those subprojects the dependency is not removed.
Closes #7658
--
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] Neuw84 commented on pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#issuecomment-1633918501
Done with the required changes.
For `ManifestReadBenchmark` I have also changed the `Files.createTemp` from Guava as it collides with `java.nio.File.Files` and the change was small.
--
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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1261073846
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,10 +66,11 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null && !prefixes.trim().isEmpty()) {
+ // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
+ // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
+ return null;
+ }
Review Comment:
I think } is an extra one and needs to be removed?
##########
aws/src/test/java/org/apache/iceberg/aws/s3/S3CommonUtils.java:
##########
@@ -0,0 +1,199 @@
+/*
+ * 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 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.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class S3CommonUtils {
+
+ private S3CommonUtils() {}
+ /**
+ * Deserializes a single {@link Object} from an array of bytes.
+ *
+ * <p>If the call site incorrectly types the return value, a {@link ClassCastException} is thrown
+ * from the call site. Without Generics in this declaration, the call site must type cast and can
+ * cause the same ClassCastException. Note that in both cases, the ClassCastException is in the
+ * call site, not in this method.
+ *
+ * <p>This code is borrowed from `org.apache.commons:commons-lang3`
+ *
+ * @param <T> the object type to be deserialized
+ * @param objectData the serialized object, must not be null
+ * @return the deserialized object
+ * @throws NullPointerException if {@code objectData} is {@code null}
+ * @throws IOException (runtime) if the serialization fails
+ */
+ public static <T> T deserialize(final byte[] objectData)
+ throws IOException, ClassNotFoundException {
+ Preconditions.checkNotNull(objectData, "objectData");
+ return deserialize(new ByteArrayInputStream(objectData));
+ }
+
+ /**
+ * Deserializes an {@link Object} from the specified stream.
+ *
+ * <p>The stream will be closed once the object is written. This avoids the need for a finally
+ * clause, and maybe also exception handling, in the application code.
+ *
+ * <p>The stream passed in is not buffered internally within this method. This is the
+ * responsibility of your application if desired.
+ *
+ * <p>If the call site incorrectly types the return value, a {@link ClassCastException} is thrown
+ * from the call site. Without Generics in this declaration, the call site must type cast and can
+ * cause the same ClassCastException. Note that in both cases, the ClassCastException is in the
+ * call site, not in this method.
+ *
+ * <p>This code is borrowed from `org.apache.commons:commons-lang3`
+ *
+ * @param <T> the object type to be deserialized
+ * @param inputStream the serialized object input stream, must not be null
+ * @return the deserialized object
+ * @throws NullPointerException if {@code inputStream} is {@code null}
+ * @throws IOException (runtime) if the serialization fails
+ * @throws ClassNotFoundException if Class is not found
+ */
+ public static <T> T deserialize(final InputStream inputStream)
+ throws IOException, ClassNotFoundException {
+ Preconditions.checkNotNull(inputStream, "inputStream");
+ try (ObjectInputStream in = new ObjectInputStream(inputStream)) {
+ @SuppressWarnings("unchecked")
+ final T obj = (T) in.readObject();
+ return obj;
+ }
+ }
+ /**
Review Comment:
nit: newline missing
--
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] nastra commented on pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#issuecomment-1645177374
@Neuw84 could you please rebase against latest master and squash all commits together?
--
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] Neuw84 commented on pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#issuecomment-1625092346
Hi @nastra,
Addressed your comments in the latest push, let me know your thoughts.
One question, there was an error in the tests:
`package StandardCharsets does not exist`
I am assuming that we are using Java 1.8 for the project no? Maybe there is some issue on the Github Actions config (although is weird because that package was introduced on 1.7).
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1262111945
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,10 +66,10 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null && !prefixes.trim().isEmpty()) {
+ // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
Review Comment:
yes, fixed on latest commit.
If you can retry the CI/CD pipeline it will be great.
Thanks!
--
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] Neuw84 commented on pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#issuecomment-1645168909
Hi @nastra, @Fokko,
I have merged the changes from this pull request ( some tests changed)
https://github.com/apache/iceberg/pull/7996
Let me know whether I need to change anything!
--
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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1258440265
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestCommonsS3Utils.java:
##########
@@ -0,0 +1,199 @@
+/*
+ * 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 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.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TestCommonsS3Utils {
Review Comment:
can we remove the `Test` prefix here and the `Test` suffix in `GCSCommonUtilsTest` as `Test` indicates that it contains tests. Why not just `IOUtils` like it was named before?
--
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] Neuw84 commented on pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#issuecomment-1628606712
Hi @nastra,
Have addressed your comments and applied spotless everywhere.
About the open questions let me know what your feelings are.
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259290534
##########
dell/src/test/java/org/apache/iceberg/dell/mock/ecs/ObjectData.java:
##########
@@ -70,7 +72,15 @@ public InputStream createInputStream(Range range) {
public S3ObjectMetadata createFullMetadata() {
S3ObjectMetadata metadata = new S3ObjectMetadata();
- metadata.setETag(DigestUtils.md5Hex(content));
+ MessageDigest md = null;
+ try {
+ md = MessageDigest.getInstance("MD5");
+ } catch (NoSuchAlgorithmException e) {
+ throw new RuntimeException(e);
+ }
+ md.update(content);
+ byte[] digest = md.digest();
+ metadata.setETag(DatatypeConverter.printHexBinary(digest));
Review Comment:
Agreed, that is not a long term solution as for Java 9 onwards we are going to need another library.
Fixing with native 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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1254451323
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -662,15 +681,4 @@ private List<String> filterByExtension(String tablePath, String extension) {
.filter(f -> f.endsWith(extension))
.collect(Collectors.toList());
}
-
- private static String addRecordsToFile(Table table, String filename) throws IOException {
Review Comment:
why the change to move this from here to the top?
##########
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:
should compilation not fail here?
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetAvro.java:
##########
@@ -41,6 +40,74 @@
class ParquetAvro {
+ static final GenericData DEFAULT_MODEL =
Review Comment:
it's hard to follow which changes were exactly done in this file due to the large diff
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257903561
##########
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:
As it is test code that is not going to be shipped I just though that this was good enough. Open to suggestions here ( it is not very clear why the code is using `BoundedInputStream` and I did not wanted to mess with it.
Open to suggestions/guidance 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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1258452021
##########
dell/src/test/java/org/apache/iceberg/dell/mock/ecs/ObjectData.java:
##########
@@ -70,7 +72,15 @@ public InputStream createInputStream(Range range) {
public S3ObjectMetadata createFullMetadata() {
S3ObjectMetadata metadata = new S3ObjectMetadata();
- metadata.setETag(DigestUtils.md5Hex(content));
+ MessageDigest md = null;
+ try {
+ md = MessageDigest.getInstance("MD5");
+ } catch (NoSuchAlgorithmException e) {
+ throw new RuntimeException(e);
+ }
+ md.update(content);
+ byte[] digest = md.digest();
+ metadata.setETag(DatatypeConverter.printHexBinary(digest));
Review Comment:
is it a good idea to replace the shaded lib usage with `javax.xml.bind.DatatypeConverter` or could we achieve the same without another dependency?
--
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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1262067081
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,10 +66,10 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null && !prefixes.trim().isEmpty()) {
+ // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
Review Comment:
formatting seems to be off?
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259290849
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestCommonsS3Utils.java:
##########
@@ -0,0 +1,199 @@
+/*
+ * 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 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.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TestCommonsS3Utils {
Review Comment:
Done!
--
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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259865545
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,12 +66,13 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null) {
+ if (!prefixes.trim().isEmpty()) {
Review Comment:
both ifs can be folded into a single if like in the previous version of the code
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,12 +66,13 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null) {
+ if (!prefixes.trim().isEmpty()) {
+ // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
+ // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
+ return null;
+ }
}
-
Review Comment:
nit: unnecessary newline removal
--
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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259861778
##########
delta-lake/src/integration/java/org/apache/iceberg/delta/TestSnapshotDeltaLakeTable.java:
##########
@@ -532,13 +533,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 = URLDecoder.decode(path, StandardCharsets.UTF_8.name());
if (dataFileUri.isAbsolute()) {
return decodedPath;
} else {
return tableRoot + File.separator + decodedPath;
}
- } catch (DecoderException e) {
+ } catch (UnsupportedEncodingException e) {
+ // In Java 10+ this will not need try-catch
Review Comment:
is that comment relevant? I think we can probably remove 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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1261086667
##########
aws/src/test/java/org/apache/iceberg/aws/s3/S3CommonUtils.java:
##########
@@ -0,0 +1,199 @@
+/*
+ * 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 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.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class S3CommonUtils {
Review Comment:
sorry I just realized that we have `IOUtil` in the codebase and some things can be re-used from there. We should remove `GCSCOmmonUtils` and use functionality from `IOUtil`.
For the `serialize()` / `deserialize()` methods that are only being used for testing, we can maybe add them to `TestHelpers`
--
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] Neuw84 commented on pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#issuecomment-1645292250
> @Neuw84 could you please rebase against latest master and squash all commits together?
Done @nastra!
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1254482980
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -662,15 +681,4 @@ private List<String> filterByExtension(String tablePath, String extension) {
.filter(f -> f.endsWith(extension))
.collect(Collectors.toList());
}
-
- private static String addRecordsToFile(Table table, String filename) throws IOException {
Review Comment:
will review the changes and repush, maybe it was a thing of IntelliJ on reformatting.
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257875998
##########
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:
I have replaced with native Java code
##########
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:
I have replaced with native Java 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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257903561
##########
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:
As it is test code that is not going to be shipped I just though that this was good enough. Open to suggestions here ( it is not very clear why the code is using `BoundedInputStream` and I did not wanted to mess with the specific test).
Open to suggestions/guidance 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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259908365
##########
delta-lake/src/integration/java/org/apache/iceberg/delta/TestSnapshotDeltaLakeTable.java:
##########
@@ -532,13 +533,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 = URLDecoder.decode(path, StandardCharsets.UTF_8.name());
if (dataFileUri.isAbsolute()) {
return decodedPath;
} else {
return tableRoot + File.separator + decodedPath;
}
- } catch (DecoderException e) {
+ } catch (UnsupportedEncodingException e) {
+ // In Java 10+ this will not need try-catch
Review Comment:
Yes, I suppose that the tooling will catch this when the project gets upgraded to a newer Java version.
Removed
##########
dell/src/test/java/org/apache/iceberg/dell/mock/ecs/ObjectData.java:
##########
@@ -70,9 +71,29 @@ public InputStream createInputStream(Range range) {
public S3ObjectMetadata createFullMetadata() {
S3ObjectMetadata metadata = new S3ObjectMetadata();
- metadata.setETag(DigestUtils.md5Hex(content));
+ MessageDigest md = null;
+ try {
+ md = MessageDigest.getInstance("MD5");
+ } catch (NoSuchAlgorithmException e) {
+ throw new RuntimeException(e);
+ }
+ md.update(content);
+ byte[] digest = md.digest();
+ metadata.setETag(bytesToHex(digest));
metadata.setContentLength((long) content.length);
metadata.setUserMetadata(userMetadata);
return metadata;
}
+
+ private static final char[] HEX_ARRAY = "0123456789ABCDEF".toCharArray();
Review Comment:
Moved!
--
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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1258442480
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,12 +66,13 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null) {
+ if (prefixes.trim().isEmpty()) {
Review Comment:
is this correct?
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259938414
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,12 +66,13 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null) {
+ if (!prefixes.trim().isEmpty()) {
Review Comment:
agreed, I have tested all the combinations just to be sure in a JShell 👍 .
```
import org.apache.commons.lang3.StringUtils;
String a=null;
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("nullllll check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("null check native");
}
a="";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("empty check ");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("empty check native");
}
a=" ";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("blank check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("blank check native");
}
a="bob";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("things check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("things check native");
}
a=" bob ";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("trim things check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("trim things check native");
}
```
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257875719
##########
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:
I replaced the code for native Java 8 code, then that shaded version is not going to be needed.
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257875719
##########
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:
I replaced the code for native Java 8 code (small change). Although the shaded version it is on
`object-client-bundle` library. The change is very small, but I could let it without changes. At least now, this code do not use the library directly.
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257875719
##########
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:
I replaced the code for native Java 8 code. Although the shaded version it is on
`object-client-bundle` library. The change is very small, but I could let it without changes. At least now, this code do not use the library directly.
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1254477750
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetAvro.java:
##########
@@ -41,6 +40,74 @@
class ParquetAvro {
+ static final GenericData DEFAULT_MODEL =
Review Comment:
Will re-push the changes. In this case, I added a private simple Pair class implementation
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1254481355
##########
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:
There was no usage of the library on this class!
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257875719
##########
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:
I replaced the code for native Java 8 code, then that shaded version is not going to be needed. Although it is on
`object-client-bundle` library. The change is very small, but I could let it without changes.
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257900833
##########
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:
The files are not exactly the same and I do not like ( but personal opinion here) the `test-jarscope` thing between projects.
Have renamed them to be more conformant with other classes within each project.
Open to suggestions/guidance 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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259914650
##########
gcp/src/test/java/org/apache/iceberg/gcp/gcs/GCSCommonUtils.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * 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.EOFException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+
+public class GCSCommonUtils {
Review Comment:
done!
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257895894
##########
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:
Have changed that code to native Java 8 compliant 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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257900833
##########
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:
The files are not exactly the same (just the part of `writes` and I do not like ( but personal opinion here) the `test-jarscope` thing between projects or adding test classes from some projects to others.
Have renamed them to be more conformant with other classes within each project.
Open to suggestions/guidance 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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257900833
##########
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:
The files are not exactly the same (just the part of `writes` and I do not like ( but personal opinion here) the `test-jarscope` thing between projects.
Have renamed them to be more conformant with other classes within each project.
Open to suggestions/guidance 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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257900833
##########
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:
The files are not exactly the same and I do not like ( but personal opinion here) the `test-jarscope` thing between projects.
Open to suggestions/guidance 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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259298548
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,12 +66,13 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null) {
+ if (prefixes.trim().isEmpty()) {
Review Comment:
Thanks for the catch. 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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259938414
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,12 +66,13 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null) {
+ if (!prefixes.trim().isEmpty()) {
Review Comment:
agreed, I have tested all the combinations just to be sure in a JShell 👍 .
`import org.apache.commons.lang3.StringUtils;
String a=null;
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("nullllll check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("null check native");
}
a="";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("empty check ");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("empty check native");
}
a=" ";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("blank check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("blank check native");
}
a="bob";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("things check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("things check native");
}
a=" bob ";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("trim things check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("trim things check native");
}`
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,12 +66,13 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null) {
+ if (!prefixes.trim().isEmpty()) {
Review Comment:
agreed, I have tested all the combinations just to be sure in a JShell 👍 .
`import org.apache.commons.lang3.StringUtils;
String a=null;
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("nullllll check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("null check native");
}
a="";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("empty check ");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("empty check native");
}
a=" ";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("blank check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("blank check native");
}
a="bob";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("things check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("things check native");
}
a=" bob ";
if (a != null && !StringUtils.isBlank(a)) {
System.out.println("trim things check");
}
if (a != null && !a.trim().isEmpty()) {
System.out.println("trim things check native");
}`
--
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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259864346
##########
gcp/src/test/java/org/apache/iceberg/gcp/gcs/GCSCommonUtils.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * 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.EOFException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+
+public class GCSCommonUtils {
Review Comment:
One file is called `CommonsS3...` whereas this one is called `GCSCommons...`. I'd suggest to put the `GCS` / `S3` part at the same position in the name
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1261383169
##########
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java:
##########
@@ -67,10 +66,11 @@ public static MapWork findMapWork(JobConf job) {
LOG.debug("Initializing for input {}", inputName);
}
String prefixes = job.get(DagUtils.TEZ_MERGE_WORK_FILE_PREFIXES);
- if (prefixes != null && !StringUtils.isBlank(prefixes)) {
- // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
- // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
- return null;
+ if (prefixes != null && !prefixes.trim().isEmpty()) {
+ // Currently SMB is broken, so we cannot check if it's compatible with IO elevator.
+ // So, we don't use the below code that would get the correct MapWork. See HIVE-16985.
+ return null;
+ }
Review Comment:
yep you are right, seems to be weird because yesterday I compiled everything.
--
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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1259860236
##########
dell/src/test/java/org/apache/iceberg/dell/mock/ecs/ObjectData.java:
##########
@@ -70,9 +71,29 @@ public InputStream createInputStream(Range range) {
public S3ObjectMetadata createFullMetadata() {
S3ObjectMetadata metadata = new S3ObjectMetadata();
- metadata.setETag(DigestUtils.md5Hex(content));
+ MessageDigest md = null;
+ try {
+ md = MessageDigest.getInstance("MD5");
+ } catch (NoSuchAlgorithmException e) {
+ throw new RuntimeException(e);
+ }
+ md.update(content);
+ byte[] digest = md.digest();
+ metadata.setETag(bytesToHex(digest));
metadata.setContentLength((long) content.length);
metadata.setUserMetadata(userMetadata);
return metadata;
}
+
+ private static final char[] HEX_ARRAY = "0123456789ABCDEF".toCharArray();
Review Comment:
`static final` fields should generally be placed at the top of the java 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] Neuw84 commented on pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#issuecomment-1632850423
Hi @nastra,
Have refactored the test methods based on the ones existing on `IOUtil` ones and have added `serialize() / deserialize()` methods to the `TestHelpers` class. Have applied everywhere `spotless` too.
--
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] nastra merged pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #7988:
URL: https://github.com/apache/iceberg/pull/7988
--
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] nastra commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "nastra (via GitHub)" <gi...@apache.org>.
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: 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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1257896777
##########
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:
Have changed that code to native Java 8 compliant coe
--
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] Neuw84 commented on a diff in pull request #7988: Remove the direct dependency on Apache Commons. Issue - #7658
Posted by "Neuw84 (via GitHub)" <gi...@apache.org>.
Neuw84 commented on code in PR #7988:
URL: https://github.com/apache/iceberg/pull/7988#discussion_r1254481355
##########
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:
I did not removed the libraries from Gradle for the subprojects ( Maven expert but not so much on Gradle), I have been searching but seems that the sub-projects were using the parent declared libraries when needed...
Will fix this line on next re-push and if you can point me to where I can delete the libraries for subprojects will be great!
--
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