Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-23588

Default null fields in Avro schemas must have "null" first in union datatypes

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None

      Description

      Many of the fields in our Avro alert schemas are default null. However, the 2.0 schemas list "null" as the second allowed data type in the union, which is not allowed by the Avro spec:

      (Note that when a default value is specified for a record field whose type is a union, the type of the default value must match the first element of the union. Thus, for unions containing "null", the "null" is usually listed first, since the default value of such unions is typically null.)

      This ticket is to update the schemas. This is a minor version bump, because any (hypothetical) alerts that specified all fields with no null values would still be readable with the union data type ordering changed.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            DM-22606 is the ticket I was thinking of. Even has the same quote from the Avro spec!

            Show
            swinbank John Swinbank added a comment - DM-22606 is the ticket I was thinking of. Even has the same quote from the Avro spec!
            Hide
            ebellm Eric Bellm added a comment - - edited

            I'm going to propose making this a major version bump and change the namespacing as well. As discussed for ZTF alerts here the current namespacing approach we're using causes conflicts with some of the standard Java tools. Essentially, having lsst.alert and lsst.alert.foo leads to complications in the generated Java code.

            After the update diaSource, diaForcedSource, cutout, etc. will all live in the top-level lsst namespace.

            Since we are not really using the namespacing (and the current schemas drop named types in both lsst.* and lsst.alert.* without much rhyme or reason) I don't think this will create any major end-user complications.

            Show
            ebellm Eric Bellm added a comment - - edited I'm going to propose making this a major version bump and change the namespacing as well. As discussed for ZTF alerts here the current namespacing approach we're using causes conflicts with some of the standard Java tools. Essentially, having lsst.alert and lsst.alert.foo leads to complications in the generated Java code. After the update diaSource, diaForcedSource, cutout, etc. will all live in the top-level lsst namespace. Since we are not really using the namespacing (and the current schemas drop named types in both lsst.* and lsst.alert.* without much rhyme or reason) I don't think this will create any major end-user complications.
            Hide
            ebellm Eric Bellm added a comment -

            John Swinbank, can you take a look? I ended up not changing the namespacing, which I'll ticket separately, but I did make some additional fields (proper motions, covariances) nullable that were previously required.

            Show
            ebellm Eric Bellm added a comment - John Swinbank , can you take a look? I ended up not changing the namespacing, which I'll ticket separately, but I did make some additional fields (proper motions, covariances) nullable that were previously required.
            Hide
            swinbank John Swinbank added a comment -

            Looks great!

            On https://github.com/lsst-dm/sample-avro-alert/pull/10 I added some tests to try parsing example data with the schemas where provided. I think it's a useful companion to this work. Up to you if you want to included here, or if you'd rather see it on another ticket (or if you think it's unnecessary, of course).

            Show
            swinbank John Swinbank added a comment - Looks great! On https://github.com/lsst-dm/sample-avro-alert/pull/10 I added some tests to try parsing example data with the schemas where provided. I think it's a useful companion to this work. Up to you if you want to included here, or if you'd rather see it on another ticket (or if you think it's unnecessary, of course).

              People

              • Assignee:
                ebellm Eric Bellm
                Reporter:
                ebellm Eric Bellm
                Reviewers:
                John Swinbank
                Watchers:
                Eric Bellm, John Swinbank
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel