DM-55424: Support scisql_s2PtInCircle Spatial Joins and scisql_angSep Area Restrictors#1032
Draft
malensek wants to merge 6 commits into
Draft
DM-55424: Support scisql_s2PtInCircle Spatial Joins and scisql_angSep Area Restrictors#1032malensek wants to merge 6 commits into
malensek wants to merge 6 commits into
Conversation
malensek
changed the title
scisql_s2PtInCircle-based Spatial Joins and scisql_angSep-based Area Restrictors
There was a problem hiding this comment.
Pull request overview
This PR extends Qserv’s query analysis so that spatial joins and cone/area restrictors can be inferred from either scisql_angSep(...) OP radius or the equivalent scisql_s2PtInCircle(..., radius) = 1 form, improving interoperability between the two scisql idioms.
Changes:
- Add
ValueExpr::getNumericConst()to centralize numeric-constant extraction used by spatial inference. - Refactor
RelationGraph::_addSpEdgesto infer spatial joins fromscisql_angSepfirst, then fall back toscisql_s2PtInCircle(...)=1. - Extend
QservRestrictorPluginto recognizescisql_angSep(..., constLon, constLat) <(=) radiusas an area restrictor, and add/expand unit tests covering the new forms and error cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/query/ValueExpr.h | Declares new helper for extracting numeric constants from ValueExpr. |
| src/query/ValueExpr.cc | Implements getNumericConst() used by join/restrictor inference. |
| src/qana/RelationGraph.cc | Adds/refactors scisql predicate extraction to support pt-in-circle spatial joins. |
| src/qana/QservRestrictorPlugin.cc | Adds scisql_angSep-based cone restrictor recognition alongside existing s2PtIn* handling. |
| src/qproc/testQueryAnaGeneral.cc | Adds coverage for pt-in-circle joins, angSep cone restrictors, and negative-threshold error cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
malensek
force-pushed
the
tickets/DM-55424
branch
2 times, most recently
from
July 14, 2026 17:24
ecc1ec4 to
6631d16
Compare
malensek
force-pushed
the
tickets/DM-55424
branch
from
July 16, 2026 20:31
6631d16 to
53ff939
Compare
This functionality was duplicated between QservRestrictorPlugin and RelationGraph, so we'll centrally locate it here.
Previously scisql_angSep could not be used to express an area restriction even though a call to scisql_angSep with a single set of constant coordinate pairs can be rewritten as an equivalent scisql_s2PtInCircle area restrictor. This change allows appropriately-formed scisql_angSep calls to be automatically converted to a circular area restrictor.
This is already provided by the ValueExpr::getNumericConst() method.
Previously spatial joins were limited to scisql_angSep constructs. This allows queries to be expressed in equivalent scisql_s2PtInCircle form. Previous constraints remain (coordinate pairs must come from different table refs, but their columns must come from the same table ref, both column pairs must be found in director tables, and the directors must have the same partitioning). This includes a minor refactor to split checks for the two scisql functions into separate helper functions.
These tests exercise spatial joins via scisql_s2PtInCircle and area restrictors via scisql_angSep.
This file contains hidden or 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
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.
Currently, Qserv infers spatial join conditions only from appropriately formed
scisql_angSepexpressions. These conditions could, however, also validly be expressed in terms ofscisql_s2PtInCircle. Similarly, a query can express an area restriction (cone evaluated against a single director table) usingscisql_angSepinstead ofscisql_s2PtInCircle, as long as one side of the coordinate pair is a constant. For example:WHERE scisql_angSep(dias.coord_ra, dias.coord_dec, 55.5, -30.2) < 0.05is equivalent to
WHERE scisql_s2PtInCircle(dias.coord_ra, dias.coord_dec, 55.5, -30.2, 0.05) = 1which Qserv already recognizes as an area restrictor.
This PR adds the missing direction for both, so either scisql form can be used interchangeably for spatial joins and area restrictors.
Summary of Changes
_addSpEdgesnow triesscisql_angSepfirst, then falls back to the equivalentscisql_s2PtInCircle(...) = 1form. Refactored the per-form extraction logic. Improved error reporting in the case of malformed queries.extractSingleAreaRestrictornow also recognizesscisql_angSep(lon, lat, constLon, constLat) <= radiusas equivalent toscisql_s2PtInCircle(...) = 1, producing the same AreaRestrictorCircle.Behavior Changes
extractSingleScisqlAreaFuncacceptedscisql_s2PtInCircle(...) = <anything>as long as one side was ascisql_s2Pt*call. It did not check that the compared value was 1, soscisql_s2PtInCircle(...) = 0would have been accepted even though the opposite was intended. A check for == 1.0 was added.BoolFactorwith an associatedNOT, it was getting ignored and treated as the positive case. This is now caught in RelationGraph / QservRestrictorPlugin.!=and<>forangSepspatial joins. Such a join was accepted by the system previously, but would not have actually been evaluable because it references data outside chunk overlap.