DM-27985: Improvements to query system, especially temporal aspects #450
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TallJimbo
force-pushed
the
tickets/DM-27985
branch
from
7227604
to
c9733e4
Compare
andy-slac
reviewed
Dec 12, 2020
andy-slac
approved these changes
Dec 14, 2020
TallJimbo
force-pushed
the
tickets/DM-27985
branch
from
c9733e4
to
a12009f
Compare
|
Thanks for the detailed review, @andy-slac. I've pushed a few more commits and amended many more, and I think I've addressed all comments. If you're up for it, two of the new ones may be worth another look. The other new one, 15afa4a, is totally mechanical, so the most you'd want to look at is the commit message, I think. |
andy-slac
reviewed
Dec 16, 2020
Comment on lines
198
to
201
| if constant is ExpressionConstant.INGEST_DATE: | ||
| return TreeSummary(hasIngestDate=True) | ||
| elif constant is not None: | ||
| return TreeSummary() |
There was a problem hiding this comment.
Maybe try to make it a bit more stable w.r.t. potential extension of ExpressionConstant, like:
if constant is ExpressionConstant.INGEST_DATE:
...
elif constant is ExpressionConstant.NULL:
....
assert constant is None, "..."
andy-slac
reviewed
Dec 16, 2020
Comment on lines
+45
to
+46
| class TimeConverter(metaclass=Singleton): | ||
| """A singleton that provides methods for mapping TAI times to integer |
There was a problem hiding this comment.
A bit surprising to see singleton used to solve this problem, but probably fine. I guess I just don't like singletons in general 🙂.
|
I checked recent commits, they look fine, couple of minor comments. |
TallJimbo
force-pushed
the
tickets/DM-27985
branch
from
a12009f
to
82b8b87
Compare
This adds a new SpatialRegionDatabaseRepresentation class to encapsulate how we store regions in the database, and shuffles some code between TimespanDatabaseRepresentation and its (and the new class's) base class, TopologicalEndpointDatabaseRepresentation. Because we still only have one representation for spatial regions in the database (opaque byte strings with no database-side operators), this doesn't change functionality, but it cleans up centralizes what was previously a lot of scattered hard-coding of how we handle regions. It may also make it a bit easier to support database-native regions with real operators someday in the future, but given that don't know if we'll ever want that, that's at most a small side bonus.
In addition to adding __repr__ and one __str__ to the classes returned by .full and .records, this makes DataCoordate.__repr__ report only required dimensions, because that's all that's in keys(). That's good because the inherited values() implementation returns a ValuesView with a __repr__ that prints the __repr__ of the whole mapping, so before this change, the keys *appearing* there would differ both from what was actually in the ValuesView and what was in the keys (and the same was true of items()/ItemsView, I think).
This moves conversion to integer nanoseconds from astropy.time.Time to Timespan, instead of at the SQLAlchemy layer, which means we can finally make the edge-case handling (which has been improved - in that it's now consistent - in other ways) actually the same in Python and the DB. This also adds methods to construct TimespanDatabaseRepresentation instances from Timespan literals. That means we don't need to provide methods that relate TimespanDatabaseRepresentation to Timespan anymore. Code that starts with a mix of those types is now responsible for doing the conversion. On the other hand, because Timespan and TimespanDatabaseRepresentation can't exactly represent instants in time, we do need "overloads" for relatinship methods that take astropy.time.Time and sqlalchemy.sql.ColumnElement objects that represent times.
This one has support for timespan columns and bindparams, but is not used yet.
This was the old way of converting expression trees to SQL.
This commit is _just_ moving code around and changing import statements.
This defers all operations until first use (instead of import-time) and adds a bit of encapsulation.
...and similarly for regRepr->RegionReprClass. These symbols invariably refer to a type object that is a subclass of TimespanDatabaseRepresentation (or SpatialRegionDatabaseRepresentation).
TallJimbo
force-pushed
the
tickets/DM-27985
branch
from
82b8b87
to
75c3c1f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.