Map EQL columns in ORDER BY clause
#122
Conversation
|
CI is failing on the new test, but should pass once cipherstash/encrypt-query-language#90 lands. Passes for me locally with EQL built from the linked PR branch. Edit: the EQL changes have been released and CI is passing now. |
CDThomas
force-pushed
the
feat/map-order-by
branch
from
516a46f to
e84ea1c
Compare
| convert_control_flow(self.inferencer.borrow_mut().enter(node))?; | ||
|
|
||
| ControlFlow::Continue(()) | ||
| } | ||
|
|
||
| fn exit<N: Visitable>(&mut self, node: &'ast N) -> ControlFlow<Break<Self::Error>> { | ||
| convert_control_flow(self.inferencer.borrow_mut().exit(node))?; | ||
| convert_control_flow(self.eql_function_tracker.borrow_mut().exit(node))?; |
There was a problem hiding this comment.
We want exit to be called for the function tracker after the inferencer. The function tracker relies on the inferencer to determine which nodes contain EQL values.
Note that all types aren't necessarily resolved at this point (specifically for literals), but that's currently OK because we're interested in types for identifiers (which are resolved by this point).
| #[derive(thiserror::Error, PartialEq, Eq, Debug)] | ||
| pub enum EqlFunctionTrackerError {} | ||
|
|
||
| impl<'ast> Visitor<'ast> for EqlFunctionTracker<'ast> { |
There was a problem hiding this comment.
The is a new visitor that we use in the type checker for determining which nodes need to be wrapped in EQL function calls (currently only EQL columns used in ORDER BY).
| // | ||
| // For example (assuming that `encrypted_col` is an identifier for an EQL column) transform | ||
| // `encrypted_col` to `cs_ore_64_8_v1(encrypted_col)`. | ||
| ast::Expr::Identifier(_) | ast::Expr::CompoundIdentifier(_) => { |
There was a problem hiding this comment.
This is the logic in the transformer that's responsible for actually wrapping nodes in EQL function calls (based on output of the new visitor in the type checker).
CDThomas
changed the title
ORDER BY in EQL ORE functionORDER BY clause
| /// Takes `eql_function_tracker`, consumes it, and returns a `HashSet` of keys for nodes | ||
| /// that the type checker has marked for wrapping with EQL function calls. | ||
| fn nodes_to_wrap(&self) -> HashSet<NodeKey<'ast>> { | ||
| self.eql_function_tracker.take().into_to_wrap() |
There was a problem hiding this comment.
Minor/nit into_to_wrap is not quite the same semantic as regular into
Name may not quite be right.
There was a problem hiding this comment.
Clippy will warn here if I remove the into_ prefix on into_to_wrap since the method consumes self.
Agreed that the naming feels a bit off though (even if into_ is the correct prefix). Maybe it would be more clear if I rename the method to into_nodes_to_wrap and return a NodesToWrap struct instead of a plain HashSet? I think that would more clearly show that the method consumes self and returns a different data structure.
A couple of examples of the into_ convention for methods that consume self in the std lib are into_iter (on various types) and Vec::into_flattened.
There was a problem hiding this comment.
Going to merge and consider refactoring to add in the NodesToWrap struct in a follow-up PR.
@auxesis I think that Proxy is generally missing docs on index types and the queries that they support at the moment. I think that I'd like to handle that in a separate PR so that I can keep this PR focused on internal changes for |
This change is the Proxy side of supporting
ORDER BYon EQL columns. The approach taken here is to wrap EQL columns inORDER BYwith the EQL function for ORE indexes.For example, the statement:
Maps to:
See the docs for new code and inline PR comments for more details on the implementation.
Related changes in the EQL repo: