[
https://issues.apache.org/jira/browse/HIVE-24883?focusedWorklogId=582371&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-582371
]
ASF GitHub Bot logged work on HIVE-24883:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 14/Apr/21 10:44
Start Date: 14/Apr/21 10:44
Worklog Time Spent: 10m
Work Description: zabetak commented on a change in pull request #2071:
URL: https://github.com/apache/hive/pull/2071#discussion_r613092276
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/exec/HiveWritableComparator.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+
+public class HiveWritableComparator extends WritableComparator {
+ private WritableComparator comparator = null;
+ protected transient boolean nullSafe;
+ transient NullOrdering nullOrdering;
+ protected transient int not_null = 2;
Review comment:
`comparator` and `not_null` may be dropped after suggested changes below.
`nullSafe` and `nullOrdering` can become `private final` I think.
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/exec/HiveWritableComparator.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+
+public class HiveWritableComparator extends WritableComparator {
+ private WritableComparator comparator = null;
+ protected transient boolean nullSafe;
+ transient NullOrdering nullOrdering;
+ protected transient int not_null = 2;
+
+ HiveWritableComparator(boolean nullSafe, NullOrdering nullOrdering) {
+ this.nullSafe = nullSafe;
+ this.nullOrdering = nullOrdering;
+ }
+
+ protected int checkNull(Object key1, Object key2) {
Review comment:
Is this method meant to be overriden by subclasses?
The method performs some comparisons between keys so maybe we could change
this method to:
`public final int compare(Object key1, Object key2)`
See also other comments in this class.
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/exec/HiveWritableComparator.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+
+public class HiveWritableComparator extends WritableComparator {
+ private WritableComparator comparator = null;
+ protected transient boolean nullSafe;
+ transient NullOrdering nullOrdering;
+ protected transient int not_null = 2;
+
+ HiveWritableComparator(boolean nullSafe, NullOrdering nullOrdering) {
+ this.nullSafe = nullSafe;
+ this.nullOrdering = nullOrdering;
+ }
+
+ protected int checkNull(Object key1, Object key2) {
+ if (key1 == null && key2 == null) {
+ if (nullSafe) {
+ return 0;
+ } else {
+ return -1;
+ }
+ } else if (key1 == null) {
+ return nullOrdering == null ? -1 :
nullOrdering.getNullValueOption().getCmpReturnValue();
+ } else if (key2 == null) {
+ return nullOrdering == null ? 1 :
-nullOrdering.getNullValueOption().getCmpReturnValue();
+ } else {
+ return not_null;
Review comment:
instead of returning and keeping a flag we could call directly the
method handling the not null comparison.
`return compareNotNulls(key1, key2);`
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveStructComparator.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.ArrayList;
+
+class HiveStructComparator extends HiveWritableComparator {
Review comment:
Consider limiting extensibility (change to `final`)
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveUnionComparator.java
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import
org.apache.hadoop.hive.serde2.objectinspector.StandardUnionObjectInspector.StandardUnion;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+
+public class HiveUnionComparator extends HiveWritableComparator {
+ WritableComparator comparator = null;
+
+ HiveUnionComparator(boolean nullSafe, NullOrdering nullOrdering) {
+ super(nullSafe, nullOrdering);
+ }
+
+ @Override
+ public int compare(Object key1, Object key2) {
+ int result = checkNull(key1, key2);
+ if (result != not_null) {
+ return result;
+ }
+
+ StandardUnion u1 = (StandardUnion) key1;
+ StandardUnion u2 = (StandardUnion) key2;
+
+ if (u1.getTag() != u2.getTag()) {
+ // If tag is not same, the keys may be of different data types. So
can not be compared.
+ return u1.getTag() > u2.getTag() ? 1 : -1;
Review comment:
Comment is not clear cause we are comparing them in the end but the
comparison is based entirely on the tag. Are we testing this case?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveMapComparator.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.LinkedHashMap;
+
+public class HiveMapComparator extends HiveWritableComparator {
+ WritableComparator comparatorValue = null;
+ WritableComparator comparatorKey = null;
+
+ HiveMapComparator(boolean nullSafe, NullOrdering nullOrdering) {
+ super(nullSafe, nullOrdering);
+ }
+
+ @Override
+ public int compare(Object key1, Object key2) {
+ int result = checkNull(key1, key2);
+ if (result != not_null) {
+ return result;
+ }
+
+ LinkedHashMap map1 = (LinkedHashMap) key1;
+ LinkedHashMap map2 = (LinkedHashMap) key2;
Review comment:
Why do we need to cast to `LinkedHashMap` ? Can't we simply cast to
`Map`?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveUnionComparator.java
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import
org.apache.hadoop.hive.serde2.objectinspector.StandardUnionObjectInspector.StandardUnion;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+
+public class HiveUnionComparator extends HiveWritableComparator {
+ WritableComparator comparator = null;
Review comment:
Should this be set outside of this class? Change to `private`?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveMapComparator.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.LinkedHashMap;
+
+public class HiveMapComparator extends HiveWritableComparator {
+ WritableComparator comparatorValue = null;
+ WritableComparator comparatorKey = null;
+
+ HiveMapComparator(boolean nullSafe, NullOrdering nullOrdering) {
+ super(nullSafe, nullOrdering);
+ }
+
+ @Override
+ public int compare(Object key1, Object key2) {
+ int result = checkNull(key1, key2);
+ if (result != not_null) {
+ return result;
+ }
+
+ LinkedHashMap map1 = (LinkedHashMap) key1;
+ LinkedHashMap map2 = (LinkedHashMap) key2;
+ if (map1.entrySet().size() != map2.entrySet().size()) {
+ return map1.entrySet().size() > map2.entrySet().size() ? 1 : -1;
+ }
+ if (map1.entrySet().size() == 0) {
+ return 0;
+ }
+
+ if (comparatorKey == null) {
+ comparatorKey =
+
WritableComparatorFactory.get(map1.keySet().iterator().next(), nullSafe,
nullOrdering);
+ comparatorValue =
+
WritableComparatorFactory.get(map1.values().iterator().next(), nullSafe,
nullOrdering);
+ }
+
+ result = comparatorKey.compare(map1.keySet().iterator().next(),
+ map2.keySet().iterator().next());
+ if (result != 0) {
+ return result;
+ }
+ return comparatorValue.compare(map1.values().iterator().next(),
map2.values().iterator().next());
Review comment:
Is it normal that we are comparing only the first key and the first
value of the map?
Do we have tests with more than one key-value pairs?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveListComparator.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.ArrayList;
+
+public class HiveListComparator extends HiveWritableComparator {
+ // For List, all elements will have same type, so only one comparator is
sufficient.
+ WritableComparator comparator = null;
+
+ HiveListComparator(boolean nullSafe, NullOrdering nullOrdering) {
+ super(nullSafe, nullOrdering);
+ }
+
+ @Override
+ public int compare(Object key1, Object key2) {
+ int result = checkNull(key1, key2);
+ if (result != not_null) {
+ return result;
+ }
+ ArrayList a1 = (ArrayList) key1;
+ ArrayList a2 = (ArrayList) key2;
Review comment:
Do we need to cast to `ArrayList` and not simply to `List`?
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/exec/WritableComparatorFactory.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import
org.apache.hadoop.hive.serde2.objectinspector.StandardUnionObjectInspector.StandardUnion;
+import org.apache.hadoop.hive.serde2.typeinfo.TypeInfo;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+
+public final class WritableComparatorFactory {
+ public static WritableComparator get(TypeInfo typeInfo, boolean nullSafe,
NullOrdering nullOrdering) {
+ switch (typeInfo.getCategory()) {
+ case PRIMITIVE:
+ return new HiveWritableComparator(nullSafe, nullOrdering);
+ case LIST:
+ return new HiveListComparator(nullSafe, nullOrdering);
+ case MAP:
+ return new HiveMapComparator(nullSafe, nullOrdering);
+ case STRUCT:
+ return new HiveStructComparator(nullSafe, nullOrdering);
+ case UNION:
+ return new HiveUnionComparator(nullSafe, nullOrdering);
+ default:
+ throw new IllegalStateException("Unexpected value: " +
typeInfo.getCategory());
+ }
+ }
+
+ public static WritableComparator get(Object key, boolean nullSafe,
NullOrdering nullOrdering) {
+ if (key instanceof ArrayList) {
+ // For array type struct is used as we do not know if all elements
of array are of same type.
+ return new HiveStructComparator(nullSafe, nullOrdering);
+ } else if (key instanceof LinkedHashMap) {
Review comment:
Should we make the checks more generic? For instance: `List` instead of
`ArrayList`, `Map` instead of `LinkedHashMap`?
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/exec/HiveWritableComparator.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+
+public class HiveWritableComparator extends WritableComparator {
Review comment:
Why do we need to `extends WritableComparator`? Do we gain something? If
we don't I would consider removing it since it makes usages of this class prone
to errors and extensions more difficult to follow.
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/exec/HiveWritableComparator.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+
+public class HiveWritableComparator extends WritableComparator {
+ private WritableComparator comparator = null;
+ protected transient boolean nullSafe;
+ transient NullOrdering nullOrdering;
+ protected transient int not_null = 2;
+
+ HiveWritableComparator(boolean nullSafe, NullOrdering nullOrdering) {
+ this.nullSafe = nullSafe;
+ this.nullOrdering = nullOrdering;
+ }
+
+ protected int checkNull(Object key1, Object key2) {
+ if (key1 == null && key2 == null) {
+ if (nullSafe) {
+ return 0;
+ } else {
+ return -1;
+ }
+ } else if (key1 == null) {
+ return nullOrdering == null ? -1 :
nullOrdering.getNullValueOption().getCmpReturnValue();
+ } else if (key2 == null) {
+ return nullOrdering == null ? 1 :
-nullOrdering.getNullValueOption().getCmpReturnValue();
+ } else {
+ return not_null;
+ }
+ }
+
+ public int compare(Object key1, Object key2) {
+ int result = checkNull(key1, key2);
+ if (result != not_null) {
+ return result;
+ }
+
+ if (comparator == null) {
+ comparator = WritableComparator.get(((WritableComparable)
key1).getClass());
+ }
+ return comparator.compare((WritableComparable)key1,
(WritableComparable)key2);
+ }
+}
Review comment:
From my understanding so far this method is meant to be used only by
primitive type comparisons.
Maybe the intention could be more clear if we renamed and declare this as
abstract:
`protected abstract int compareNotNulls(Object key1, Object key2);`
and then create another subclass exclusively for primitive types.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveStructComparator.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.ArrayList;
+
+class HiveStructComparator extends HiveWritableComparator {
+ WritableComparator[] comparator = null;
Review comment:
Change to `private`?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveUnionComparator.java
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import
org.apache.hadoop.hive.serde2.objectinspector.StandardUnionObjectInspector.StandardUnion;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+
+public class HiveUnionComparator extends HiveWritableComparator {
Review comment:
Consider reducing visibility (`public` to package) and extensibility
(make `final`).
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveMapComparator.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.LinkedHashMap;
+
+public class HiveMapComparator extends HiveWritableComparator {
+ WritableComparator comparatorValue = null;
+ WritableComparator comparatorKey = null;
+
+ HiveMapComparator(boolean nullSafe, NullOrdering nullOrdering) {
+ super(nullSafe, nullOrdering);
+ }
+
+ @Override
+ public int compare(Object key1, Object key2) {
+ int result = checkNull(key1, key2);
+ if (result != not_null) {
+ return result;
+ }
+
+ LinkedHashMap map1 = (LinkedHashMap) key1;
+ LinkedHashMap map2 = (LinkedHashMap) key2;
+ if (map1.entrySet().size() != map2.entrySet().size()) {
+ return map1.entrySet().size() > map2.entrySet().size() ? 1 : -1;
+ }
Review comment:
Isn't `map.entrySet().size()` equivalent to `map.size()`? Why do we use
the former?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveMapComparator.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.LinkedHashMap;
+
+public class HiveMapComparator extends HiveWritableComparator {
+ WritableComparator comparatorValue = null;
+ WritableComparator comparatorKey = null;
Review comment:
Consider reducing visibility and extensibility. See comments in other
classes.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveListComparator.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.ArrayList;
+
+public class HiveListComparator extends HiveWritableComparator {
+ // For List, all elements will have same type, so only one comparator is
sufficient.
+ WritableComparator comparator = null;
+
+ HiveListComparator(boolean nullSafe, NullOrdering nullOrdering) {
+ super(nullSafe, nullOrdering);
+ }
+
+ @Override
+ public int compare(Object key1, Object key2) {
+ int result = checkNull(key1, key2);
+ if (result != not_null) {
+ return result;
+ }
+ ArrayList a1 = (ArrayList) key1;
+ ArrayList a2 = (ArrayList) key2;
+ if (a1.size() != a2.size()) {
+ return a1.size() > a2.size() ? 1 : -1;
+ }
+ if (a1.size() == 0) {
+ return 0;
+ }
+
+ if (comparator == null) {
+ // For List, all elements should be of same type.
+ comparator = WritableComparatorFactory.get(a1.get(0), nullSafe,
nullOrdering);
Review comment:
What if the first element of a `List` is `null`? `ArrayList` doesn't
allow this but other implems do.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/HiveListComparator.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.ArrayList;
+
+public class HiveListComparator extends HiveWritableComparator {
+ // For List, all elements will have same type, so only one comparator is
sufficient.
+ WritableComparator comparator = null;
Review comment:
Consider reducing visibility accessibility (see comments in other
classes).
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/exec/WritableComparatorFactory.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.hadoop.hive.ql.exec;
+
+import org.apache.hadoop.hive.ql.util.NullOrdering;
+import
org.apache.hadoop.hive.serde2.objectinspector.StandardUnionObjectInspector.StandardUnion;
+import org.apache.hadoop.hive.serde2.typeinfo.TypeInfo;
+import org.apache.hadoop.io.WritableComparable;
+import org.apache.hadoop.io.WritableComparator;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+
+public final class WritableComparatorFactory {
+ public static WritableComparator get(TypeInfo typeInfo, boolean nullSafe,
NullOrdering nullOrdering) {
+ switch (typeInfo.getCategory()) {
+ case PRIMITIVE:
+ return new HiveWritableComparator(nullSafe, nullOrdering);
+ case LIST:
+ return new HiveListComparator(nullSafe, nullOrdering);
+ case MAP:
+ return new HiveMapComparator(nullSafe, nullOrdering);
+ case STRUCT:
+ return new HiveStructComparator(nullSafe, nullOrdering);
+ case UNION:
+ return new HiveUnionComparator(nullSafe, nullOrdering);
+ default:
+ throw new IllegalStateException("Unexpected value: " +
typeInfo.getCategory());
+ }
+ }
+
+ public static WritableComparator get(Object key, boolean nullSafe,
NullOrdering nullOrdering) {
Review comment:
When `key == null` it will create the default `HiveWritableComparator`.
Is this intended?
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 582371)
Time Spent: 1h 40m (was: 1.5h)
> Add support for complex types columns in Hive Joins
> ---------------------------------------------------
>
> Key: HIVE-24883
> URL: https://issues.apache.org/jira/browse/HIVE-24883
> Project: Hive
> Issue Type: Bug
> Components: Hive
> Affects Versions: 4.0.0
> Reporter: mahesh kumar behera
> Assignee: mahesh kumar behera
> Priority: Major
> Labels: pull-request-available
> Fix For: 4.0.0
>
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> Hive fails to execute joins on array type columns as the comparison functions
> are not able to handle array type columns.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
