Support cartesian shape with doc values by craigtaverner · Pull Request #88487 · elastic/elasticsearch

craigtaverner · 2022-07-12T17:42:27Z

Also refactoring testing infrastructure to make testing easier to share across different test classes.

elasticmachine · 2022-07-12T17:42:31Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticsearchmachine · 2022-07-12T17:43:23Z

Hi @craigtaverner, I've created a changelog YAML for you.

iverase

This is looking pretty solid, just left some small comments.

The most important thing to improve is the test coverage of the doc_value flag in ShapeFieldMapperTests, in particular making sure that set the flag to true, it generates doc_values and that the default value changes depending on the versions.

iverase · 2022-07-13T05:54:48Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/stats/EnumCounters.java

        boolean first = true;
        for (E e : enums) {
+            if (first == false) buf.append(", ");
+            first = false;


I think this is not necessary in this PR? can we revert it?

Sure I can revert it. I wonder though, in what kind of PR would a tiny simplification like this be appropriate?

Just open a PR for it. If this optimization introduces a bug, it would be strange to be associated to this PR.

iverase · 2022-07-13T05:55:32Z

...spatial/src/main/java/org/elasticsearch/xpack/spatial/index/fielddata/CoordinateEncoder.java

 public interface CoordinateEncoder {

    CoordinateEncoder GEO = new GeoShapeCoordinateEncoder();
+    CoordinateEncoder Cartesian = new CartesianShapeCoordinateEncoder();


s/Cartesian/CARTESIAN

for consistency?

Done in e52ba26

iverase · 2022-07-13T06:00:32Z

.../spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeDocValuesQuery.java

+
+    protected abstract Component2D create(GEOMETRY geometry);
+
+    protected abstract Component2D create(GEOMETRY[] geometries);


this is nice, do we need two methods? would not be enough with something like:

protected abstract Component2D create(GEOMETRY... geometry);

That is what I did originally, but it turns out varargs does not play with generics, so in the generic class you have to use the two methods to get the same effect.

I think it is because with type erasure we cannot tell the difference between Object[] and Object (since Object[] is also an Object).

iverase · 2022-07-13T06:15:45Z

.../spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeDocValuesQuery.java

+
+    /** Override if special cases, like dateline support, should be considered */
+    protected boolean addSpecialCase(List<Component2D> components2D, GEOMETRY geometry) {
+        return false;


Not strong opinion but I wonder if we should change the API to somethng like:

protected abstract Component2D create(GEOMETRY... geometry); protected abstract void add(List<Component2D> components2D, GEOMETRY geometry);

So we don't need this special case.

Interesting approach. Is easier to read and reason about, so I've changed it to that, sort-of.

iverase · 2022-07-13T06:24:38Z

...tial/src/main/java/org/elasticsearch/xpack/spatial/index/fielddata/LabelPositionVisitor.java

 * TODO: We could instead choose the point closer to the centroid which improves unbalanced trees
 */
-public class LabelPositionVisitor implements TriangleTreeReader.Visitor {
+public class LabelPositionVisitor<T extends ToXContentFragment> implements TriangleTreeReader.Visitor {


Do we need to extend ToXContentFragment here? I think we don't need any method from that interface.

Agreed. I was just using a pattern I had used all over the place in the prototype, before moving to the common interface between GeoPoint and CartesianPoint. But in reality, many places (like this) don't care about the type at all, so I'll remove it.

iverase · 2022-07-13T06:25:30Z

server/src/main/java/org/elasticsearch/index/mapper/ShapeIndexer.java

+
+import java.util.List;
+
+public interface ShapeIndexer {


Add some java docs?

iverase · 2022-07-13T06:35:41Z

...patial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java

        });
    }

+    @Override


I am missing here explicit test for doc_values, like that they are generated when set the flag to true or that the default value is true in current version but. false in previous versions.

Added in 1066e01

iverase · 2022-07-13T06:37:40Z

...patial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java

+    @Override
+    protected boolean supportsSearchLookup() {
+        // TODO: is this really true? We have failing tests, but they look like test issues not core issues
+        return false;


It seems that this was not needed before because we did not have doc values so it was equivalent to set this to false. Now we need to do it explicitly.

iverase · 2022-07-13T06:40:26Z

...lugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/fielddata/ShapeValues.java

+ *
+ * There is just one value for one document.
+ */
+public abstract class ShapeValues<T extends ToXContentFragment> {


Do we need this class in this PR?

Good catch, indeed. I originally pulled in some of the other generic refactoring from the prototype, realised I did not need it and reverted the classes that extend this class, but forgot to remove this class itself!

Because we plan to support points separately.

…icsearch into cartesian_doc_values

I originally pulled in some of the other generic refactoring from the prototype, realised I did not need it and reverted the classes that extend this class, but forgot to remove this class itself!

No longer relevant

There were 16 tests shared between the geo and cartesian tests. These are moved into a base class. The reordering of dependencies required that we move 3 common dateline tests into a utility class.

iverase

LGTM

* upstream/master: (2974 commits) Reserved cluster state service (elastic#88527) Add transport action immutable state checks (elastic#88491) Remove suggest flag from index stats docs (elastic#85479) Polling cluster formation state for master-is-stable health indicator (elastic#88397) Add test execution guide in yamlRestTest asciidoc (elastic#88490) Add troubleshooting guide for corrupt repository (elastic#88391) [Transform] Finetune Schedule to be less noisy on retry and retry slower (elastic#88531) Updatable API keys - auto-update legacy RDs (elastic#88514) Fix typo in TransportForceMergeAction and TransportClearIndicesCacheA… (elastic#88064) Fixed NullPointerException on bulk request (elastic#88358) Avoid needless index metadata builders during reroute (elastic#88506) Set metadata on request in API key noop test (elastic#88507) Fix passing positional args to ES in Docker (elastic#88502) Improve description for task api detailed param (elastic#88493) Support cartesian shape with doc values (elastic#88487) Promote usage of Subjects in Authentication class (elastic#88494) Add CCx 2.0 feature flag (elastic#88451) Reword the watcher 'always' and 'never' condition docs (elastic#86105) Simplify azure discovery installation docs (elastic#88404) Breakup FIPS CI testing jobs ... # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java # x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

Support shape with doc values

b321e7f

Also refactoring testing infrastructure to make testing easier to share across different test classes.

craigtaverner added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes labels Jul 12, 2022

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 12, 2022

elasticsearchmachine added the v8.4.0 label Jul 12, 2022

Update docs/changelog/88487.yaml

16106e9

iverase requested changes Jul 13, 2022

View reviewed changes

craigtaverner added 5 commits July 13, 2022 11:40

Fixed test regression with dateline wrapping

77561a2

Removed points tests

36d696b

Because we plan to support points separately.

Merge branch 'cartesian_doc_values' of github.com:craigtaverner/elast…

3782bc0

…icsearch into cartesian_doc_values

Merge remote-tracking branch 'origin/master' into cartesian_doc_values

1496df3

Revert trivial cleanup as unrelated to PR

937b44e

craigtaverner mentioned this pull request Jul 13, 2022

Trivial code cleanup #88505

Merged

craigtaverner added 8 commits July 13, 2022 15:19

Capitalize CARTESIAN for symmetry with GEO

e52ba26

Simplify dateline in ShapeDocValuesQuery

c5a352d

No need for specific generic here

2c5bf3b

ShapeIndexer javadocs

6281215

Removed unused class

1d07431

I originally pulled in some of the other generic refactoring from the prototype, realised I did not need it and reverted the classes that extend this class, but forgot to remove this class itself!

Added tests for setting doc_values in ShapeFieldM.

1066e01

Removed older TODO lines

13b64f1

No longer relevant

Reduced duplication between geo and cartesian test

2ae3b4a

There were 16 tests shared between the geo and cartesian tests. These are moved into a base class. The reordering of dependencies required that we move 3 common dateline tests into a utility class.

iverase approved these changes Jul 13, 2022

View reviewed changes

iverase merged commit 6797d44 into elastic:master Jul 14, 2022

craigtaverner deleted the cartesian_doc_values branch October 20, 2023 11:44


		protected abstract Component2D create(GEOMETRY geometry);

		protected abstract Component2D create(GEOMETRY[] geometries);

Conversation

craigtaverner commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 12, 2022

Uh oh!

elasticsearchmachine commented Jul 12, 2022

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

craigtaverner commented Jul 12, 2022 •

edited

Loading