additional work to stop double wrapping nodes #1235
Conversation
…odes into the query tree for visitors that require it.
apmoriarty
requested review from
alerman,
billoley,
cawaring,
drewfarris,
hlgp,
ivakegg,
jschmidt10,
jwomeara,
keith-ratcliffe and
matthpeterson
as code owners
| * | ||
| * Ensure that all {@link org.apache.commons.jexl2.parser.ASTReferenceExpression} nodes have a parent ASTReference. | ||
| */ | ||
| public class EnsureReferenceNodesVisitor extends BaseVisitor { |
There was a problem hiding this comment.
Should this be a RebuildingVisitor?
There was a problem hiding this comment.
I concur. If the plan is for a visitor to modify the tree, it should probably be a rebuilding visitor.
| ASTJexlScript visitedScript = (ASTJexlScript) PullupUnexecutableNodesVisitor.pullupDelayedPredicates(script, true, config, indexedFields, | ||
| indexOnlyFields, nonEventFields, metadataHelper); | ||
| ASTJexlScript expectedScript = JexlASTHelper.parseJexlQuery(expected); | ||
| // assertTrue(TreeEqualityVisitor.isEqual(expectedScript, visitedScript)); |
apmoriarty
changed the title
| @@ -92,6 +92,9 @@ public ExpandMultiNormalizedTerms(ShardQueryConfiguration config, MetadataHelper | |||
| throw new DatawaveFatalQueryException(qe); | |||
| } | |||
|
|
|||
| // inject reference nodes when needed | |||
| script = (T) EnsureReferenceNodesVisitor.ensureReferences(script); | |||
There was a problem hiding this comment.
Why do we need this here? Is a previous visitor breaking this contract?
There was a problem hiding this comment.
ExpandMultiNormalizedTerms can produce a reference expression without a reference node
There was a problem hiding this comment.
But you call it before the visitor is actually called?
|
|
||
| // This swap should be safe, the reference node should have a single child that is a reference expression | ||
| JexlNode source = ASTDelayedPredicate.getDelayedPredicateSource(marker); | ||
| JexlNode wrappedSource = JexlNodes.wrap(source); |
There was a problem hiding this comment.
Won't this simply add extra parens?
| @@ -92,6 +92,9 @@ public RangeConjunctionRebuildingVisitor(ShardQueryConfiguration config, Scanner | |||
| throw new DatawaveFatalQueryException(qe); | |||
| } | |||
|
|
|||
| // inject reference nodes when needed | |||
| script = (T) EnsureReferenceNodesVisitor.ensureReferences(script); | |||
There was a problem hiding this comment.
Is some visitor breaking this contract such that we need to do this?
There was a problem hiding this comment.
The RangeConjunctionRebuildingVisitor can produce a reference expression without a reference node
There was a problem hiding this comment.
But you call it before the visitor is actually called?
There was a problem hiding this comment.
I still think that instead of adding in a new visitor to remove extra parens that we should use this as a tool to identify existing visitors which are double-wrapping nodes, and fix those visitors instead. Adding an extra visitor means more work to do during query planning and ultimately doesn't fix the root issue. That being said, we have already merged in broken code, which this apparently fixes... So, I will approve, but I ask that you create a ticket to fix the root cause, please.
| * | ||
| * Ensure that all {@link org.apache.commons.jexl2.parser.ASTReferenceExpression} nodes have a parent ASTReference. | ||
| */ | ||
| public class EnsureReferenceNodesVisitor extends BaseVisitor { |
There was a problem hiding this comment.
I concur. If the plan is for a visitor to modify the tree, it should probably be a rebuilding visitor.
|
|
||
| @Override | ||
| public Object visit(SimpleNode node, Object data) { | ||
| node.childrenAccept(this, data); |
There was a problem hiding this comment.
How is this a short-circuit?
| log.warn("Tried to pull up a delayed marker that had more than one child."); | ||
| } | ||
|
|
||
| // This swap should be safe, the reference node should have a single child that is a reference expression |
There was a problem hiding this comment.
You check, but you have not guaranteed that the reference node has a single child. Consider throwing an exception if you encounter a reference node with multiple children, or just remove the check & logging since this should never happen.
|
We determined there is a better way to go about fixing the root issues. We will revert the commit that has broken the code base. |
The original implementation for removing extra reference expressions broke a few assumptions made by various visitors.