DM-31725: stand up initial version of package #1

TallJimbo · 2022-06-24T20:38:08Z

No description provided.

python/lsst/daf/relation/_relation.py

gpdf · 2022-08-20T23:00:36Z

Is this something being factored out of daf_butler, or ctrl_mpexec? (I happened to see some automated mail from GitHub about it.)

TallJimbo · 2022-08-21T01:41:42Z

It's new code that would otherwise probably go into daf_butler: it's a rewrite of much of the query expression handling.

codecov-commenter · 2022-09-24T03:25:39Z

Codecov Report

❗ No coverage uploaded for pull request base (main@c433d42). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #1   +/-   ##
=======================================
  Coverage        ?   88.57%           
=======================================
  Files           ?        1           
  Lines           ?       35           
  Branches        ?        2           
=======================================
  Hits            ?       31           
  Misses          ?        3           
  Partials        ?        1

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

timj

I've not looked at every file in details but the package is well organized, it has some nice documentation, and follows all our conventions. I'm very pleased to see 90% test coverage.

timj · 2022-11-30T23:14:39Z

doc/changes/README.rst

+* ``perf``: A performance enhancement.
+* ``doc``: A documentation improvement.
+* ``removal``: An API removal or deprecation.
+* ``other``: Other Changes and Additions of interest to general users.


We dropped "other" at some point because no-one could remember the difference between "other" and "misc" but I see I haven't updated the documentation in all the packages to reflect that (specifically daf_butler still talks about other).

timj · 2022-11-30T23:15:57Z

doc/lsst.daf.relation/sql.rst

+
+The SQL engine has a non-trivial `Engine.conform` method and overrides other methods to keep relation trees in a certain order when applying new operations.
+It uses a custom `Select` marker type to partition the tree into subqueries and mark a tree as conformed.
+This order attempts to avoid subqueries whenever possible by pulling `Projection`, `Deduplication`, `Sort`, and `Slice` operations downstream when commutation rules permit, both to keep the SQL output simple and to reduce the loss of row-ordering imposed by `Sort` operations whenever possible (i.e. a `Sort` can only be applied in the outermost query if it is to have any effect).


These classes don't turn into links in the sphinx docs.

Thanks, need to add that pesky leading . to them. (By the way, since I only discovered this behavior while working on this ticket, I'm sure we've got a ton of preexisting bad links in daf_butler that need it as well.)

timj · 2022-11-30T23:16:29Z

doc/lsst.daf.relation/sql.rst

+
+The `Engine` and `Payload` classes are generic over a parameter we refer to as the "logical column type".
+In the simplest case, the logical column type is just `sqlalchemy.sql.ColumnElement`, and this is what the default implementations of most `Engine` methods assume.
+Custom subclasses of the SQL `Engine` can use other types, such as wrappers that hold one or more `sqlalchemy.sql.ColumnElement` objects, as long as they override a few `Engine` methods to handle them.


Do we have to include some sqlalchemy index in the sphinx configuration for these links to work?

Good point, and I think I was meaning to ask @jonathansick about it. I don't see anything about intersphinx in either the package-local conf.py or the one in https://github.com/lsst/pipelines_lsst_io, so I assume this is something we import from documenteer in those files.

I also haven't been able to find the right URL to associate with the sqlalchemy package in the intersphinx mapping, which may mean there isn't one - their docs sure look like they've been built with sphinx, but I don't think that means they necessarily publish the interface that intersphinx needs.

I'll add sqlalchemy to the documenteer configuration for pipelines packages. It does use Sphinx for the documentation sites of individual versions. DM-37194

Here's background on the Sphinx configuration for Rubin to clarify how conf.py files work and this documentation page specifically on the Sphinx configuration settings (via a page in the Developer Guide).

codecov · 2022-12-02T15:10:05Z

Codecov Report

❗ No coverage uploaded for pull request base (main@c433d42). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main       #1   +/-   ##
=======================================
  Coverage        ?   97.10%           
=======================================
  Files           ?       15           
  Lines           ?     1242           
  Branches        ?      117           
=======================================
  Hits            ?     1206           
  Misses          ?       19           
  Partials        ?       17

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

This includes a large number of related changes: - Markers (including Transfer and Materialization) are now Relation types, not operation types. - UnaryOperation.apply is now final, with a number of new hooks for customization by both derived operations and engines, with backtracking dedicated to Engines. - The SQL engine overrides these hooks to partition and reorder its trees into subqueries indicated by the new Select marker. This lets it preserve row-order from sorts much better. It does not attempt to support backtracking at all. - The iteration engine does support backtracking, and since it always preserves order, we can drop all ordering checks (and the preserves_order method itself) in the backtracking algorithm. - Relations now use immutable concrete containers (frozenset, tuple) instead of their collections.abc generalizations to ensure hashability (though this is just in type annotations - users still have to actually pass in hashable containers at runtime). And, finally, there's now a pretty complete battery of unit tests, with missing coverage limited to: - Simple exception-raising checks for logic bugs in calling code that just didn't seem worth that much attention. - Defensive code that is at least very difficult to trigger, if not impossible, without violating other rules (e.g. getting a not-already-conformed Relation tree in the SQL engine to call its conform method on).

