Address additional comments for PR 16416

[yingsu00]

#16416 is waiting to be merged. As per offline discussion, we create this issue to track the nits issue in this PR.

[yingsu00]

cc @simmend @nmahadevuni @rongrong

[yingsu00]

@nmahadevuni Naveen do you have time to work on this in the next a few days?

[yingsu00]

Thanks @harshjk for submitting PR17917 to resolve this issue. This message just tracks the unresolved comments in PR #17917

1. EquivalenceClassProperty.java, move line 231-232 to the caller
2. EquivalenceClassProperty.java, remove unnecessary toString() on Expressions in the toString() method
3. KeyProperty.java Remove line 113 requireNonNull(newKey, "newKey is null");
4. Replace the builder classes in LogicalPropertiesImpl to static methods
5. RemoveRedundantAggregateDistinct.java, simplify apply() by replacing the streaming API to for loop. The current implementation is hard to read. Also, @rongrong mentioned to check match once here

[simmend]

Thanks all!

…

On Fri, Jul 8, 2022 at 3:45 PM Ying ***@***.***> wrote: Thanks @harshjk <https://github.com/harshjk> for submitting PR17917 to resolve this issue. This message just tracks the unresolved comments in PR #17917 <#17917> 1. EquivalenceClassProperty.java, move line 231-232 to the caller 2. EquivalenceClassProperty.java, remove unnecessary toString() on Expressions in the toString() method 3. KeyProperty.java Remove line 113 requireNonNull(newKey, "newKey is null"); 4. Replace the builder classes in LogicalPropertiesImpl to static methods 5. RemoveRedundantAggregateDistinct.java, simplify apply() by replacing the streaming API to for loop. The current implementation is hard to read. Also, @rongrong <https://github.com/rongrong> mentioned to check match once here — Reply to this email directly, view it on GitHub <#17869 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEMBPVTH3Q7XQFPUUNC3NULVTCVP5ANCNFSM5YVTRQDA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rschlussel]

Commented on #17917, but one other thing to do is to refactor how the Property classes are constructed to make all the Property classes immutable.

[pratyakshsharma]

can we close this issue now that all the PRs are merged @yingsu00 ?