Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Validate lineage for all JexlNode rebuilding visitors #925

Closed
wants to merge 10 commits into from
Closed

WIP Validate lineage for all JexlNode rebuilding visitors #925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cef34ff
Ensure valid lineage maintained by RebuildingVisitors
lbschanno Sep 19, 2020
9c207a7
Add FacetCheck unit tests
lbschanno Sep 23, 2020
1dce355
Fix BooleanOptimizationRebuildingVisitor lineage
lbschanno Sep 24, 2020
e8aa820
Add assert to assertScriptEquality
lbschanno Sep 24, 2020
b9dab24
Verify TreeFlatteningRebuildingVisitor lineage
lbschanno Sep 26, 2020
c6ca239
Verify GeoWavePruningVisitor lineage
lbschanno Sep 26, 2020
fe2682b
Merge branch 'master' into validate-lineage
lbschanno Sep 26, 2020
8df7304
Merge branch 'master' into validate-lineage
lbschanno Oct 18, 2020
a60fd4e
Remove System.out.print
lbschanno Oct 18, 2020
0f33dc8
Remove unrelated refactoring
lbschanno Oct 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions warehouse/query-core/src/main/java/datawave/query/jexl/JexlASTHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1609,14 +1609,14 @@ public static boolean validateLineage(JexlNode rootNode, boolean failHard) {
if (child != null) {
if (child.jjtGetParent() == null) {
if (failHard)
throw new RuntimeException("Failed to validate lineage: Tree included a child with a null parent.");
throw new RuntimeException("Failed to validate lineage: Tree included a child with a null parent. " + child );
else
log.error("Failed to validate lineage: Tree included a child with a null parent.");

result = false;
} else if (child.jjtGetParent() != node) {
if (failHard)
throw new RuntimeException("Failed to validate lineage: Included a child with a conflicting parent.");
throw new RuntimeException("Failed to validate lineage: Included a child with a conflicting parent. " + child);
else
log.error("Failed to validate lineage: Included a child with a conflicting parent.");

Expand All @@ -1625,7 +1625,7 @@ public static boolean validateLineage(JexlNode rootNode, boolean failHard) {
workingStack.push(child);
} else {
if (failHard)
throw new RuntimeException("Failed to validate lineage: Included a null child.");
throw new RuntimeException("Failed to validate lineage: Included a null child. " + child);
else
log.error("Failed to validate lineage: Included a null child.");

Expand Down
213 changes: 118 additions & 95 deletions warehouse/query-core/src/main/java/datawave/query/jexl/visitors/AllTermsIndexedVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,33 +28,36 @@
import org.apache.commons.jexl2.parser.ASTOrNode;
import org.apache.commons.jexl2.parser.JexlNode;
import org.apache.commons.jexl2.parser.JexlNodes;
import org.apache.commons.jexl2.parser.Node;
import org.apache.commons.jexl2.parser.ParserTreeConstants;
import org.apache.log4j.Logger;

import java.text.MessageFormat;
import java.util.Arrays;
import java.util.NoSuchElementException;
import java.util.Objects;

import static org.apache.commons.jexl2.parser.JexlNodes.promote;

/**
*
*
*/
public class AllTermsIndexedVisitor extends RebuildingVisitor {

private static final Logger log = Logger.getLogger(AllTermsIndexedVisitor.class);

protected final ShardQueryConfiguration config;
protected final MetadataHelper helper;

public AllTermsIndexedVisitor(ShardQueryConfiguration config, MetadataHelper helper) {
Preconditions.checkNotNull(config);
Preconditions.checkNotNull(helper);
Preconditions.checkNotNull(config, "ShardQueryConfiguration must not be null");
Preconditions.checkNotNull(helper, "MetadataHelper must not be null");

this.config = config;
this.helper = helper;
}

@SuppressWarnings("unchecked")
public static <T extends JexlNode> T isIndexed(T script, ShardQueryConfiguration config, MetadataHelper helper) {
Preconditions.checkNotNull(script);
Preconditions.checkNotNull(script, "JEXL script must not be null");

AllTermsIndexedVisitor visitor = new AllTermsIndexedVisitor(config, helper);

Expand All @@ -63,111 +66,56 @@ public static <T extends JexlNode> T isIndexed(T script, ShardQueryConfiguration

@Override
public Object visit(ASTJexlScript node, Object data) {
ASTJexlScript newNode = new ASTJexlScript(ParserTreeConstants.JJTJEXLSCRIPT);
newNode.image = node.image;

int newIndex = 0;
for (int i = 0; i < node.jjtGetNumChildren(); i++) {
Node newChild = (Node) node.jjtGetChild(i).jjtAccept(this, data);

if (newChild != null) {
// When we have an AND or OR
if ((newChild instanceof ASTOrNode || newChild instanceof ASTAndNode)) {
// Only add that node if it actually has children
if (0 < newChild.jjtGetNumChildren()) {
newNode.jjtAddChild(newChild, newIndex);
newIndex++;
}
} else {
// Otherwise, we want to add the child regardless
newNode.jjtAddChild(newChild, newIndex);
newIndex++;
}
}
}
JexlNode copy = getCopyWithVisitedChildren(node, data);

if (newNode.jjtGetNumChildren() == 0) {
if (copy.jjtGetNumChildren() == 0) {
NotFoundQueryException qe = new NotFoundQueryException(DatawaveErrorCode.NO_ANYFIELD_EXPANSION_MATCH);
log.warn(qe);
throw new EmptyUnfieldedTermExpansionException(qe);
}

return newNode;
return copy;
}

@Override
public Object visit(ASTOrNode node, Object data) {
ASTOrNode newNode = new ASTOrNode(ParserTreeConstants.JJTORNODE);
newNode.image = node.image;

int newIndex = 0;
for (int i = 0; i < node.jjtGetNumChildren(); i++) {
Node newChild = (Node) node.jjtGetChild(i).jjtAccept(this, data);

if (newChild != null) {
// When we have an AND or OR
if ((newChild instanceof ASTOrNode || newChild instanceof ASTAndNode)) {
// Only add that node if it actually has children
if (0 < newChild.jjtGetNumChildren()) {
newNode.jjtAddChild(newChild, newIndex);
newIndex++;
}
} else {
// Otherwise, we want to add the child regardless
newNode.jjtAddChild(newChild, newIndex);
newIndex++;
}
}
}

switch (newNode.jjtGetNumChildren()) {
case 0:
return null;
case 1:
JexlNode child = newNode.jjtGetChild(0);
JexlNodes.promote(newNode, child);
return child;
default:
return newNode;
}
return visitJunctionNode(node, data);
}

@Override
public Object visit(ASTAndNode node, Object data) {
ASTAndNode newNode = new ASTAndNode(ParserTreeConstants.JJTANDNODE);
newNode.image = node.image;
return visitJunctionNode(node, data);
}

private Object visitJunctionNode(JexlNode node, Object data) {
JexlNode copy = getCopyWithVisitedChildren(node, data);

int newIndex = 0;
for (int i = 0; i < node.jjtGetNumChildren(); i++) {
Node newChild = (Node) node.jjtGetChild(i).jjtAccept(this, data);

if (newChild != null) {
// When we have an AND or OR
if ((newChild instanceof ASTOrNode || newChild instanceof ASTAndNode)) {
// Only add that node if it actually has children
if (0 < newChild.jjtGetNumChildren()) {
newNode.jjtAddChild(newChild, newIndex);
newIndex++;
}
} else {
// Otherwise, we want to add the child regardless
newNode.jjtAddChild(newChild, newIndex);
newIndex++;
}
}
}
switch (newNode.jjtGetNumChildren()) {
switch (copy.jjtGetNumChildren()) {
case 0:
return null;
case 1:
JexlNode child = newNode.jjtGetChild(0);
JexlNodes.promote(newNode, child);
JexlNode child = copy.jjtGetChild(0);
promote(copy, child);
return child;
default:
return newNode;
return copy;
}
}

private JexlNode getCopyWithVisitedChildren(JexlNode node, Object data) {
// @formatter:off
JexlNode[] children = Arrays.stream(JexlNodes.children(node))
lbschanno marked this conversation as resolved.
Show resolved Hide resolved
.map(n -> (JexlNode) n.jjtAccept(this, data)) // Visit the node
.filter(Objects::nonNull) // Filter out null nodes
.filter(n -> (!JexlNodes.isAnd(n) && !JexlNodes.isOr(n)) || JexlNodes.isNotChildless(n)) // Filter out empty junction nodes
.toArray(JexlNode[]::new);
// @formatter:on
JexlNode copy = JexlNodes.newInstanceOfType(node);
copy.image = node.image;
JexlNodes.children(copy, children);
return copy;
}

@Override
public Object visit(ASTEQNode node, Object data) {
return equalityVisitor(node, data);
Expand All @@ -179,14 +127,16 @@ public Object visit(ASTNENode node, Object data) {
}

/**
* Determine, for a binary equality node, if the field name is indexed
*
* Determine, for a binary equality node, if the field name is indexed.
*
* @param node
* the node to verify the indexed status of
* @param data
* @return
* the node data
* @return a copy of the
*/
protected JexlNode equalityVisitor(JexlNode node, Object data) {
String fieldName = null;
String fieldName;
try {
fieldName = JexlASTHelper.getIdentifier(node);
} catch (NoSuchElementException e) {
Expand All @@ -196,10 +146,10 @@ protected JexlNode equalityVisitor(JexlNode node, Object data) {
throw new InvalidFieldIndexQueryFatalQueryException(qe);
}
try {
// in the case of an ANY_FIELD or NO_FIELD, the query could not be exapnded against the index and hence this
// In the case of an ANY_FIELD or NO_FIELD, the query could not be expanded against the index and hence this
// term has no results. Leave it in the query as such.
if (Constants.ANY_FIELD.equals(fieldName) || Constants.NO_FIELD.equals(fieldName)) {
return copy(node);
return node;
}

if (!this.helper.isIndexed(fieldName, config.getDatatypeFilter())) {
Expand All @@ -211,59 +161,132 @@ protected JexlNode equalityVisitor(JexlNode node, Object data) {
QueryException qe = new QueryException(DatawaveErrorCode.INDETERMINATE_INDEX_STATUS, e);
throw new DatawaveFatalQueryException(qe);
}
return copy(node);

return node;
}

/**
* Throw an {@link datawave.query.exceptions.InvalidFieldIndexQueryFatalQueryException}.
*
* @param node
* the node
* @param data
* the node data
* @return nothing
*/
@Override
public Object visit(ASTERNode node, Object data) {
QueryException qe = new QueryException(DatawaveErrorCode.EQUALS_REGEX_NODE_PROCESSING_ERROR, MessageFormat.format("Node: {0}", PrintingVisitor
.formattedQueryString(node).replace('\n', ' ')));
throw new InvalidFieldIndexQueryFatalQueryException(qe);
}

/**
* Throw an {@link datawave.query.exceptions.InvalidFieldIndexQueryFatalQueryException}.
*
* @param node
* the node
* @param data
* the node data
* @return nothing
*/
@Override
public Object visit(ASTNRNode node, Object data) {
QueryException qe = new QueryException(DatawaveErrorCode.NOT_EQUALS_REGEX_NODE_PROCESSING_ERROR, MessageFormat.format("Node: {0}", PrintingVisitor
.formattedQueryString(node).replace('\n', ' ')));
throw new InvalidFieldIndexQueryFatalQueryException(qe);
}

/**
* Throw an {@link datawave.query.exceptions.InvalidFieldIndexQueryFatalQueryException}.
*
* @param node
* the node
* @param data
* the node data
* @return nothing
*/
@Override
public Object visit(ASTLTNode node, Object data) {
QueryException qe = new QueryException(DatawaveErrorCode.LT_NODE_PROCESSING_ERROR, MessageFormat.format("Node: {0}", PrintingVisitor
.formattedQueryString(node).replace('\n', ' ')));
throw new InvalidFieldIndexQueryFatalQueryException(qe);
}

/**
* Throw an {@link datawave.query.exceptions.InvalidFieldIndexQueryFatalQueryException}.
*
* @param node
* the node
* @param data
* the node data
* @return nothing
*/
@Override
public Object visit(ASTGTNode node, Object data) {
QueryException qe = new QueryException(DatawaveErrorCode.GT_NODE_PROCESSING_ERROR, MessageFormat.format("Node: {0}", PrintingVisitor
.formattedQueryString(node).replace('\n', ' ')));
throw new InvalidFieldIndexQueryFatalQueryException(qe);
}

/**
* Throw an {@link datawave.query.exceptions.InvalidFieldIndexQueryFatalQueryException}.
*
* @param node
* the node
* @param data
* the node data
* @return nothing
*/
@Override
public Object visit(ASTLENode node, Object data) {
QueryException qe = new QueryException(DatawaveErrorCode.LTE_NODE_PROCESSING_ERROR, MessageFormat.format("Node: {0}", PrintingVisitor
.formattedQueryString(node).replace('\n', ' ')));
throw new InvalidFieldIndexQueryFatalQueryException(qe);
}

/**
* Throw an {@link datawave.query.exceptions.InvalidFieldIndexQueryFatalQueryException}.
*
* @param node
* the node
* @param data
* the node data
* @return nothing
*/
@Override
public Object visit(ASTGENode node, Object data) {
QueryException qe = new QueryException(DatawaveErrorCode.GTE_NODE_PROCESSING_ERROR, MessageFormat.format("Node: {0}", PrintingVisitor
.formattedQueryString(node).replace('\n', ' ')));
throw new InvalidFieldIndexQueryFatalQueryException(qe);
}

/**
* Throw an {@link datawave.query.exceptions.InvalidFieldIndexQueryFatalQueryException}.
*
* @param node
* the node
* @param data
* the node data
* @return nothing
*/
@Override
public Object visit(ASTAssignment node, Object data) {
QueryException qe = new QueryException(DatawaveErrorCode.ASSIGNMENT_NODE_PROCESSING_ERROR, MessageFormat.format("Node: {0}", PrintingVisitor
.formattedQueryString(node).replace('\n', ' ')));
throw new InvalidFieldIndexQueryFatalQueryException(qe);
}

/**
* Throw an {@link datawave.query.exceptions.InvalidFieldIndexQueryFatalQueryException}.
*
* @param node
* the node
* @param data
* the node data
* @return nothing
*/
@Override
public Object visit(ASTFunctionNode node, Object data) {
QueryException qe = new QueryException(DatawaveErrorCode.FUNCTION_NODE_PROCESSING_ERROR, MessageFormat.format("Node: {0}", PrintingVisitor
.formattedQueryString(node).replace('\n', ' ')));
Expand Down
3 changes: 2 additions & 1 deletion ...core/src/main/java/datawave/query/jexl/visitors/BooleanOptimizationRebuildingVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ protected JexlNode optimizeTree(JexlNode currentNode, JexlNode newNode, Object d
for (int i = 0; i < toAttach.jjtGetNumChildren(); i++) {
JexlNode node = copy(prunedNode);
JexlNode attach = (JexlNode) toAttach.jjtGetChild(i).jjtAccept(this, data);
attach.jjtSetParent(node);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see lots of refactoring here but this is the only line that actually fixes a parenting problem for this class. Refactoring for the sake of refactoring may do more harm than good if anybody else is working on these visitors for any other reason. It also makes it very difficult for anybody to actually accept the entire package and merge. I think you may need to pull back a little from the refactoring efforts. Better yet a separate pull request per visitor that is only refactoring would be considerably more palatable.

node.jjtAddChild(attach, node.jjtGetNumChildren());
newNode.jjtAddChild(node, newNode.jjtGetNumChildren());
node.jjtSetParent(newNode);
Expand Down Expand Up @@ -110,7 +111,7 @@ && hasChildOr(child)) {

/**
* Returns true if there is a child OR (or OR directly inside ASTReference or ASTReferenceException).
*
*
* @param currentNode
* @return
*/
Expand Down
Loading