DonalEvans commented on a change in pull request #5400:
URL: https://github.com/apache/geode/pull/5400#discussion_r461909472
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -939,7 +939,7 @@ protected boolean evaluateEntry(IndexInfo indexInfo,
ExecutionContext context, O
// either of them is null and operator is other than == or !=
if (result == QueryService.UNDEFINED) {
// Undefined is added to results for != conditions only
- if (operator != OQLLexerTokenTypes.TOK_NE || operator !=
OQLLexerTokenTypes.TOK_NE_ALT) {
Review comment:
It is a change in behaviour, but I think it's a change that introduces
the originally intended behaviour. The previous logic always evaluated to true,
which made me think that there might have been a mistake in it. The comment
about "for != conditions only" I think is referring to the `TOK_NE` and
`TOK_NE_ALT` types (not equal and not equal alt), not that the operator should
be not equal to them.
Previously, any time the result was `UNDEFINED` it would be added to the
results, but with the change, it's only added to the results if the operator is
`TOK_NE` or `TOK_NE_ALT`, which sounds from the comment to be what was
originally intended. All of the tests are passing here with this change, and I
ran some other tests privately that also hit no issues, so I'm fairly confident
that this isn't breaking anything.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
