WIP Validate lineage for all JexlNode rebuilding visitors #925

lbschanno · 2020-09-26T03:24:53Z

WIP for #880 for verifying/fixing JexlNode rebuilding visitors to ensure that they preserve proper parentage.

I have fixed/verified parentage for the following visitors:

AllTermsIndexedVisitor
BooleanOptimizationVisitor
FacetCheck
DateIndexCleanupVisitor
ExpandMultinormalizedTerms
TreeFlatteningRebuildingVisitor
FixNegativeNumbersVisitor
GeowavePruningVisitor
QueryModelVisitor
RangeCoalescingVisitor
RegexFunctionVisitor

I am currently working on fixing parentage for the following visitors:

UniqueExpressionTermsVisitor
ExpandCompositeTerms

There are a number of rebuilding visitors that do not have any unit tests associated with them that I could find, nor was there enough documentation for me to be able to extrapolate test cases for them. I would appreciate any help with examples or desired test cases so that I can write tests for each of the following and verify parentage.

BoundedRangeDetectionVisitor
FunctionIndexQueryExpansionVisitor
FunctionNormalizationRebuildingVisitor
IsNotNullIntentVisitor
ParallelIndexExpansion
PruneLessSelectiveFieldsVisitor
PushdownLargeFieldedListsVisitor
PushdownMissingIndexRangeNodesVisitor
PushFunctionsIntoExceededValueRanges
RangeConjunctionRebuildingVisitor
RangeExpansionThresholdRebuildingVisitor

Multiple RebuildingVisitor implementations do not return a rebuilt query that is considered valid according to JexlASTHelper.validateLineage(). Ensure that existing RebuildingVisitors are tested, and when necessary, modified to return a JEXL tree with a valid lineage. Fixes #880 Fix AllTermsIndexedVisitor Verify DateIndexCleanupVisitor lineage Verify ExpandMultinormalizedTerms lineage Verify FixNegativeNumbersVisitor lineage Remove unnecessary query print Remove System.out.print Verify FixUnindexedNumericTerms lineage Verify QueryModelVisitor lineage Fix RangeCoalescingVisitor Verify RegexFunctionVisitor lineage

warehouse/query-core/src/main/java/datawave/query/jexl/visitors/AllTermsIndexedVisitor.java

ivakegg · 2020-10-08T20:42:02Z

...ry-core/src/main/java/datawave/query/jexl/visitors/BooleanOptimizationRebuildingVisitor.java


            for (int i = 0; i < toAttach.jjtGetNumChildren(); i++) {
                JexlNode node = copy(prunedNode);
                JexlNode attach = (JexlNode) toAttach.jjtGetChild(i).jjtAccept(this, data);
+                attach.jjtSetParent(node);


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.

ivakegg · 2020-10-09T14:38:59Z

warehouse/query-core/src/main/java/datawave/query/jexl/visitors/QueryModelVisitor.java

 import datawave.query.exceptions.DatawaveFatalQueryException;
 import datawave.query.jexl.JexlASTHelper;
 import datawave.query.jexl.JexlNodeFactory;
 import datawave.query.jexl.JexlNodeFactory.ContainerType;
+import datawave.query.model.QueryModel;


OK, I am stumped in this visitor as to where any lineage problems were fixed. Is this one purely refactoring?

ivakegg · 2020-10-09T14:41:32Z

warehouse/query-core/src/main/java/datawave/query/jexl/visitors/RangeCoalescingVisitor.java

                andNode.jjtAddChild(newChild, i);
                newChild.jjtSetParent(andNode);
            }
        }
-
+


The changes in this visitor looks good.

ivakegg · 2020-10-09T17:03:44Z

warehouse/query-core/src/main/java/datawave/query/jexl/visitors/RegexFunctionVisitor.java

