minyyy commented on PR #36121: URL: https://github.com/apache/spark/pull/36121#issuecomment-1095230817
> Is there a perf number ? Not yet, but I can create a benchmark for it. > FYI, It's for semantics why the `ExpressionSet` inherit Iterable which make sure we have the same ordering with different runs, some history #29598 I actually saw this PR. But from what I understood #29598. It did 2 things. The first is that it fixed the insertion stability issue of `AttributeSet`. The second is a refactor of `ExpressionSet` which does not relate to insertion order. Per the comment in the PR, the reason for the refactor is that the default implementation of some set operations returns `Set[Expression]` instead of `ExpressionSet`. However, changing the base class from `Set` to `Iterable` is not the correct refactor IMO. Per https://www.scala-lang.org/api/2.12.3/scala/collection/Set.html > Implementation note: If your additions and mutations return the same kind of set as the set you are defining, you should inherit from SetLike as well. What we should do is to also extend `SetLike` instead of changing `Set` to `Iterable`. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
