kgyrtkirk commented on a change in pull request #1161:
URL: https://github.com/apache/hive/pull/1161#discussion_r450299299
##
File path: common/src/java/org/apache/hadoop/hive/conf/Validator.java
##
@@ -357,14 +357,15 @@ public String validate(String value) {
final Path path = FileSystems.getDefault().getPath(value);
if (path == null && value != null) {
return String.format("Path '%s' provided could not be located.",
value);
- }
- final boolean isDir = Files.isDirectory(path);
- final boolean isWritable = Files.isWritable(path);
- if (!isDir) {
-return String.format("Path '%s' provided is not a directory.", value);
- }
- if (!isWritable) {
-return String.format("Path '%s' provided is not writable.", value);
+ } else if (path != null) {
+final boolean isDir = Files.isDirectory(path);
+final boolean isWritable = Files.isWritable(path);
+if (!isDir) {
+ return String.format("Path '%s' provided is not a directory.",
value);
+}
+if (!isWritable) {
+ return String.format("Path '%s' provided is not writable.", value);
+}
}
return null;
Review comment:
I think this should be something else than `null` - did the old
behaviour accept `null` as a valid path? I believe `Files.isDirectory(null)`
returned false
looking from the other end: I don't think `null` should be accepted as a
"writeabledirectory"
##
File path: common/src/java/org/apache/hadoop/hive/common/FileUtils.java
##
@@ -926,8 +925,7 @@ public static File createLocalDirsTempFile(Configuration
conf, String prefix, St
* delete a temporary file and remove it from delete-on-exit hook.
*/
public static boolean deleteTmpFile(File tempFile) {
-if (tempFile != null) {
- tempFile.delete();
+if (tempFile != null && tempFile.delete()) {
Review comment:
this change will leave the file registered in the shutdownmanager in
case the file was already deleted
##
File path: common/src/java/org/apache/hadoop/hive/common/type/HiveVarchar.java
##
@@ -62,6 +62,9 @@ public boolean equals(Object rhs) {
if (rhs == this) {
return true;
}
-return this.getValue().equals(((HiveVarchar)rhs).getValue());
+if (rhs instanceof HiveVarchar) {
+ return this.getValue().equals(((HiveVarchar) rhs).getValue());
Review comment:
nice catch! :D
##
File path: common/src/java/org/apache/hive/common/util/SuppressFBWarnings.java
##
@@ -0,0 +1,37 @@
+/*
+ * 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.hive.common.util;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.CLASS)
+public @interface SuppressFBWarnings {
+/**
+ * The set of FindBugs warnings that are to be suppressed in
+ * annotated element. The value can be a bug category, kind or pattern.
+ *
+ */
+String[] value() default {};
+
+/**
+ * Optional documentation of the reason why the warning is suppressed
+ */
+String justification() default "";
+}
Review comment:
I think at some point we should probably introduce some rule to ensure
that every file ends with a newline...but for now: could you add one here ? :D
##
File path: common/src/java/org/apache/hadoop/hive/common/StringInternUtils.java
##
@@ -135,10 +135,10 @@ public static Path internUriStringsInPath(Path path) {
public static Map internValuesInMap(Map map) {
if (map != null) {
- for (K key : map.keySet()) {
-String value = map.get(key);
+ for (Map.Entry entry : map.entrySet()) {
+String value = entry.getValue();
if (value != null) {
- map.put(key, value.intern());
+ map.put(entry.getKey(), value.intern());
Review comment:
I know it's probably out of scope in this patch - but in case the value
is already "interned" - then it will not change; at a cost of a conditional we
can skip the hash lookup(put) as well
This is an