HIVE-27112 - implement array_except UDF in Hive #4090

tarak271 · 2023-03-01T06:30:09Z

What changes were proposed in this pull request?

Implement array_except function in Hive

Why are the changes needed?

This enhancement is already implemented in Spark

Does this PR introduce any user-facing change?

No

How was this patch tested?

Created Junit tests as well as qtests as part of this change

okumin · 2023-03-01T13:07:59Z

ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFArrayExcept.java

+ */
+@Description(name = "array_except", value = "_FUNC_(array, value) - Returns an array of the elements in array1 but not in array2.", extended =
+    "Example:\n" + "  > SELECT _FUNC_(array(1, 2, 3,4), array(2,3)) FROM src LIMIT 1;\n"
+        + "  [1,4]") @NDV(maxNdv = 2) public class GenericUDFArrayExcept extends AbstractGenericUDFArrayBase {


How about changing the definition from _FUNC_(array, value) to _FUNC_(array1, array2)?

Line break might be wrong

We have to remove @NDV(maxNdv = 2) because it means the number of distinct values is up to 2. This UDF can return any arrays, meaning the maximum number of distinct values is infinity

Made changes as recommended, formatting is done as per https://github.com/apache/hive/blob/master/dev-support/eclipse-styles.xml

okumin · 2023-03-01T13:19:16Z

ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFArrayExcept.java

+  @Override public ObjectInspector initialize(ObjectInspector[] arguments) throws UDFArgumentException {
+    ObjectInspector defaultOI = super.initialize(arguments);
+    checkArgCategory(arguments, ARRAY2_IDX, ObjectInspector.Category.LIST, FUNC_NAME,
+        org.apache.hadoop.hive.serde.serdeConstants.LIST_TYPE_NAME); //Array1 is already getting validated in Parent class


Should we check if the type of elements of array1 is equal to array2? It might be OK if we allow type conversions here.

Added test cases when one array is of int type and one array is of String type at ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFArrayExcept.java#testPrimitive

Thanks. I think we should carefully think of the expected specification first. What should the following SQL return?

SELECT array_except(array(1, 2, 3), array(2.0, 3.3));

If it should return array(1, 3), meaning type conversion is applied, we should add a test case where elements are removed with type conversions.
If it should return array(1, 2, 3), meaning the second argument is meaningless if types are unmatched, I personally think we should raise a syntax error. That's because it happens only when a user misunderstands the types of the 1st and 2nd arguments.

For example, Spark 3.4 fails in that case. PrestoSQL returns [1.0, 3.0] with the same SQL, meaning PrestoSQL applies the type conversion from int to float. I personally think either is fine, but it should be tested.

spark-sql (default)> select array_except(array(1, 2, 3), array(2.0, 3.3)); [DATATYPE_MISMATCH.BINARY_ARRAY_DIFF_TYPES] Cannot resolve "array_except(array(1, 2, 3), array(2.0, 3.3))" due to data type mismatch: Input to function `array_except` should have been two "ARRAY" with same element type, but it's ["ARRAY<INT>", "ARRAY<DECIMAL(2,1)>"].; line 1 pos 7; 'Project [unresolvedalias(array_except(array(1, 2, 3), array(2.0, 3.3)), None)] +- OneRowRelation

As recommended, throwing error for unmatched data types

okumin · 2023-03-01T13:30:12Z

ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFArrayExcept.java

+    }
+
+    List<?> retArray3 = ((ListObjectInspector) argumentOIs[ARRAY_IDX]).getList(array);
+    retArray3.removeAll(((ListObjectInspector) argumentOIs[ARRAY2_IDX]).getList(arguments[ARRAY2_IDX].get()));


I guess we shouldn't directly mutate the object here because ListObjectInspector can return the original value

I think it is possible to access the original object like SELECT array1, ARRAY_EXCEPT(array1, array2)

We want test cases to cover such usage

We should have to care the case where array2 is null because List#removeAll raises NPE

Added above test cases in ql/src/test/queries/clientpositive/udf_array_except.q

Thanks. As I commented on a test file, I feel SELECT array1, ARRAY_EXCEPT(array1, array2) returns an incorrect result.

Sorry that i missed your update regarding the incorrect result.. Now I have changed the code to avoid manipulating inputs

sonarcloud · 2023-03-02T10:13:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

github-actions · 2023-05-02T00:19:05Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.

saihemanth-cloudera · 2023-05-02T00:29:05Z

@tarak271 - Do you still plan to work on this PR?

tarak271 · 2023-05-03T10:07:58Z

Yes @saihemanth-cloudera, need some help with review. Coding part is complete

saihemanth-cloudera · 2023-05-08T23:00:32Z

Can you rerun the tests on this PR? I can't find a way to rerun the Build CI test suit. Can you push an empty commit? Thanks.

sonarcloud · 2023-05-10T08:47:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

tarak271 · 2023-05-10T13:04:11Z

@saihemanth-cloudera tests got completed now, please help with review

saihemanth-cloudera

LGTM +1

okumin

@tarak271 Looks cool! I only put a few minor comments.

okumin · 2023-06-10T01:48:13Z

ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFArrayExcept.java


  public GenericUDFArrayExcept() {
    super(FUNC_NAME, 2, 2, ObjectInspector.Category.LIST);
  }

  @Override public ObjectInspector initialize(ObjectInspector[] arguments) throws UDFArgumentException {
    ObjectInspector defaultOI = super.initialize(arguments);
+    array2OI = (ListObjectInspector) arguments[ARRAY2_IDX];


NITs:

This line should be after the following checkArgCategory because it validates the type and checkArgCategory throws a kind message

okumin · 2023-06-10T01:54:23Z

ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFArrayExcept.java

    checkArgCategory(arguments, ARRAY2_IDX, ObjectInspector.Category.LIST, FUNC_NAME,
        org.apache.hadoop.hive.serde.serdeConstants.LIST_TYPE_NAME); //Array1 is already getting validated in Parent class
+    arrayElementOI = arrayOI.getListElementObjectInspector();
+    array2ElementOI = array2OI.getListElementObjectInspector();


NIT: array2OI can be a local variable, or we can simply do ObjectInspectorUtils.compareTypes(arrayOI, arguments[ARRAY2_IDX]) because it seems to recursively check types of LIST, MAP, etc.

Removed all those variables

please note that elements in the input array can be of type array or list, that's why sending element itself to check compatibility

okumin · 2023-06-10T01:55:56Z

ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFArrayExcept.java

+    List inputArrayCopy = new ArrayList<>();
+    inputArrayCopy.addAll(retArray3);
+    inputArrayCopy.removeAll(((ListObjectInspector) argumentOIs[ARRAY2_IDX]).getList(arguments[ARRAY2_IDX].get()));
+    return inputArrayCopy.stream().distinct().map(o -> converter.convert(o)).collect(Collectors.toList());


We got robustness. Thanks!

okumin · 2023-06-10T01:58:20Z

ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFArrayExcept.java

+        runAndVerify(inputStringList,inputStringList,asList());
+        runAndVerify(inputStringList,asList(),inputStringList);
+        // Empty input arrays
+        runAndVerify(asList(),asList(),asList());


Great test cases!

sonarcloud · 2023-06-12T21:18:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
No Duplication information

okumin

Looks good to me. I want to ask @saihemanth-cloudera about the final check and merge since I am not a committer.
@tarak271 Nice work. I definitely have use cases to use this UDF!

saihemanth-cloudera · 2023-06-23T17:00:41Z

LGTM +1

… Rama Rao Lethavadla, reviewed by Okumin, Sai Hemanth Gantasala)

tarak271 added 2 commits February 28, 2023 19:51

HIVE-27112 - implement array_except UDF in Hive

a57805e

HIVE-27112 - implement array_except UDF in Hive

65e4e38

kgyrtkirk added the tests pending label Mar 1, 2023

tarak271 changed the title ~~Tarak hive 27112~~ HIVE-27112 - implement array_except UDF in Hive Mar 1, 2023

kgyrtkirk added tests passed and removed tests pending labels Mar 1, 2023

okumin reviewed Mar 1, 2023

View reviewed changes

HIVE-27112 - implement array_except UDF in Hive

e167901

kgyrtkirk added tests pending and removed tests passed labels Mar 2, 2023

tarak271 requested a review from okumin March 2, 2023 12:10

kgyrtkirk added tests passed and removed tests pending labels Mar 2, 2023

github-actions bot added the stale label May 2, 2023

github-actions bot removed the stale label May 3, 2023

tarak271 marked this pull request as draft May 10, 2023 07:04

tarak271 marked this pull request as ready for review May 10, 2023 07:09

HIVE-27112 - implement array_except UDF in Hive

75ebbf6

kgyrtkirk added tests pending and removed tests passed labels May 10, 2023

kgyrtkirk added tests passed and removed tests pending labels May 10, 2023

saihemanth-cloudera approved these changes May 11, 2023

View reviewed changes

HIVE-27112 - implement array_except UDF in Hive

e7a3444

kgyrtkirk added tests pending tests failed and removed tests passed tests pending labels Jun 9, 2023

Merge branch 'master' into tarak-HIVE-27112

930f9be

kgyrtkirk added tests pending tests unstable and removed tests failed tests pending labels Jun 9, 2023

okumin reviewed Jun 10, 2023

View reviewed changes

Update show_functions.q.out

f0b9d0e

kgyrtkirk added tests pending and removed tests unstable labels Jun 12, 2023

HIVE-27112 - implement array_except UDF in Hive

2464f3b

tarak271 requested a review from okumin June 12, 2023 11:25

kgyrtkirk added tests unstable tests pending and removed tests pending tests unstable labels Jun 12, 2023

kgyrtkirk added tests passed and removed tests pending labels Jun 13, 2023

okumin approved these changes Jun 13, 2023

View reviewed changes

saihemanth-cloudera approved these changes Jun 13, 2023

View reviewed changes

saihemanth-cloudera merged commit 14a1f70 into apache:master Jun 23, 2023
5 checks passed

tarak271 mentioned this pull request Jul 5, 2023

HIVE-27482 - Minor issues in array_intersect udf #4469

Merged

yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023

HIVE-27112 - implement array_except UDF in Hive (apache#4090) (Taraka…

b0b7d9a

… Rama Rao Lethavadla, reviewed by Okumin, Sai Hemanth Gantasala)

tarak271 added a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023

HIVE-27112 - implement array_except UDF in Hive (apache#4090) (Taraka…

93966ae

… Rama Rao Lethavadla, reviewed by Okumin, Sai Hemanth Gantasala)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-27112 - implement array_except UDF in Hive #4090

HIVE-27112 - implement array_except UDF in Hive #4090

tarak271 commented Mar 1, 2023

okumin Mar 1, 2023

tarak271 Mar 2, 2023

okumin Mar 1, 2023

tarak271 Mar 2, 2023

okumin May 20, 2023 •

edited

tarak271 Jun 9, 2023

okumin Mar 1, 2023

tarak271 Mar 2, 2023

okumin May 20, 2023

tarak271 Jun 9, 2023

sonarcloud bot commented Mar 2, 2023

github-actions bot commented May 2, 2023

saihemanth-cloudera commented May 2, 2023

tarak271 commented May 3, 2023

saihemanth-cloudera commented May 8, 2023

sonarcloud bot commented May 10, 2023

tarak271 commented May 10, 2023

saihemanth-cloudera left a comment

okumin left a comment

okumin Jun 10, 2023

tarak271 Jun 12, 2023

okumin Jun 10, 2023

tarak271 Jun 12, 2023

tarak271 Jun 12, 2023

okumin Jun 10, 2023

okumin Jun 10, 2023

sonarcloud bot commented Jun 12, 2023

okumin left a comment

saihemanth-cloudera commented Jun 23, 2023

HIVE-27112 - implement array_except UDF in Hive #4090

HIVE-27112 - implement array_except UDF in Hive #4090

Conversation

tarak271 commented Mar 1, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okumin May 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 2, 2023

github-actions bot commented May 2, 2023

saihemanth-cloudera commented May 2, 2023

tarak271 commented May 3, 2023

saihemanth-cloudera commented May 8, 2023

sonarcloud bot commented May 10, 2023

tarak271 commented May 10, 2023

saihemanth-cloudera left a comment

Choose a reason for hiding this comment

okumin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 12, 2023

okumin left a comment

Choose a reason for hiding this comment

saihemanth-cloudera commented Jun 23, 2023

okumin May 20, 2023 •

edited