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

multi-level replacement in Schema aliases

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • afw

    Description

      Schema aliases should support more than one level (i.e. an alias may resolve to another alias).

      Attachments

        Issue Links

          Activity

            No builds found.
            jbosch Jim Bosch created issue -
            jbosch Jim Bosch made changes -
            Field Original Value New Value
            Epic Link DM-1099 [ 13906 ]
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Sprint Science Pipelines DM-S14-1 [ 96 ]
            jbosch Jim Bosch made changes -
            Labels AppsPlanning SciencePipelines
            jbosch Jim Bosch made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            jbosch Jim Bosch added a comment -

            Daniel, up for a small review in afw?

            This adds recursive behavior to the afw::table alias system: instead of just replacing name lookups that begin with registered aliases with the alias target once, we do it repeatedly until we don't find another match. Should be straightforward and well-localized, so I don't think it should be a problem that there's a lot of unfamiliar code nearby.

            All changes are on afw branch u/jbosch/DM-1282:

            afw:u/jbosch/DM-1282 % git diff --stat master...u/jbosch/DM-1282
             python/lsst/afw/table/Base.i |    8 ++++----
             src/table/AliasMap.cc        |   37 +++++++++++++++++++++++++------------
             src/table/Schema.cc          |   10 +++++-----
             tests/testTableAliases.py    |   17 +++++++++++++++++
             4 files changed, 51 insertions(+), 21 deletions(-)

            jbosch Jim Bosch added a comment - Daniel, up for a small review in afw? This adds recursive behavior to the afw::table alias system: instead of just replacing name lookups that begin with registered aliases with the alias target once, we do it repeatedly until we don't find another match. Should be straightforward and well-localized, so I don't think it should be a problem that there's a lot of unfamiliar code nearby. All changes are on afw branch u/jbosch/ DM-1282 : afw:u/jbosch/DM-1282 % git diff --stat master...u/jbosch/DM-1282 python/lsst/afw/table/Base.i | 8 ++++---- src/table/AliasMap.cc | 37 +++++++++++++++++++++++++------------ src/table/Schema.cc | 10 +++++----- tests/testTableAliases.py | 17 +++++++++++++++++ 4 files changed, 51 insertions(+), 21 deletions(-)
            jbosch Jim Bosch made changes -
            Reviewers Daniel Wang [ danielw ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            jbosch Jim Bosch made changes -
            Link This issue blocks DM-1332 [ DM-1332 ]
            danielw Daniel Wang [X] (Inactive) made changes -
            Status In Review [ 10004 ] Review Complete [ 10101 ]

            Jim,
            I looked at the code, and it seems sensible, but I was wondering if we care about the efficiency of detecting cycles. Are AliasMaps going to be big? I get the impression that cycles are supposed to be an indication of a (hopefully) rare bug state, so it shouldn't matter that we walk through the cycle up to N hops to report a cycle in an N-sized map. Is this true? If so, then we don't care too much about efficiency, but we might put a comment saying that yes, we could get by with less hops if we're willing to do more bookkeeping, but it's not worth it in this case.

            Otherwise, great job, looks pretty clean.

            danielw Daniel Wang [X] (Inactive) added a comment - Jim, I looked at the code, and it seems sensible, but I was wondering if we care about the efficiency of detecting cycles. Are AliasMaps going to be big? I get the impression that cycles are supposed to be an indication of a (hopefully) rare bug state, so it shouldn't matter that we walk through the cycle up to N hops to report a cycle in an N-sized map. Is this true? If so, then we don't care too much about efficiency, but we might put a comment saying that yes, we could get by with less hops if we're willing to do more bookkeeping, but it's not worth it in this case. Otherwise, great job, looks pretty clean.
            jbosch Jim Bosch added a comment -

            Exactly: it's a rare bug state, and I just didn't want it to result in an endless loop. I'll add the comment.

            jbosch Jim Bosch added a comment - Exactly: it's a rare bug state, and I just didn't want it to result in an endless loop. I'll add the comment.
            jbosch Jim Bosch made changes -
            Resolution Done [ 10000 ]
            Status Review Complete [ 10101 ] Done [ 10002 ]

            People

              jbosch Jim Bosch
              jbosch Jim Bosch
              Daniel Wang [X] (Inactive)
              Daniel Wang [X] (Inactive), Jim Bosch
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.