Skip to content

HIVE-28876 Strings should be compared with equals(), not with == #5744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

HIVE-28876 Strings should be compared with equals(), not with == #5744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constraint/ConstraintsUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ public static void validateCheckConstraint(List<FieldSchema> columns, List<SQLCh
throw new SemanticException(
ErrorMsg.INVALID_CSTR_SYNTAX.getMsg("Invalid type for CHECK constraint: ") + cc.getCheck_expression());
}
if (checkExpr.getTypeInfo().getTypeName() != serdeConstants.BOOLEAN_TYPE_NAME) {
if (!serdeConstants.BOOLEAN_TYPE_NAME.equals(checkExpr.getTypeInfo().getTypeName())) {
throw new SemanticException(
ErrorMsg.INVALID_CSTR_SYNTAX.getMsg("Only boolean type is supported for CHECK constraint: ") +
cc.getCheck_expression() + ". Found: " + checkExpr.getTypeInfo().getTypeName());
Expand Down
3 changes: 2 additions & 1 deletion ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Objects;

import static org.apache.hadoop.hive.conf.HiveConf.ConfVars.REPL_DUMP_METADATA_ONLY;
import static org.apache.hadoop.hive.conf.HiveConf.ConfVars.REPL_DUMP_SKIP_IMMUTABLE_DATA_COPY;
Expand Down Expand Up @@ -307,7 +308,7 @@ private boolean addTasksForPartition(Table table, AlterTableAddPartitionDesc add
+ partSpecToString(partSpec.getPartSpec()) + " with source location: "
+ partSpec.getLocation());
if (!lastProcessedStageFound && lastPartSpec != null &&
lastPartSpec.getLocation() != partSpec.getLocation()) {
!Objects.equals(lastPartSpec.getLocation(), partSpec.getLocation())) {
//Don't process copy task if already processed as part of previous run
continue;
}
Expand Down
4 changes: 2 additions & 2 deletions ql/src/java/org/apache/hadoop/hive/ql/exec/repl/util/ReplUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,8 +361,8 @@ public static boolean isCreateOperation(DDLOperation<?> ddlOperation) {
}

private static String getMetricStageName(String stageName, ReplicationMetricCollector metricCollector) {
if( stageName == "REPL_DUMP" || stageName == "REPL_LOAD" || stageName == "ATLAS_DUMP" || stageName == "ATLAS_LOAD"
|| stageName == "RANGER_DUMP" || stageName == "RANGER_LOAD" || stageName == "RANGER_DENY"){
if( "REPL_DUMP".equals(stageName) || "REPL_LOAD".equals(stageName) || "ATLAS_DUMP".equals(stageName) || "ATLAS_LOAD".equals(stageName)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have to much sentiment if the string is equals or ==. Both are acceptable. I would chime @deniskuzZ and @ayushtkn to tell which is the most commonly used style in Hive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Java the == operator compares only the references not the actual content of the objects so this change is needed here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old code also works because Java uses constant folding at compile time for optimization.
and this stageName comes from private final String STAGE_NAME = "REPL_LOAD";
but to make it consistent with non-final fields, .equals() is a better option.

You can check the first answer from this.

https://stackoverflow.com/questions/8581824/does-java-compiler-include-string-constant-folding

|| "RANGER_DUMP".equals(stageName) || "RANGER_LOAD".equals(stageName) || "RANGER_DENY".equals(stageName)){
return stageName;
}
if(isDumpMetricCollector(metricCollector)){
Expand Down
Loading