DM-55424: Support scisql_s2PtInCircle Spatial Joins and scisql_angSep Area Restrictors#1032
Draft
malensek wants to merge 5 commits into
Draft
DM-55424: Support scisql_s2PtInCircle Spatial Joins and scisql_angSep Area Restrictors#1032malensek wants to merge 5 commits into
malensek wants to merge 5 commits into
Conversation
malensek
changed the title
scisql_s2PtInCircle-based Spatial Joins and scisql_angSep-based Area RestrictorsThere 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.
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.
malensek
force-pushed
the
tickets/DM-55424
branch
from
487f2e1 to
ecc1ec4
Compare
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.
malensek
force-pushed
the
tickets/DM-55424
branch
from
ecc1ec4 to
6631d16
Compare
Comment on lines
+333
to
+347
| case query::CompPredicate::EQUALS_OP: | ||
| case query::CompPredicate::NULL_SAFE_EQUALS_OP: | ||
| case query::CompPredicate::NOT_EQUALS_OP: | ||
| case query::CompPredicate::NOT_EQUALS_OP_ALT: | ||
| // While this doesn't make much sense numerically (floating | ||
| // point numbers are being tested for equality), it is | ||
| // technically evaluable. | ||
| fe = getAngSepFunc(cp.left); | ||
| if (!fe) { | ||
| angSep = cp.left->getNumericConst(); | ||
| fe = getAngSepFunc(cp.right); | ||
| } else { | ||
| angSep = cp.right->getNumericConst(); | ||
| } | ||
| break; |
Member
Author
There was a problem hiding this comment.
This was how it worked before this PR (just relocated), so I guess this definitely warrants some discussion...
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.