[SPARK-44018][SQL] Improve the hashCode and toString for some DS V2 Expression #41543

beliefer · 2023-06-10T08:07:17Z

What changes were proposed in this pull request?

The hashCode() of UserDefinedScalarFunc and GeneralScalarExpression is not good enough. Take for example, GeneralScalarExpression uses Objects.hash(name, children), it adopt the hash code of name and children's reference and then combine them together as the GeneralScalarExpression's hash code.
In fact, we should adopt the hash code for each element in children.

Because UserDefinedAggregateFunc and GeneralAggregateFunc missing hashCode(), this PR also want add them.

This PR also improve the toString for UserDefinedAggregateFunc and GeneralAggregateFunc by using bool primitive comparison instead Objects.equals. Because the performance of bool primitive comparison better than Objects.equals.

Why are the changes needed?

Improve the hash code for some DS V2 Expression.

Does this PR introduce any user-facing change?

'Yes'.

How was this patch tested?

N/A

beliefer · 2023-06-11T03:02:42Z

ping @cloud-fan cc @asiunov

asiunov

Thanks for fixing this! I'm not familiar well with how these classes work and are used, but I have a few suggestions based on how equals/hashCode are usually implemented.

asiunov · 2023-06-11T05:22:15Z

...src/main/java/org/apache/spark/sql/connector/expressions/aggregate/GeneralAggregateFunc.java

Suggested change

return Objects.hash(name) * 31 + Arrays.hashCode(children);

return Objects.hash(name, isDistinct) * 31 + Arrays.hashCode(children);

asiunov · 2023-06-11T05:23:52Z

...main/java/org/apache/spark/sql/connector/expressions/aggregate/UserDefinedAggregateFunc.java

Suggested change

return Objects.hash(name, canonicalName) * 31 + Arrays.hashCode(children);

return Objects.hash(name, canonicalName, isDistinct) * 31 + Arrays.hashCode(children);

asiunov · 2023-06-11T05:24:44Z

...main/java/org/apache/spark/sql/connector/expressions/aggregate/UserDefinedAggregateFunc.java

(added isDistinct == that.isDistinct)

Suggested change

return Objects.equals(name, that.name) && Objects.equals(canonicalName, that.canonicalName) &&

Arrays.equals(children, that.children);

return isDistinct == that.isDistinct && Objects.equals(name, that.name) &&

Objects.equals(canonicalName, that.canonicalName) && Arrays.equals(children, that.children);

asiunov · 2023-06-11T05:27:32Z

...src/main/java/org/apache/spark/sql/connector/expressions/aggregate/GeneralAggregateFunc.java

bool primitive comparison works faster than Objects.equals, also it is usually placed at the beginning, because it is faster to check than string/objects.

Suggested change

return Objects.equals(name, that.name) && Objects.equals(isDistinct, that.isDistinct) &&

return isDistinct == that.isDistinct && Objects.equals(name, that.name) &&

Yeah. bool primitive comparison faster than Objects.equals. But put bool primitive comparison at first is not good for readability.

pan3793 · 2023-06-12T06:25:17Z

A question for education: what's the benefit of a good hash in such cases?

beliefer · 2023-06-12T07:22:08Z

A question for education: what's the benefit of a good hash in such cases?

Personally, I think we shouldn't use references of any java object for hash code.

beliefer · 2023-06-12T08:29:51Z

ping @cloud-fan cc @huaxingao

asiunov · 2023-06-12T20:40:33Z

@pan3793, in this case, I built spark using bazel with a linter enabled, which errored on this hashcode implementation saying that Arrays.hashCode must be used.

sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/UserDefinedScalarFunc.java:61: error: [ArrayHashCode] hashcode method on array does not hash array contents
    return Objects.hash(name, canonicalName, children);
                       ^
    (see https://errorprone.info/bugpattern/ArrayHashCode)
  Did you mean 'return Objects.hash(name, canonicalName, Arrays.hashCode(children));'?

There is a conceptual issue here: hashCode and equals should be always designed in such a way that two non-equal objects may have equal hash codes (meaning a hash collision), but two equal objects must have equal hash codes. If the latter is wrong and such objects are used in a hashtable, it is possible that these two objects will be inserted in different buckets, meaning two equal objects will be present in this hashtable. For two arrays equals returns false and hashCode are different even if their elements are the same. This is because arrays' equals/hashCode are based on reference in memory and nothing else.

I do not remember all the details, but I remember there was a good explanation on designing hashCode/equals in Effective Java by Joshua Bloch.

pan3793 · 2023-06-13T01:48:21Z

... two equal objects must have equal hash codes ... If the latter is wrong and such objects are used in a hashtable ...

@asiunov thanks for the explanation, I know the principle, but I don't find where they are used as hash keys. That's why I put the in such cases in my question.

asiunov · 2023-06-13T03:27:07Z

Oh, I see. Sorry, I do not know the details of how these classes work. I reported this issue mostly because I've got linter errors, and just as a good-to-fix for any future use.

VindhyaG · 2023-06-19T05:26:09Z

Hi @beliefer One question. If hashCode function is added to UserDefinedAggregateFunc as public that means this is user facing change right?

beliefer · 2023-06-19T06:28:34Z

Hi @beliefer One question. If hashCode function is added to UserDefinedAggregateFunc as public that means this is user facing change right?

Yes. But if it's incorrect, we should fix it.

VindhyaG · 2023-06-19T06:36:15Z

Hi @beliefer One question. If hashCode function is added to UserDefinedAggregateFunc as public that means this is user facing change right?

Yes. But if it's incorrect, we should fix it.

It says No above in the "Does this PR introduce any user-facing change? " section . Since we are adding two new methods as user facing so wanted to confirm

VindhyaG · 2023-06-19T06:36:56Z

Hi @beliefer One question. If hashCode function is added to UserDefinedAggregateFunc as public that means this is user facing change right?

Yes. But if it's incorrect, we should fix it.

It says No above in the "Does this PR introduce any user-facing change? " section . Since we are adding two new methods as user facing so wanted to confirm (since that could implicate some changes in doc etc? just a doubt )

cloud-fan · 2023-06-19T07:54:34Z

... we are adding two new methods ...

It overrides two methods, so it's not an API change but rather a bug fix.

cloud-fan · 2023-06-19T07:54:42Z

thanks, merging to master/3.4!

…xpression ### What changes were proposed in this pull request? The `hashCode() `of `UserDefinedScalarFunc` and `GeneralScalarExpression` is not good enough. Take for example, `GeneralScalarExpression` uses `Objects.hash(name, children)`, it adopt the hash code of `name` and `children`'s reference and then combine them together as the `GeneralScalarExpression`'s hash code. In fact, we should adopt the hash code for each element in `children`. Because `UserDefinedAggregateFunc` and `GeneralAggregateFunc` missing `hashCode()`, this PR also want add them. This PR also improve the toString for `UserDefinedAggregateFunc` and `GeneralAggregateFunc` by using bool primitive comparison instead `Objects.equals`. Because the performance of bool primitive comparison better than `Objects.equals`. ### Why are the changes needed? Improve the hash code for some DS V2 Expression. ### Does this PR introduce _any_ user-facing change? 'Yes'. ### How was this patch tested? N/A Closes #41543 from beliefer/SPARK-44018. Authored-by: Jiaan Geng <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 8c84d2c) Signed-off-by: Wenchen Fan <[email protected]>

…xpression ### What changes were proposed in this pull request? The `hashCode() `of `UserDefinedScalarFunc` and `GeneralScalarExpression` is not good enough. Take for example, `GeneralScalarExpression` uses `Objects.hash(name, children)`, it adopt the hash code of `name` and `children`'s reference and then combine them together as the `GeneralScalarExpression`'s hash code. In fact, we should adopt the hash code for each element in `children`. Because `UserDefinedAggregateFunc` and `GeneralAggregateFunc` missing `hashCode()`, this PR also want add them. This PR also improve the toString for `UserDefinedAggregateFunc` and `GeneralAggregateFunc` by using bool primitive comparison instead `Objects.equals`. Because the performance of bool primitive comparison better than `Objects.equals`. ### Why are the changes needed? Improve the hash code for some DS V2 Expression. ### Does this PR introduce _any_ user-facing change? 'Yes'. ### How was this patch tested? N/A Closes apache#41543 from beliefer/SPARK-44018. Authored-by: Jiaan Geng <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

beliefer · 2023-06-19T12:18:20Z

@cloud-fan @asiunov @pan3793 @VindhyaG Thank you!

…xpression ### What changes were proposed in this pull request? The `hashCode() `of `UserDefinedScalarFunc` and `GeneralScalarExpression` is not good enough. Take for example, `GeneralScalarExpression` uses `Objects.hash(name, children)`, it adopt the hash code of `name` and `children`'s reference and then combine them together as the `GeneralScalarExpression`'s hash code. In fact, we should adopt the hash code for each element in `children`. Because `UserDefinedAggregateFunc` and `GeneralAggregateFunc` missing `hashCode()`, this PR also want add them. This PR also improve the toString for `UserDefinedAggregateFunc` and `GeneralAggregateFunc` by using bool primitive comparison instead `Objects.equals`. Because the performance of bool primitive comparison better than `Objects.equals`. ### Why are the changes needed? Improve the hash code for some DS V2 Expression. ### Does this PR introduce _any_ user-facing change? 'Yes'. ### How was this patch tested? N/A Closes apache#41543 from beliefer/SPARK-44018. Authored-by: Jiaan Geng <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 8c84d2c) Signed-off-by: Wenchen Fan <[email protected]>

github-actions bot added the SQL label Jun 10, 2023

beliefer changed the title ~~[SPARK-44018][SQL] Improve the hashCode for Some DS V2 Expression~~ [SPARK-44018][SQL] Improve the hash code for some DS V2 Expression Jun 10, 2023

asiunov reviewed Jun 11, 2023

View reviewed changes

beliefer changed the title ~~[SPARK-44018][SQL] Improve the hash code for some DS V2 Expression~~ [SPARK-44018][SQL] Improve the hashCode and toString for some DS V2 Expression Jun 12, 2023

asiunov approved these changes Jun 12, 2023

View reviewed changes

beliefer added 3 commits June 19, 2023 11:09

[SPARK-44018][SQL] Improve the hashCode for Some DS V2 Expression

293be4e

Update code

3b806f1

Update code

2158903

beliefer force-pushed the SPARK-44018 branch from 7b0c4b1 to 2158903 Compare June 19, 2023 03:30

cloud-fan closed this in 8c84d2c Jun 19, 2023

	return Objects.hash(name) * 31 + Arrays.hashCode(children);
	return Objects.hash(name, isDistinct) * 31 + Arrays.hashCode(children);

	return Objects.hash(name, canonicalName) * 31 + Arrays.hashCode(children);
	return Objects.hash(name, canonicalName, isDistinct) * 31 + Arrays.hashCode(children);

-    return Objects.equals(name, that.name) && Objects.equals(canonicalName, that.canonicalName) &&
-      Arrays.equals(children, that.children);
+    return isDistinct == that.isDistinct && Objects.equals(name, that.name) &&
+      Objects.equals(canonicalName, that.canonicalName) && Arrays.equals(children, that.children);

	return Objects.equals(name, that.name) && Objects.equals(isDistinct, that.isDistinct) &&
	return isDistinct == that.isDistinct && Objects.equals(name, that.name) &&

[SPARK-44018][SQL] Improve the hashCode and toString for some DS V2 Expression #41543

[SPARK-44018][SQL] Improve the hashCode and toString for some DS V2 Expression #41543

Uh oh!

Conversation

beliefer commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

beliefer commented Jun 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asiunov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asiunov Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

asiunov Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

asiunov Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

asiunov Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

beliefer Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

pan3793 commented Jun 12, 2023

Uh oh!

beliefer commented Jun 12, 2023

Uh oh!

beliefer commented Jun 12, 2023

Uh oh!

asiunov commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan3793 commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asiunov commented Jun 13, 2023

Uh oh!

VindhyaG commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beliefer commented Jun 19, 2023

Uh oh!

VindhyaG commented Jun 19, 2023

Uh oh!

VindhyaG commented Jun 19, 2023

Uh oh!

cloud-fan commented Jun 19, 2023

Uh oh!

cloud-fan commented Jun 19, 2023

Uh oh!

beliefer commented Jun 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

beliefer commented Jun 10, 2023 •

edited

Loading

beliefer commented Jun 11, 2023 •

edited

Loading

asiunov left a comment •

edited

Loading

asiunov commented Jun 12, 2023 •

edited

Loading

pan3793 commented Jun 13, 2023 •

edited

Loading

VindhyaG commented Jun 19, 2023 •

edited

Loading