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.
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
Finish refactoring IngestTypeVisitor, add datatype focused test #2347
Finish refactoring IngestTypeVisitor, add datatype focused test #2347
Conversation
apmoriarty
commented
Apr 17, 2024
•
edited
edited
- completed refactoring datatype-gathering logic from the IngestTypePruningVisitor to the IngestTypeVisitor.
- add ShapesTest, a WiseGuys-style test with a focus on querying across datatypes
- updated IngestTypePruningVisitor and IngestTypeVisitor tests based on integration testing
05f8b42
to
820a244
Compare
warehouse/query-core/src/test/java/datawave/query/ShapesTest.java
Outdated
Show resolved
Hide resolved
320ca82
to
fdcca13
Compare
a94b830
to
0bc9071
Compare
@@ -82,11 +83,13 @@ public static Set<String> getIngestTypes(JexlNode node, TypeMetadata typeMetadat | |||
Object o = node.jjtAccept(visitor, null); | |||
if (o instanceof Set) { | |||
Set<String> ingestTypes = (Set<String>) o; | |||
if (!ingestTypes.contains(UNKNOWN_TYPE)) { | |||
if (ingestTypes.contains(UNKNOWN_TYPE)) { | |||
ingestTypes.retainAll(Collections.singleton(UNKNOWN_TYPE)); | |||
return ingestTypes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated return. Remove this one and the flow doesnt change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, in this case I only want to return a collection that contains the UNKNOWN_TYPE
. I'll add a comment to that effect.
warehouse/query-core/src/main/java/datawave/query/jexl/visitors/IngestTypePruningVisitor.java
Show resolved
Hide resolved
@@ -438,7 +441,7 @@ public void testEvaluationOnlyField() { | |||
@Test | |||
public void testPruneNegation() { | |||
String query = "A == '1' || !((_Delayed_ = true) && (A == '1' && C == '2'))"; | |||
test(query, "A == '1'"); | |||
test(query, query); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this indicate we are not pruning the negation, if the expected now has the negation included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the negation would self-prune, let me take a closer look at why this stopped visiting the negated branch..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small things. Nothing too major. Please check the unit test with changed expected output.
warehouse/query-core/src/main/java/datawave/query/jexl/visitors/IngestTypePruningVisitor.java
Show resolved
Hide resolved
warehouse/query-core/src/main/java/datawave/query/jexl/visitors/IngestTypeVisitor.java
Show resolved
Hide resolved
warehouse/query-core/src/main/java/datawave/query/jexl/visitors/IngestTypeVisitor.java
Show resolved
Hide resolved
warehouse/query-core/src/test/java/datawave/query/CompositeFunctionsTest.java
Outdated
Show resolved
Hide resolved
64e2bfa
to
815e48c
Compare
…ion interaction Completed refactor of IngestTypeVisitor and IngestTypePruningVisitor Changed how visitor handles negations and null terms
815e48c
to
3fe6790
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!