You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/02/11 08:15:18 UTC
[GitHub] [geode] albertogpz opened a new pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
albertogpz opened a new pull request #6028:
URL: https://github.com/apache/geode/pull/6028
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] albertogpz closed pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
albertogpz closed pull request #6028:
URL: https://github.com/apache/geode/pull/6028
--
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:
users@infra.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
jhuynh1 commented on pull request #6028:
URL: https://github.com/apache/geode/pull/6028#issuecomment-788226436
Usually the queries that don't have indexes are considered "correct" and the ones with indexes usually were checked against those results.
Was the issue due to the map index not mapping values it didn't know about?
A solution down the line might be to allow custom indexing.
Is there a way to disable the != only for queries that use the map index? Queries that use the base compact range index should still be able to execute != in a timely fashion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a change in pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #6028:
URL: https://github.com/apache/geode/pull/6028#discussion_r576391687
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -1506,7 +1506,8 @@ private void applyProjectionForIndexInit(List currentRuntimeIters)
: modifiedIndexExpr.evaluate(this.initContext);
if (indexKey == null) {
- indexKey = IndexManager.NULL;
+ // indexKey = IndexManager.NULL;
Review comment:
I think you are right. There is trouble with the != queries.
Shouldn't they not use the index?
According to the documentation, indexes are not used in expressions that contain NOT(https://geode.apache.org/docs/guide/19/developing/query_index/indexing_guidelines.html) . I think the same could be done for queries using != which is very similar to using NOT.
Nevertheless, when I checked the code, it seems that queries with NOT also used the indexes.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6028:
URL: https://github.com/apache/geode/pull/6028#issuecomment-789988265
> > > > Usually the queries that don't have indexes are considered "correct" and the ones with indexes usually were checked against those results.
> > > > Was the issue due to the map index not mapping values it didn't know about?
> > > > A solution down the line might be to allow custom indexing.
> > > > Is there a way to disable the != only for queries that use the map index? Queries that use the base compact range index should still be able to execute != in a timely fashion.
> > >
> > >
> > > I created the test in order to compare queries with and without indexes. Nevertheless, I think that currently queries without indexes do not give the right results with where clauses in which the map for the key passed is compared with null, for example: "positions['SUN'] != null" or "positions['SUN'] = null. In these cases for entries that contain the map (positions != null) but positions does not contain the "SUN" key, it considers that positions['SUN'] is null.
> > > I have changed the code in the latest commits to work the way I expected.
> > > Besides, I have added a change so that indexes are used again when using CompactRangeIndexes but not when using CompactMapRangeIndexes.
> > > There's only 3 test cases that are failing and I still need to investigate.
> >
> >
> > The cases that are failing try to create the following index:
> > pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID]
> > They also fail if the index is: pf.collectionHolderMap[(pf.ID).toString()]
> > I wonder what type of index is that. According to the documentation, when you create a Map index you specify either "*" in order to create an index for every key or one or more keys (v.g. "SUN", IBM",...) but in those cases the index contains something variable.
> > Does it make sense to support it given that there is no such thing in the documentation?
>
> pf.collectionHolderMap[(pf.ID).toString()] <= I think(not 100% sure) this can be considered equivalent to entry.mapField["ID value as key"] So similar to a map index
>
> pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID] <= I think is a map index where the value that is mapped happens to be an array. For example "SELECT * FROM /exampleRegion p WHERE p.collectionHolderMap.get('1').arr[0] = '0'"
>
> This query is listed in the doc here: https://gemfire.docs.pivotal.io/96/geode/getting_started/querying_quick_reference.html
> Although it probably discussed in detail in the indexing portion. I would guess the docs got too cluttered with all the types of queries that are possible with OQL (as one can continue to drill down into objects infinitely)
The query makes perfect sense to me. What is not that clear is that you use a variable as a key in the map index. Because in that, case,
> > > > Usually the queries that don't have indexes are considered "correct" and the ones with indexes usually were checked against those results.
> > > > Was the issue due to the map index not mapping values it didn't know about?
> > > > A solution down the line might be to allow custom indexing.
> > > > Is there a way to disable the != only for queries that use the map index? Queries that use the base compact range index should still be able to execute != in a timely fashion.
> > >
> > >
> > > I created the test in order to compare queries with and without indexes. Nevertheless, I think that currently queries without indexes do not give the right results with where clauses in which the map for the key passed is compared with null, for example: "positions['SUN'] != null" or "positions['SUN'] = null. In these cases for entries that contain the map (positions != null) but positions does not contain the "SUN" key, it considers that positions['SUN'] is null.
> > > I have changed the code in the latest commits to work the way I expected.
> > > Besides, I have added a change so that indexes are used again when using CompactRangeIndexes but not when using CompactMapRangeIndexes.
> > > There's only 3 test cases that are failing and I still need to investigate.
> >
> >
> > The cases that are failing try to create the following index:
> > pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID]
> > They also fail if the index is: pf.collectionHolderMap[(pf.ID).toString()]
> > I wonder what type of index is that. According to the documentation, when you create a Map index you specify either "*" in order to create an index for every key or one or more keys (v.g. "SUN", IBM",...) but in those cases the index contains something variable.
> > Does it make sense to support it given that there is no such thing in the documentation?
>
> pf.collectionHolderMap[(pf.ID).toString()] <= I think(not 100% sure) this can be considered equivalent to entry.mapField["ID value as key"] So similar to a map index
>
> pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID] <= I think is a map index where the value that is mapped happens to be an array. For example "SELECT * FROM /exampleRegion p WHERE p.collectionHolderMap.get('1').arr[0] = '0'"
>
> This query is listed in the doc here: https://gemfire.docs.pivotal.io/96/geode/getting_started/querying_quick_reference.html
> Although it probably discussed in detail in the indexing portion. I would guess the docs got too cluttered with all the types of queries that are possible with OQL (as one can continue to drill down into objects infinitely)
The query makes perfect sense to me. What is not that clear is what type of index is created because neither a fixed value nor "*" is used as a key to the map but rather a variable. That leads me to think that it is not a Map index.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
jhuynh1 commented on pull request #6028:
URL: https://github.com/apache/geode/pull/6028#issuecomment-781672802
Interesting change! I'm going to think about it a bit, if none of the existing tests have failed, then I feel a bit more comfortable with this change. It would be good to add a test for the scenario you were interested in fixing (maybe making sure the number of index mappings is < the number of entries.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6028:
URL: https://github.com/apache/geode/pull/6028#issuecomment-789073349
> Usually the queries that don't have indexes are considered "correct" and the ones with indexes usually were checked against those results.
> Was the issue due to the map index not mapping values it didn't know about?
>
> A solution down the line might be to allow custom indexing.
>
> Is there a way to disable the != only for queries that use the map index? Queries that use the base compact range index should still be able to execute != in a timely fashion.
I created the test in order to compare queries with and without indexes. Nevertheless, I think that currently queries without indexes do not give the right results with where clauses in which the map for the key passed is compared with null, for example: "positions['SUN'] != null" or "positions['SUN'] = null. In these cases for entries that contain the map (positions != null) but positions does not contain the "SUN" key, it considers that positions['SUN'] is null.
I have changed the code in the latest commits to work the way I expected.
Besides, I have added a change so that indexes are used again when using CompactRangeIndexes but not when using CompactMapRangeIndexes.
There's only 3 test cases that are failing and I still need to investigate.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6028:
URL: https://github.com/apache/geode/pull/6028#issuecomment-789755947
> > Usually the queries that don't have indexes are considered "correct" and the ones with indexes usually were checked against those results.
> > Was the issue due to the map index not mapping values it didn't know about?
> > A solution down the line might be to allow custom indexing.
> > Is there a way to disable the != only for queries that use the map index? Queries that use the base compact range index should still be able to execute != in a timely fashion.
>
> I created the test in order to compare queries with and without indexes. Nevertheless, I think that currently queries without indexes do not give the right results with where clauses in which the map for the key passed is compared with null, for example: "positions['SUN'] != null" or "positions['SUN'] = null. In these cases for entries that contain the map (positions != null) but positions does not contain the "SUN" key, it considers that positions['SUN'] is null.
>
> I have changed the code in the latest commits to work the way I expected.
>
> Besides, I have added a change so that indexes are used again when using CompactRangeIndexes but not when using CompactMapRangeIndexes.
>
> There's only 3 test cases that are failing and I still need to investigate.
The cases that are failing try to create the following index:
pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID]
They also fail if the index is: pf.collectionHolderMap[(pf.ID).toString()]
I wonder what type of index is that. According to the documentation, when you create a Map index you specify either "*" in order to create an index for every key or one or more keys (v.g. "SUN", IBM",...) but in those cases the index contains something variable.
Does it make sense to support it given that there is no such thing in the documentation?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] albertogpz edited a comment on pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
albertogpz edited a comment on pull request #6028:
URL: https://github.com/apache/geode/pull/6028#issuecomment-789988265
> > > > Usually the queries that don't have indexes are considered "correct" and the ones with indexes usually were checked against those results.
> > > > Was the issue due to the map index not mapping values it didn't know about?
> > > > A solution down the line might be to allow custom indexing.
> > > > Is there a way to disable the != only for queries that use the map index? Queries that use the base compact range index should still be able to execute != in a timely fashion.
> > >
> > >
> > > I created the test in order to compare queries with and without indexes. Nevertheless, I think that currently queries without indexes do not give the right results with where clauses in which the map for the key passed is compared with null, for example: "positions['SUN'] != null" or "positions['SUN'] = null. In these cases for entries that contain the map (positions != null) but positions does not contain the "SUN" key, it considers that positions['SUN'] is null.
> > > I have changed the code in the latest commits to work the way I expected.
> > > Besides, I have added a change so that indexes are used again when using CompactRangeIndexes but not when using CompactMapRangeIndexes.
> > > There's only 3 test cases that are failing and I still need to investigate.
> >
> >
> > The cases that are failing try to create the following index:
> > pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID]
> > They also fail if the index is: pf.collectionHolderMap[(pf.ID).toString()]
> > I wonder what type of index is that. According to the documentation, when you create a Map index you specify either "*" in order to create an index for every key or one or more keys (v.g. "SUN", IBM",...) but in those cases the index contains something variable.
> > Does it make sense to support it given that there is no such thing in the documentation?
>
> pf.collectionHolderMap[(pf.ID).toString()] <= I think(not 100% sure) this can be considered equivalent to entry.mapField["ID value as key"] So similar to a map index
>
> pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID] <= I think is a map index where the value that is mapped happens to be an array. For example "SELECT * FROM /exampleRegion p WHERE p.collectionHolderMap.get('1').arr[0] = '0'"
>
> This query is listed in the doc here: https://gemfire.docs.pivotal.io/96/geode/getting_started/querying_quick_reference.html
> Although it probably discussed in detail in the indexing portion. I would guess the docs got too cluttered with all the types of queries that are possible with OQL (as one can continue to drill down into objects infinitely)
The query makes perfect sense to me. What is not that clear is what type of index is created because neither a fixed value nor "*" is used as a key to the map but rather a variable. That leads me to think that it is not a Map index.
I have made a last change (see last commit) that treats these indexes as regular indexes, not Map indexes, and all test cases seem to pass.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
jhuynh1 commented on pull request #6028:
URL: https://github.com/apache/geode/pull/6028#issuecomment-789968684
> > > Usually the queries that don't have indexes are considered "correct" and the ones with indexes usually were checked against those results.
> > > Was the issue due to the map index not mapping values it didn't know about?
> > > A solution down the line might be to allow custom indexing.
> > > Is there a way to disable the != only for queries that use the map index? Queries that use the base compact range index should still be able to execute != in a timely fashion.
> >
> >
> > I created the test in order to compare queries with and without indexes. Nevertheless, I think that currently queries without indexes do not give the right results with where clauses in which the map for the key passed is compared with null, for example: "positions['SUN'] != null" or "positions['SUN'] = null. In these cases for entries that contain the map (positions != null) but positions does not contain the "SUN" key, it considers that positions['SUN'] is null.
> > I have changed the code in the latest commits to work the way I expected.
> > Besides, I have added a change so that indexes are used again when using CompactRangeIndexes but not when using CompactMapRangeIndexes.
> > There's only 3 test cases that are failing and I still need to investigate.
>
> The cases that are failing try to create the following index:
> pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID]
> They also fail if the index is: pf.collectionHolderMap[(pf.ID).toString()]
>
> I wonder what type of index is that. According to the documentation, when you create a Map index you specify either "*" in order to create an index for every key or one or more keys (v.g. "SUN", IBM",...) but in those cases the index contains something variable.
> Does it make sense to support it given that there is no such thing in the documentation?
pf.collectionHolderMap[(pf.ID).toString()] <= I think(not 100% sure) this can be considered equivalent to entry.mapField["ID value as key"] So similar to a map index
pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID] <= I think is a map index where the value that is mapped happens to be an array. For example "SELECT * FROM /exampleRegion p WHERE p.collectionHolderMap.get('1').arr[0] = '0'"
This query is listed in the doc here: https://gemfire.docs.pivotal.io/96/geode/getting_started/querying_quick_reference.html
Although it probably discussed in detail in the indexing portion. I would guess the docs got too cluttered with all the types of queries that are possible with OQL (as one can continue to drill down into objects infinitely)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jhuynh1 commented on a change in pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
jhuynh1 commented on a change in pull request #6028:
URL: https://github.com/apache/geode/pull/6028#discussion_r574857948
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -1506,7 +1506,8 @@ private void applyProjectionForIndexInit(List currentRuntimeIters)
: modifiedIndexExpr.evaluate(this.initContext);
if (indexKey == null) {
- indexKey = IndexManager.NULL;
+ // indexKey = IndexManager.NULL;
Review comment:
I don't think we can do this without losing some capabilities... such as != queries. This line of code also affects regular ranged indexes and not just map indexes.
I think we'd have to find some way for the index to supply the correct entries for those types of queries. Or find a better way to store the entries in the index.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jhuynh1 commented on a change in pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
jhuynh1 commented on a change in pull request #6028:
URL: https://github.com/apache/geode/pull/6028#discussion_r578786729
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledIndexOperation.java
##########
@@ -104,7 +105,13 @@ public Object evaluate(ExecutionContext context) throws TypeMismatchException,
}
if (rcvr instanceof Map) {
- return ((Map) rcvr).get(index);
+ if (context instanceof QueryExecutionContext) {
Review comment:
So when we execute a query and the index is a map index... if the key is not present in the map, we could get an actual "null" back. When we were adding the NULL key as a collection into the map, we were possibly getting a valid collection?
I'm surprised a bit that the != queries work but I haven't walked through this code in a while. SInce the tests are passing, that's great. I would assume the != queries will now be doing a full region crawl... if that's the case, they will be a lot slower.. not sure if that's the case, but if it is, it might not be acceptable => might need to convince the dev list on this one...
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledIndexOperation.java
##########
@@ -104,7 +105,13 @@ public Object evaluate(ExecutionContext context) throws TypeMismatchException,
}
if (rcvr instanceof Map) {
- return ((Map) rcvr).get(index);
+ if (context instanceof QueryExecutionContext) {
+ return ((Map) rcvr).get(index);
+ }
+ if (((Map<?, ?>) rcvr).containsKey(index)) {
Review comment:
Both if statements do the same thing, maybe add an || to the previous if call, but I am ok with keeping them separate.
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -1555,6 +1555,9 @@ private void applyProjection(boolean add, ExecutionContext context)
throws FunctionDomainException, TypeMismatchException, NameResolutionException,
QueryInvocationTargetException, IMQException {
Object indexKey = indexedExpr.evaluate(context);
+ if (IndexManager.NULL.equals(indexKey)) {
+ return;
+ }
if (indexKey == null) {
Review comment:
Should this still be present? wouldn't this case be "iimpossible now that we added the three lines above?"
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6028:
URL: https://github.com/apache/geode/pull/6028#issuecomment-791335022
> > > > > Usually the queries that don't have indexes are considered "correct" and the ones with indexes usually were checked against those results.
> > > > > Was the issue due to the map index not mapping values it didn't know about?
> > > > > A solution down the line might be to allow custom indexing.
> > > > > Is there a way to disable the != only for queries that use the map index? Queries that use the base compact range index should still be able to execute != in a timely fashion.
> > > >
> > > >
> > > > I created the test in order to compare queries with and without indexes. Nevertheless, I think that currently queries without indexes do not give the right results with where clauses in which the map for the key passed is compared with null, for example: "positions['SUN'] != null" or "positions['SUN'] = null. In these cases for entries that contain the map (positions != null) but positions does not contain the "SUN" key, it considers that positions['SUN'] is null.
> > > > I have changed the code in the latest commits to work the way I expected.
> > > > Besides, I have added a change so that indexes are used again when using CompactRangeIndexes but not when using CompactMapRangeIndexes.
> > > > There's only 3 test cases that are failing and I still need to investigate.
> > >
> > >
> > > The cases that are failing try to create the following index:
> > > pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID]
> > > They also fail if the index is: pf.collectionHolderMap[(pf.ID).toString()]
> > > I wonder what type of index is that. According to the documentation, when you create a Map index you specify either "*" in order to create an index for every key or one or more keys (v.g. "SUN", IBM",...) but in those cases the index contains something variable.
> > > Does it make sense to support it given that there is no such thing in the documentation?
> >
> >
> > pf.collectionHolderMap[(pf.ID).toString()] <= I think(not 100% sure) this can be considered equivalent to entry.mapField["ID value as key"] So similar to a map index
> > pf.collectionHolderMap[(pf.ID).toString()].arr[pf.ID] <= I think is a map index where the value that is mapped happens to be an array. For example "SELECT * FROM /exampleRegion p WHERE p.collectionHolderMap.get('1').arr[0] = '0'"
> > This query is listed in the doc here: https://gemfire.docs.pivotal.io/96/geode/getting_started/querying_quick_reference.html
> > Although it probably discussed in detail in the indexing portion. I would guess the docs got too cluttered with all the types of queries that are possible with OQL (as one can continue to drill down into objects infinitely)
>
> The query makes perfect sense to me. What is not that clear is what type of index is created because neither a fixed value nor "*" is used as a key to the map but rather a variable. That leads me to think that it is not a Map index.
> I have made a last change (see last commit) that treats these indexes as regular indexes, not Map indexes, and all test cases seem to pass.
I have created a JIRA (https://issues.apache.org/jira/browse/GEODE-9004) and a PR (https://github.com/apache/geode/pull/6096) to address the issues found with a proposal of changes to be made.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a change in pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #6028:
URL: https://github.com/apache/geode/pull/6028#discussion_r578981283
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -1555,6 +1555,9 @@ private void applyProjection(boolean add, ExecutionContext context)
throws FunctionDomainException, TypeMismatchException, NameResolutionException,
QueryInvocationTargetException, IMQException {
Object indexKey = indexedExpr.evaluate(context);
+ if (IndexManager.NULL.equals(indexKey)) {
+ return;
+ }
if (indexKey == null) {
Review comment:
Yes. This is left for the case when we are adding an entry (we are not querying) and the map does not contain the key (line 114 in CompiledIndexOperation). In that case, we don't add the index.
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -1555,6 +1555,9 @@ private void applyProjection(boolean add, ExecutionContext context)
throws FunctionDomainException, TypeMismatchException, NameResolutionException,
QueryInvocationTargetException, IMQException {
Object indexKey = indexedExpr.evaluate(context);
+ if (IndexManager.NULL.equals(indexKey)) {
+ return;
+ }
if (indexKey == null) {
Review comment:
Yes. This is left for the case when we are adding an entry (we are not querying) and the map does not contain the key (line 114 in CompiledIndexOperation.java). In that case, we don't add the index.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a change in pull request #6028: WIP, DO NOT REVIEW. Do not create index when indexKey is null
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #6028:
URL: https://github.com/apache/geode/pull/6028#discussion_r579429969
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledIndexOperation.java
##########
@@ -104,7 +105,13 @@ public Object evaluate(ExecutionContext context) throws TypeMismatchException,
}
if (rcvr instanceof Map) {
- return ((Map) rcvr).get(index);
+ if (context instanceof QueryExecutionContext) {
Review comment:
I have pushed a new commit with some test cases to compare the results of queries with the indexes and without them.
As you can see, the tests when indexes are used fail.
The funny thing is that these test cases also fail if run against the latest code in development. So I think there is something not working right with the indexes, with or without my change.
----------------------------------------------------------------
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:
users@infra.apache.org