DM-17720: Improve user expression handling in preflight #129
Conversation
| expression : `exprParser.Node` | ||
| Parsed user expression tree. | ||
| dimensions : `DimensionGraph` | ||
| All Dimensions included in this query. |
There was a problem hiding this comment.
How hard would it be to make this all dimensions known to the registry (which is now its own class, DimensionUniverse, by the way), and then expand the query to include dimensions referenced in the expression but not the dataset types?
I could imagine it being useful to be able to run single-epoch processing on all visits that overlap a tract, for example.
There was a problem hiding this comment.
It should be possible in principle but it's probably non very trivial. Make a new ticket if you want that extra feature 🙂
| Exception | ||
| Any exception raised by SQLAlchemy is propagated to caller. | ||
| """ | ||
| if isinstance(expression, (exprParser.StringLiteral, exprParser.NumericLiteral)): |
There was a problem hiding this comment.
Would it be possible to transform this conditional block into delegation to a polymorphic method on the exprParser classes that returns a ClauseElement?
All of these isinstance checks make me think of a classic OO antipattern, but I could also interpret this as the kind of match on algebraic sum types that would be quite natural in a language like Rust or OCaml that supported them, and in that case I suppose I don't have an objection to trying to fake that in Python.
There was a problem hiding this comment.
I thought about adding that facility to exprParser tree classes but decided that I want to keep that code free of extra dependencies like sqlalchemy - if we want to support few additional tree transformations it would not be right design to do all of that inside tree itself. You are right that this is a common problem and some languages have better support for implementing this sort of double-dispatch code. I'll see if I can add "better" visitor patter support for those classes if it does not make things too complicated.
There was a problem hiding this comment.
@TallJimbo, I have re-implemented double dispatch using visitor pattern, could you check latest commit d11f7e4 to see if that smells better now.
andy-slac
force-pushed
the
tickets/DM-17720
branch
from
3fddc64
to
d11f7e4
Compare
XOR is not universally supported in SQL grammar and is not in sqlalchemy which makes it hard to translate it to sqlalchemy expression. It's not really necessary and it can be expressed by other means.
User expression is now parsed by preflight and then translated into sqlalchemy clause expression. It now also supports using link names instead of table.column syntax in expressions.
It adds a little bit of code but it is cleaner solution overall and it should be safer for potential grammar extensions.
andy-slac
force-pushed
the
tickets/DM-17720
branch
from
d11f7e4
to
a466767
Compare
User expression is now parsed by preflight and then translated into
sqlalchemy clause expression. It now also supports using link names
instead of table.column syntax in expressions.