handle struct-based UDT literals in core

[benbellick]

Support both opaque (google.protobuf.Any) and
structured (Literal.Struct) encodings for user-defined type literals per Substrait spec.

• Split UserDefinedLiteral into UserDefinedAny and UserDefinedStruct
• Move type parameters to interface level for parameterized types
• Add first-class POJO representation for type parameters
• Test coverage including roundtrip tests
• Throw exception on unhandled struct-based representation in isthmus

Closes #611

[benbellick]

FYI, this PR makes no attempt to actually validate if the struct representation provided matches the definition in the yaml file. I think that this is the right thing to do, but it turned out to be slightly more complicated than I thought, as it involves threading an ExtensionCollection through the codebase a bit. Thus, I left it for another issue (#614).

[benbellick]

This exception doesn't happen anymore because we don't parse the Any here. Instead, we have a reference to the pre-parsed proto directly.

[benbellick]

This is consistent with the above, where we don't yet support ParamerizedType conversion.

[benbellick]

This is also consistent with the above.

[benbellick]

This is #612

[vbarua]

Started taking a look at this and left some comments. Mostly looks reasonable. Want to come back and look at your tests with fresh eyes, and also think about parameterized types with a fresh 🧠.

[vbarua]

Is there a reason to use a List<Literal> here instead of just a Expression.StructLiteral, which would map directly to the protobuf?

[benbellick]

Sharing this comment because I believe it is relevant.

---

Basically it is the same issue, which is that the POJO class called StructLiteral is representing a special case of the Literal class.

  message Literal { // Both StructLiteral and UserDefinedStructLiteral are representing this _whole_ message
    oneof literal_type {
      ...
      Struct struct = 25;
      ...
      UserDefined user_defined = 33;
    }
    ...
    message Struct {
      // A possibly heterogeneously typed list of literals
      repeated Literal fields = 1;
    }

    message UserDefined {
      oneof type_anchor_type {
        // points to a type_anchor defined in this plan
        uint32 type_reference = 1;

        // points to a type_alias_anchor defined in this plan.
        uint32 type_alias_reference = 5;
      }

      // The parameters to be bound to the type class, if the type class is
      // parameterizable.
      repeated Type.Parameter type_parameters = 3;

      // a user-defined literal can be encoded in one of two ways
      oneof val {
        // the value of the literal, serialized using some type-specific protobuf message
        google.protobuf.Any value = 2;
        // the value of the literal, serialized using the structure definition in its declaration
        Literal.Struct struct = 4;
      }
    }
  }

Back to your comment, switching this member variable to be Expression.StructLiteral would amount to embedding one literal proto inside of another. For example, the proto Struct doesn't actually have a nullable field. But the POJO Expression.StructLiteral does have a nullable field because it inherits from Literal.

This doesn't mean that we couldn't replace the member variable as you suggest, but if we did do that, we would be carrying around extra meaningless data, which I think is more confusing ultimately.

[vbarua]

The docstring feels a little inconsistent. You have

literal with value encoded as {@code Literal.Struct}

but in the class the values are encoded as a List<Literal>, and the second part of the docstring is consistent with that.

[benbellick]

I guess that this could be made less confusing. The intention was to mean the proto Literal.Struct, no some java class. What do you think about just altering the message to say

User-defined literal with value encoded via the proto message {@code Literal.Struct}.

?

[benbellick]

I altered it to instead link to the actual proto. Let me know what you think! I also did the same for the any case.

[vbarua]

Release Notes
Capturing the value as an Any instead of a ByteString does feel nicer ✨

[vbarua]

Release Notes
We should call out that we don't construct UserDefinedLiterals anymore.

[vbarua]

Is the function of https://github.com/substrait-io/substrait-java/blob/main/core/src/main/java/io/substrait/function/ParameterizedType.java applicable here? It feels vaguely related.

[benbellick]

🤔 yes that is an interesting point. Looking into it!

[benbellick]

So as I understand it, ParameterizedType.java is used for representing abstract types with parameters in yaml files. Where as the Parameter above being introduced is actually a concrete argument passed into the type.

So for example, List<any1> could be a ParameterizedType, whereas List<int32> is a type with parameters [int32].

[vbarua]

Is there a usecase for having boolean literal type parameters? I can't imagine a usecase where something like MySpecialType<false> is something you would need.

[benbellick]

#613 (comment)

🤷 just wanted to be consistent with the spec

[vbarua]

What would this type be used for?

[benbellick]

As discussed offline, these were inspired by this portion of the simple extension schema:

  type_param_defs: # an array of compound type parameter definitions
    type: array
    items:
      ...
      properties:
        ...
        type: # expected metatype for the parameter
          type: string
          enum:
            - dataType
            - boolean
            - integer
            - enumeration
            - string

So while I don't understand the usage of it, I thought it was best to include all of them for consistency with the spec.

[vbarua]

What would this type be used for?

[benbellick]

#613 (comment)

[benbellick]

FYI, I have a WIP PR for implementing this in Isthmus but I split it into two PRs because the code there is more complicated. This keeps the PRs a bit smaller!