andy-slac · 2022-12-12T18:04:05Z

doc/lsst.daf.relation/index.rst

+   always returns the operand relation passed to it.
+`Join`  (`BinaryOperation`) / `Relation.join`
+   Perform a natural join: combine two relations by matching rows with the same values in their common columns (and satisfying an optional column expression, via a `Predicate`), producing a new relation whose columns are the union of the columns of its operands.
+   This is equivalent to [``INNER``] ``JOIN`` in SQL.


Don't we also need outer joins, I think I saw at least one case in daf_butler?

TallJimbo commented Jun 24, 2022

View reviewed changes

python/lsst/daf/relation/_relation.py Outdated Show resolved Hide resolved

TallJimbo force-pushed the tickets/DM-31725 branch from 8434c98 to 762b5b9 Compare July 4, 2022 03:16

TallJimbo force-pushed the tickets/DM-31725 branch from e8bc082 to a97dbc1 Compare July 22, 2022 02:50

TallJimbo force-pushed the tickets/DM-31725 branch from 108b966 to e3bc06a Compare September 27, 2022 20:05

TallJimbo force-pushed the tickets/DM-31725 branch 2 times, most recently from 8f379e6 to 31a832b Compare October 11, 2022 18:24

TallJimbo force-pushed the tickets/DM-31725 branch 5 times, most recently from a2531bd to fdbcf8e Compare November 3, 2022 19:42

TallJimbo force-pushed the tickets/DM-31725 branch from 310cf1a to e909b0a Compare November 29, 2022 21:23

TallJimbo marked this pull request as ready for review November 29, 2022 21:26

TallJimbo self-assigned this Nov 29, 2022

TallJimbo requested a review from timj November 29, 2022 21:39

timj approved these changes Dec 1, 2022

View reviewed changes

TallJimbo force-pushed the tickets/DM-31725 branch from e909b0a to 24fe7d5 Compare December 2, 2022 15:05

TallJimbo added 5 commits December 2, 2022 13:47

Update package metadata and build files to latest middleware versions.

4b61572

Complete prototype for RFC.

47b0b6f

Fix typing and logic bugs in column objects.

49ff359

Drop 'other' category from changlog readme.

069bf3d

TallJimbo force-pushed the tickets/DM-31725 branch from 8729132 to 069bf3d Compare December 2, 2022 18:48

TallJimbo added 2 commits December 2, 2022 13:48

Add pypi section to build.yaml.

acf2679

Fix bug in Projection.commute.

1d5d9be

TallJimbo merged commit 619ccee into main Dec 6, 2022

TallJimbo deleted the tickets/DM-31725 branch December 6, 2022 18:15

andy-slac reviewed Dec 12, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-31725: stand up initial version of package #1

DM-31725: stand up initial version of package #1

TallJimbo commented Jun 24, 2022

gpdf commented Aug 20, 2022

TallJimbo commented Aug 21, 2022

codecov-commenter commented Sep 24, 2022 •

edited

timj left a comment

timj Nov 30, 2022

timj Nov 30, 2022

TallJimbo Dec 2, 2022

timj Nov 30, 2022

TallJimbo Dec 2, 2022 •

edited

jonathansick Dec 2, 2022

codecov bot commented Dec 2, 2022 •

edited

andy-slac Dec 12, 2022

DM-31725: stand up initial version of package #1

DM-31725: stand up initial version of package #1

Conversation

TallJimbo commented Jun 24, 2022

gpdf commented Aug 20, 2022

TallJimbo commented Aug 21, 2022

codecov-commenter commented Sep 24, 2022 • edited

Codecov Report

timj left a comment

Choose a reason for hiding this comment

timj Nov 30, 2022

Choose a reason for hiding this comment

timj Nov 30, 2022

Choose a reason for hiding this comment

TallJimbo Dec 2, 2022

Choose a reason for hiding this comment

timj Nov 30, 2022

Choose a reason for hiding this comment

TallJimbo Dec 2, 2022 • edited

Choose a reason for hiding this comment

jonathansick Dec 2, 2022

Choose a reason for hiding this comment

codecov bot commented Dec 2, 2022 • edited

Codecov Report

andy-slac Dec 12, 2022

Choose a reason for hiding this comment

codecov-commenter commented Sep 24, 2022 •

edited

TallJimbo Dec 2, 2022 •

edited

codecov bot commented Dec 2, 2022 •

edited