Closed dclemenzi closed 5 years ago
I think it's okay as a workaround and you won't lose anything. The default MATCH_ALL nativeQueryBuilder will result in the query builder derived from your OGC filter being used anyway (see here).
But it seems like a bug on the plugin side. Looking at it again I think it's there just to support GeoTools NATURAL/REVERSE_ORDER (specifically these tests). Given this it seems that really where it's currently checking isSort it should be checking "isFidSort", with the implementation being to check specifically whether sort is SortBy.NATURAL_ORDER
or SortBy.REVERSE_ORDER
.
Appreciate the quick response.
Based on the code it does not look like it derives from the query? The boolean check is using getNativeQueryBuilder() not getQueryBuilder().
Still not sure why when sorting on _uid this would cause the query to fail in the first place. This particular query does not return a significant number of results which would imply that the sorting is being applied before the filter? But this behavior is not consistent with sorting on other fields which work. This is probably more of an ElasticSearch issue or an issue with how we have it configured and outside the scope of this plugin.
FYI the _uid and _type fields are deprecated in version 6.0.0 and are being removed in version 7.0.0 so this will probably need to change when updating this plugin to the latest version. Tried changing it to _id but that resulted in this error:
{"error":{"root_cause":[{"type":"illegal_argument_exception","reason":"Fielddata is not supported on field [_id] of type [_id]"}],"type":"search_phase_execution_exception","reason":"all shards failed","phase":"query","grouped":true,"failed_shards":[{"shard":0,"index":"coalesce-linkages","node":"yR7p1MvzSFuB4yOHb-Wzxg","reason":{"type":"illegal_argument_exception","reason":"Fielddata is not supported on field [_id] of type [_id]"}}]},"status":400}
We are running version 5.4 so maybe this would work in 6+?
So my workaround for this issue still works but now that I am using the hints for aggregations and highlighting I would prefer to pass-through the hints from the client without making any modifications. Is there any reason why this check is using the nativeQueryBuilder instead of the queryBuilder? Switching this resolves the issue and it looks like its the only thing nativeQueryBuilder is used for. Setting the "q" hint it modifies the nativeQueryBuilder in FilterToElastic which is why the workaround works.
It's not obvious to me why it's nativeQueryBuilder instead of queryBuilder. Do tests still pass if you make the switch?
It seems like the better solution might be to remove the _uid block here and instead add the logic in as a condition here, something like below. I can try to test this out unless you get to it first.
...
} else if (sort == SortBy.NATURAL_ORDER || sort == SortBy.REVERSE_ORDER) {
searchRequest.addSort("_uid", naturalSortOrder);
} ...
Having issues with line 195 in ElasticFeatureSource appending _uid sorting when another SortBy property is specified.
Which creates this curl command:
On a (364 Million) database this is causing the following exception:
Removing the _uid sort resolves the issue and returns the expected results. Looking through the code I figured out that we could bypass this logic by adding the following hint onto my query:
Is this the correct way? Is there a better way? Am I loosing anything by replacing the nativeQueryBuilder?