DM-14771: Add support for views to Schema #52
Conversation
|
@pschella, note that I also already started on the dataUnit.py parts of this on DM-14428. Looks like we went in almost the same direction, and you got farther than I did, so there's certainly no need to roll any of that back. But once you've got the rest of this working I'd be happy to take over that part - I know you're due to disappear at virtually any moment. |
|
|
||
| if isinstance(selectable, str): | ||
| selectable = select([text(selectable.lstrip("SELECT"))]) | ||
|
|
There was a problem hiding this comment.
I wonder if you can do something simple like
t = alias(selectable, name=name)
instead?
There was a problem hiding this comment.
I guess I was thinking about something different here. Ignore my comment if it confuses you 😕
| @@ -77,7 +77,7 @@ def __init__(self, registryConfig, schemaConfig): | |||
| self.config = SqlRegistryConfig(registryConfig) | |||
| self.storageClasses = StorageClassFactory() | |||
| self._schema = Schema(schemaConfig) | |||
| self._engine = create_engine(self.config['db']) | |||
| self._engine = create_engine(self.config['db'], echo=True) | |||
There was a problem hiding this comment.
I guess this is for debugging, it should not go into master?
| """When not ``None`` the primary table entry corresponding to this | ||
| `DataUnitJoin` (`sqlalchemy.core.TableClause`, optional). | ||
| """ | ||
| return getattr(self, '_table', None) |
There was a problem hiding this comment.
Why not just self._table?
pschella
force-pushed
the
tickets/DM-14771
branch
from
a5312a1
to
e9edc5c
Compare
config/schema.yaml
Outdated
| # - ExposureRange | ||
| # sql: | ||
| # (lhs.camera = rhs.camera) AND | ||
| # (lhs.exposure BETWEEN rhs.valid_first AND rhs.valid_last) |
There was a problem hiding this comment.
Instead of commenting this out, let's just leave this as it was, and ignore it when building the schema for now. I don't think we will ever want to make it a view; I think we'll instead want to find some way to inject this join expression into preflight queries.
I think since this "sql" entry is not inside a "tables" entry, this shouldn't be parsed by the DataUnitJoin code; if it is, we should fix that instead.
There was a problem hiding this comment.
Yes, it is skipped.
There was a problem hiding this comment.
I just commented it out such that the skipping is more explicit.
| rhs : `tuple` | ||
| Right-hand-side of the join. | ||
| summarizes : `DataUnit` | ||
| Summarizes this other `DataUnit`. |
There was a problem hiding this comment.
This is a DataUnitJoin, not a DataUnit, right?
| rhs=rhs, | ||
| summarizes=summarizes, | ||
| table=table) | ||
| self.joins[dataUnitJoinName] = dataUnitJoin |
There was a problem hiding this comment.
I don't think this needs to be changed here, but as an FYI, on DM-14428 I'll probably switch the keys to be (lhs, rhs), as I think that's what we'll want to use for lookup most of the time. Please shout if you think that'll be problematic.
There was a problem hiding this comment.
I think that is fine.
Based on MIT-licensed sqlalchemy work by Mike Bayer <mike_mp@zzzcomputing.com>.
pschella
force-pushed
the
tickets/DM-14771
branch
from
e9edc5c
to
de25a89
Compare
No description provided.