[CALCITE-7393] Support RelDataTypeDigest #4761
Conversation
zhuwenzhuang
force-pushed
the
CALCITE-6907
branch
from
604cf22 to
04538b2
Compare
zhuwenzhuang
changed the title
|
Thank you for your contribution! I have a few suggestions: |
| /** | ||
| * Digest of a RelDataType. | ||
| */ | ||
| public interface RelDataTypeDigest { |
There was a problem hiding this comment.
This could be two interfaces, in case Digest is useful somewhere else.
There was a problem hiding this comment.
I noticed that the hashCode caching in RelDigest and RexCall already addresses some of the hashCode caching/comparison latency issues. Could you please clarify what you mean by “be two interfaces”?
There was a problem hiding this comment.
interface HasDigestString {
String getDigestString()
}
interface RelDataTypeDigest extends HasDigestString {
RelDataType getType();
}
Other classes may implement HasDigestString.
| } | ||
|
|
||
| @Override public boolean equals(@Nullable Object obj) { | ||
| @Override public boolean deepEquals(Object obj) { |
There was a problem hiding this comment.
I think you'll need to leave the old implementations around too, they may break users who expect them to exist.
There was a problem hiding this comment.
If we allow user to implement equals/hashCode for nested type, cache for hashCode and pointer compare for deepEquals will not work for them.
This situation is similiar to https://issues.apache.org/jira/browse/CALCITE-3786
|
|
||
| protected final @Nullable List<RelDataTypeField> fieldList; | ||
| protected @Nullable String digest; | ||
| protected RelDataTypeDigest digest; |
There was a problem hiding this comment.
Changing protected fields is a breaking change, which has to be reflected in the semantic versioning. This would require a 2.0 release.
There was a problem hiding this comment.
On the other hand, I don't know what else you can do to make the digest lazy. Maybe other people can weigh on this dilemma. This is why you should probably file a JIRA issue just for this topic and discuss the design there.
There was a problem hiding this comment.
Maybe we can allow user to set String digest to define their own digest. otherwise, we use the RelDataTypeDigest
There was a problem hiding this comment.
My suggestion is that some protected/public variables and methods must still be guaranteed to work properly. For example, this
"String digest" might eventually call
"toString" from
"RelDataTypeDigest" to get the value? Because these variables are exposed to users, and someone might currently be using them, we need to provide a compatibility fix. If you think reassigning the original variable would go against your design intention (e.g., it might increase memory usage instead of reducing it), then you need to declare that this is a breaking change.
There was a problem hiding this comment.
@xiedeyantu Thanks, it should be a breaking change. I commented in the commit message.
zhuwenzhuang
force-pushed
the
CALCITE-6907
branch
from
04538b2 to
a03c48a
Compare
zhuwenzhuang
changed the title
zhuwenzhuang
force-pushed
the
CALCITE-6907
branch
4 times, most recently
from
1f17a42 to
955f537
Compare
zhuwenzhuang
force-pushed
the
CALCITE-6907
branch
from
955f537 to
f33c6a2
Compare
zhuwenzhuang
force-pushed
the
CALCITE-6907
branch
from
592f23b to
ac7bf74
Compare
use RelDataTypeDigest to reduce composite type's digest memory and digest relative operation's latency This is a break change, computeDigest will not set the string digest field, and innerDigest will be initialized by default. We recommend users use innerDigest to replace legacy string digest to archive better memory/latency for nested types. For users rely on set String digest for UDT, we keep the old behavior of hashCode/equals.
zhuwenzhuang
force-pushed
the
CALCITE-6907
branch
from
ac7bf74 to
078f2e4
Compare
|
3 participants
Use RelDataTypeDigest to reduce composite type's digest memory and digest relative operation's latency.
test case: HepPlannerTest.testLargeTypeDigest
This is the first part of latency/memory optimization of large plan.