It would be great to unify this. Can you log a JIRA case?
The changes might be breaking. You can mitigate this by making the
reader more liberal (e.g. accept both upper-case and lower-case enum
names), and also by communicating what you intend to change. People
should speak up if they expect the format to be exactly the same
between versions.
It would be useful if you ensure that everything you intend to change
occurs in at least one test, before you make the change. Then the
before-and-after will be clearly visible in the patch.
Julian
On Wed, Jun 9, 2021 at 7:57 AM Konstantin Orlov wrote:
>
> Yes, it’s possible.
>
> But my concern is the lack of common approach. Here a few examples:
>
> 1) Correlate and Join both explains joinType as lowerName of enum’s value,
> hence it could be simply restored by relInput.getEnum("joinType",
> JoinRelType.class)
>
> @Override public RelWriter explainTerms(RelWriter pw) {
> return super.explainTerms(pw)
> .item("joinType", joinType.lowerName)
> ...
> }
>
> 2) MultiJoin uses similar approach, but instead of lowerName uses original
> enum’s name
>
> @Override public RelWriter explainTerms(RelWriter pw) {
> List joinTypeNames = new ArrayList<>();
> ...
> for (int i = 0; i < inputs.size(); i++) {
> joinTypeNames.add(joinTypes.get(i).name());
> ...
> }
>
> return pw.item("joinFilter", joinFilter)
> ...
> .item("joinTypes", joinTypeNames)
> ...
> }
>
> 3) TableModify uses RelEnumTypes.fromEnum(getOperation())) to explain
> operation type, but uses input.getEnum("operation", Operation.class) to
> restore it,
> whereas there is RelEnumTypes#toEnum(String) method
>
> 4) Spool explains both types as enum value, and probably will fail during
> serialisation to json cause
> org.apache.calcite.rel.externalize.RelJson#toJson(java.lang.Object)
> doesn’t know how to handle enum
>
> @Override public RelWriter explainTerms(RelWriter pw) {
> return super.explainTerms(pw)
> .item("readType", readType)
> .item("writeType", writeType);
> }
>
> Does it make any sense to unify this?
>
>
> --
> Regards,
> Konstantin Orlov
>
>
>
>
> > On 8 Jun 2021, at 22:03, Julian Hyde wrote:
> >
> > Including class names in serialized JSON makes me nervous. It is a common
> > attack surface. Could we, say, include the name of the enum (without
> > package name) and map it to a list of supported Enums?
> >
> >> On Jun 8, 2021, at 9:20 AM, Konstantin Orlov wrote:
> >>
> >> Hi, folks!
> >>
> >> We have a problem with Spool node deserialisation. Currently it explains
> >> both readType and writeType fields as Enum,
> >> thus those fields being serialized as map:
> >>
> >> {
> >>"id":"2",
> >>"relOp”:"MyTableSpool",
> >>"readType":{
> >> "class":"org.apache.calcite.rel.core.Spool$Type",
> >> "name":"LAZY"
> >>},
> >>"writeType":{
> >> "class":"org.apache.calcite.rel.core.Spool$Type",
> >> "name":"EAGER"
> >>}
> >> }
> >>
> >> When deserializing, we use RelInput#getEnum which expects the provided tag
> >> being a string value representing the enum's value name.
> >>
> >> Should we follow the way used for serialization of JoinRelType within the
> >> Join node (serialize the enum value as its name in lower case)? Here is
> >> example:
> >>
> >> {
> >>"id":"4",
> >>"relOp”:"MyJoin",
> >>"condition":{
> >> "op":{
> >> "name":">",
> >> "kind":"SqlKind#GREATER_THAN",
> >> "syntax":"SqlSyntax#BINARY"
> >> },
> >> "operands":[
> >> {
> >> "input":1,
> >> "name":"$1"
> >> },
> >> {
> >> "input":4,
> >> "name":"$4"
> >> }
> >> ]
> >>},
> >>"joinType":"inner",
> >>"variablesSet":[0],
> >>"correlationVariables":[0],
> >>"inputs":["0","3"]
> >> }
> >>
> >> Or it's better to get the RelInput being able to deserialize enum
> >> represented as map as well?
> >>
> >> Personally I prefer the first option cause the type of the enum is known
> >> for sure, so it's better not to waste the time to (de-)serialize it.
> >>
> >>
> >> --
> >> Regards,
> >> Konstantin Orlov
> >>
> >>
> >>
> >>
> >
>
