Uploaded image for project: 'Avro'
  1. Avro
  2. AVRO-2019

Improve documentation for logical type annotations in IDL

    Details

    • Type: Improvement
    • Status: Patch Available
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: doc, logical types
    • Labels:
      None

      Description

      The IDL documentation lacks information for how annotations can be specified for logical types, like in the following example:

      protocol test {
          record test {
              @logicalType("timestamp-millis")
              long time;
          }
      }
      
      1. AVRO-2019.patch
        0.9 kB
        Andrew Rosca

        Issue Links

          Activity

          Hide
          arosca Andrew Rosca added a comment -

          minor addition to IDL logical type documentation

          Show
          arosca Andrew Rosca added a comment - minor addition to IDL logical type documentation
          Hide
          nkollar Nandor Kollar added a comment -

          Indeed, looks like this is missing from the documentation. Is this covered with tests too? I was looking for a relevant test case, but couldn't find any. Andrew Rosca do you know any relevant test case for this feature?

          Show
          nkollar Nandor Kollar added a comment - Indeed, looks like this is missing from the documentation. Is this covered with tests too? I was looking for a relevant test case, but couldn't find any. Andrew Rosca do you know any relevant test case for this feature?
          Hide
          howellbridger Bridger Howell added a comment -

          Isn't the way logicalType decorators work on types in IDL just a natural consequence of how IDL treats any decorator on a type as a JSON property for that type, combined with the meaning of that field in the spec?

          I think this intent is expressed clearly enough in the IDL documentation "Annotation Ordering and Namespaces" (perhaps this section title is a bit too specific):

          Java-style annotations may be used to add additional properties to types and fields throughout Avro IDL.

          (Some details on specific properties/annotations)

          Some annotations like those listed above are handled specially. All other annotations are added as properties to the protocol, message, schema or field.

          I don't think I found a great generic/shared test of this, but there are at least a few spots where the the set of tests ran by org.apache.avro.compiler.idl.TestIdl show off this kind of behavior.

          For the IDL input simple.avdl, I see a case where the type of a field declared like:
          @foo("bar") MD5 hash = "0000000000000000";
          has the property "foo": "bar".

          Show
          howellbridger Bridger Howell added a comment - Isn't the way logicalType decorators work on types in IDL just a natural consequence of how IDL treats any decorator on a type as a JSON property for that type, combined with the meaning of that field in the spec? I think this intent is expressed clearly enough in the IDL documentation "Annotation Ordering and Namespaces" (perhaps this section title is a bit too specific): Java-style annotations may be used to add additional properties to types and fields throughout Avro IDL. (Some details on specific properties/annotations) Some annotations like those listed above are handled specially. All other annotations are added as properties to the protocol, message, schema or field. I don't think I found a great generic/shared test of this, but there are at least a few spots where the the set of tests ran by org.apache.avro.compiler.idl.TestIdl show off this kind of behavior. For the IDL input simple.avdl , I see a case where the type of a field declared like: @foo("bar") MD5 hash = "0000000000000000"; has the property "foo": "bar" .
          Hide
          busbey Sean Busbey added a comment -

          Given the docs on logical types elsewhere, I think it's worth calling out this particular use of annotations in IDL. Folks who rely on IDL to use Avro shouldn't have to know the implementation details of how logical types are implemented via properties so that they can reason out how to use them in IDL.

          Show
          busbey Sean Busbey added a comment - Given the docs on logical types elsewhere, I think it's worth calling out this particular use of annotations in IDL. Folks who rely on IDL to use Avro shouldn't have to know the implementation details of how logical types are implemented via properties so that they can reason out how to use them in IDL.
          Hide
          busbey Sean Busbey added a comment -

          it looks like the timestamp related logical types are only in 1.8+. Folks have any preference between getting them added back into the 1.7 line vs updating this doc to use decimal (which is the only logical type defined in 1.7 AFAICT)

          Show
          busbey Sean Busbey added a comment - it looks like the timestamp related logical types are only in 1.8+. Folks have any preference between getting them added back into the 1.7 line vs updating this doc to use decimal (which is the only logical type defined in 1.7 AFAICT)
          Hide
          howellbridger Bridger Howell added a comment -

          Clarification on my previous comment: I do think this documentation update is a good idea; I was just trying to say that IDL logical types don't necessarily need to be treated internally as a feature separate from the generic metadata facility of IDL annotations.

          In order to break @logicalType annotations in IDL you'd probably either be changing the avro spec for logical types, or changing the way that annotations in IDL are processed, either of which is a breaking change already.
          On the other hand, if you try to retain the way @logicalType maps to a logical type separately from the way annotations are mapped, that could in many cases make IDL annotations harder to understand by complicating the mechanism.

          Show
          howellbridger Bridger Howell added a comment - Clarification on my previous comment: I do think this documentation update is a good idea; I was just trying to say that IDL logical types don't necessarily need to be treated internally as a feature separate from the generic metadata facility of IDL annotations. In order to break @logicalType annotations in IDL you'd probably either be changing the avro spec for logical types, or changing the way that annotations in IDL are processed, either of which is a breaking change already. On the other hand, if you try to retain the way @logicalType maps to a logical type separately from the way annotations are mapped, that could in many cases make IDL annotations harder to understand by complicating the mechanism.

            People

            • Assignee:
              arosca Andrew Rosca
              Reporter:
              arosca Andrew Rosca
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development