Repository navigation
Conversation
foster33
marked this pull request as draft
October 9, 2026 15:41
foster33
marked this pull request as ready for review
October 9, 2026 16:45
lbschanno
requested changes
Oct 9, 2026
| * <p> | ||
| * All simple comparison operators are inspected (equality, inequality, and the four range operators). | ||
| * Null and numeric literals are ignored. This is the inverse of {@link FieldsWithNumericValuesVisitor}; | ||
| * see also {@link FieldsWithNumericRangeVisitor} for the range only numeric case. |
Collaborator
There was a problem hiding this comment.
Suggested change
| * see also {@link FieldsWithNumericRangeVisitor} for the range only numeric case. | |
| * see also {@link FieldsWithNumericRangeValuesVisitor} for the range only numeric case. |
| import datawave.query.util.TypeMetadata; | ||
|
|
||
| /** | ||
| * A {@link QueryRule} implementation that checks a JEXL query for the usage of numeric values for non-numeric fields. |
Collaborator
There was a problem hiding this comment.
This class-level comment should be updated to include information about also checking for non-numeric values given for numeric fields.
|
|
||
| import datawave.query.jexl.JexlASTHelper; | ||
|
|
||
| class FieldsWithNonNumericValuesVisitorTest { |
lbschanno
reviewed
Oct 9, 2026
| /** | ||
| * A {@link QueryRule} implementation that checks a JEXL query for the usage of numeric values for non-numeric fields. | ||
| */ | ||
| public class NumericValueRule extends ShardQueryRule { |
Collaborator
There was a problem hiding this comment.
Probably a bit of a pain to rename what with needing to update configuration files, but perhaps this should be renamed to NumericFieldsRule. Not a requirement, though.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NumericValueRulecurrently only checks one direction: a numeric range (<,>,<=,>=) applied to a field that is not aNumberType. It does not catch the inverse, a non-numeric literal applied to a field that IS aNumberType.This issue shows up when a term like
FOO:>60is parsed.>is a legal term character, so the query becomes the JEXL equalityFOO == '>60'. IfFOOis aNumberTypefield,/validatereturns no message and the query later fails during normalization.This change adds the inverse check:
FieldsWithNonNumericValuesVisitorcollects fields compared (==,!=,<,>,<=,>=) against a string literal thatNumberUtils.isCreatablerejects.NumericValueRuleruns the new visitor alongside the existing range check and emitsNon-numeric values supplied for numeric field(s): ...for any such field whoseTypeMetadataincludesNumberType.The
TypeMetadatalookup cache is hoisted so both checks share it. The existing range check message and behavior are unchanged.