-                if (functionMetadata.name().equals("excludeRegex")) {
-                    newParent = new ASTAndNode(ParserTreeConstants.JJTANDNODE);
+
+        switch (functionMetadata.name()) {


much cleaner, thanks

ivakegg · 2020-10-09T17:48:46Z

First I must explain what a (query property) "marker" node is. A portion of a query (jexl subtree) can be marked as have some attribute by attaching an assignment node to that subtree. So if I want to mark some node A with a marker N, then I replace A with ((N = true) && (A)). These markers can be thought of as query execution hints that the planner puts in place to allow the execution to do certain things.

Second I must explain what "push down" means. To "push down" some portion of a query (jexl subtree) is to use the field index instead of the global index, or pushing down farther to the query evaluation phase. Query execution basically started with global index lookup, then field index lookup (if required), then query evaluation meaning bouncing the boolean logic against all of the values for a document.

Here is some description of these visitors:

BoundedRangeDetectionVisitor
This is supposed to detect whether there are bounded ranges, however it appears to only be looking for bounds. I am actually changing this visitor here: Change to mark bounded ranges in JEXL #926
FunctionIndexQueryExpansionVisitor
This will expand all of the functions within the query to include something that we can run against the index. Basically every function node in the tree will be replaced with (function && (<some query)). The index query created will be dependent on the query descriptor for the query in question.
FunctionNormalizationRebuildingVisitor
This visitor will normalize the values within the arguments of a function node. The reason this is separated out is that the function descriptor needs to be used to determine which arguments are actually values and for which field do they apply. The field name is needed to lookup the data type and subsequently the normalization required. Normalization of values is required for index lookup.
IsNotNullIntentVisitor
if an ER (regular expression) node is of the form FIELD =~ '*', then replace with: FIELD != null
ParallelIndexExpansion
This is used for expanding regex nodes into the set of discrete values. It is also used for unfielded term expansion (FixUnfieldedTermsVisitor) by extension. If a regex (or range) cannot be expanded into descrete values, then it is wrapped with an ExceededValueThresholdMarkerJexlNode marker.
PruneLessSelectiveFieldsVisitor
Not used for now. You can ignore this one for now until we start populating the metadata with more extensive metrics at which time this visitor may be changed extensively.
PushdownLargeFieldedListsVisitor
This will take a very large list of values OR'ed together for one field into an ExceededOrThresholdMarkerJexlNode construct. This essentially will allow an "Ivarator" (DatawaveFieldIndedListIteratorJexl) to be used instead of creating a separate source for each term (see QueryIterator).
PushdownMissingIndexRangeNodesVisitor
This will push down terms as "evaluation only" (meaning the index is not used) when there is an entry in the metadata table that denotes that the field is not indexed for some time range. This is an initial form of a solution for Handle the cases were a field is both indexed and not indexed within a time range #825.
PushFunctionsIntoExceededValueRanges
If we have an ExceededValueThresholdMarkerJexlNode on a range which had been produced as a result of the FunctionIndexQueryExpansionVisitor above, then the function is pushed into the marker node construct to allow the "Ivarator" to evaluation the function when scanning the field index.
RangeConjunctionRebuildingVisitor
This will expand ranges into discrete values as the ParallelIndexExpansion visitor does for regex nodes.
RangeExpansionThresholdRebuildingVisitor
This one is not used and can be ignored/dropped.

lbschanno · 2020-10-18T07:34:13Z

@ivakegg I have removed any refactoring not related to fixes. I will create separate PRs for that work as we discussed so that this PR is focused on fixes only.

lbschanno · 2020-10-20T17:45:49Z

Closing this PR to break it up into individual PRs for each visitor.

lbschanno added 6 commits September 23, 2020 15:46

Add FacetCheck unit tests

9c207a7

Fix BooleanOptimizationRebuildingVisitor lineage

1dce355

Add assert to assertScriptEquality

e8aa820

Verify TreeFlatteningRebuildingVisitor lineage

b9dab24

Verify GeoWavePruningVisitor lineage

c6ca239

lbschanno requested review from alerman, apmoriarty, billoley, cawaring, drewfarris, hlgp, ivakegg, jschmidt10, jwomeara, keith-ratcliffe and matthpeterson as code owners September 26, 2020 03:24

lbschanno changed the title ~~WIP: Validate lineage for all JexlNode rebuilding visitors~~ WIP Validate lineage for all JexlNode rebuilding visitors Sep 26, 2020

Merge branch 'master' into validate-lineage

fe2682b

lbschanno marked this pull request as draft September 26, 2020 03:26

ivakegg requested changes Oct 8, 2020

View reviewed changes

ivakegg reviewed Oct 9, 2020

View reviewed changes

ivakegg mentioned this pull request Oct 9, 2020

Handle the cases were a field is both indexed and not indexed within a time range #825

Closed

lbschanno added 3 commits October 18, 2020 03:15

Merge branch 'master' into validate-lineage

8df7304

Remove System.out.print

a60fd4e

Remove unrelated refactoring

0f33dc8

lbschanno closed this Oct 20, 2020

lbschanno deleted the validate-lineage branch November 5, 2020 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Validate lineage for all JexlNode rebuilding visitors #925

WIP Validate lineage for all JexlNode rebuilding visitors #925

lbschanno commented Sep 26, 2020

ivakegg Oct 8, 2020

ivakegg Oct 9, 2020

ivakegg Oct 9, 2020 •

edited

Loading

ivakegg Oct 9, 2020

ivakegg commented Oct 9, 2020

lbschanno commented Oct 18, 2020

lbschanno commented Oct 20, 2020

WIP Validate lineage for all JexlNode rebuilding visitors #925

WIP Validate lineage for all JexlNode rebuilding visitors #925

Conversation

lbschanno commented Sep 26, 2020

ivakegg Oct 8, 2020

Choose a reason for hiding this comment

ivakegg Oct 9, 2020

Choose a reason for hiding this comment

ivakegg Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

ivakegg Oct 9, 2020

Choose a reason for hiding this comment

ivakegg commented Oct 9, 2020

lbschanno commented Oct 18, 2020

lbschanno commented Oct 20, 2020

ivakegg Oct 9, 2020 •

edited

Loading