Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Geo: Proposal for refactoring out ShapeBuilder hierarchy #40908

Closed
2 of 3 tasks
imotov opened this issue Apr 5, 2019 · 8 comments
Closed
2 of 3 tasks

Geo: Proposal for refactoring out ShapeBuilder hierarchy #40908

imotov opened this issue Apr 5, 2019 · 8 comments
Assignees
@iverase
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes Meta Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@imotov
Copy link
Contributor

imotov commented Apr 5, 2019 •
edited
Loading

At the moment shapeBuilders perform much more functionality than just shape building. We have serialization, parsing logic plus logic necessary for handling dateline transformation and conversion of shapes for lucene indexing in both libs/geo and S4J formats. As we are removing support for the S4J shapes, I would like to propose refactoring ShapeBuilder set of classes.

In #36477 we introduced a hierarchy of geo classes that can replace the hierarchy build with ShapeBuilder as a root. This hierarchy has pretty much all functionality except GeoJson parser and logic that is needed to convert shapes into the format consumable by Lucene, if we could add the GeoJson parser to libs/geo, and extract the logic we can remove the entire ShapeBuilder hierarchy and significantly simplify the geo processing pipeline structure. I think it will also simplify switching to different reference system, since it will allows us to have the reference system-specific logic separate from the geo hierarchy.

It will also remove the issue that we are facing in geosql at the moment. The only way to retrieve a shape in geo at the moment is through theShapeParser.parse(....).buildGeometry() path, which converts some shapes into somewhat equivalent set of shapes for lucene consumption, among other things it changes the order of coordinates of polygons, splits polygons on dateline, simplifies collections, etc. This can be really confusing for geosql users, since they will get shapes different from what they put into elasticsearch.

To summarize the following actions will be required:
@imotov imotov added :Analytics/Geo Indexing, search aggregations of geo points and shapes Meta team-discuss labels Apr 5, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

imotov added a commit to imotov/elasticsearch that referenced this issue Apr 11, 2019
@imotov
SQL: Add ST_X, ST_Y and ST_GEOMETRY_TYPE functions
c1721d3 
Adds 3 new geosql functions. The ST_Z functions as well as ST_X and
ST_Y functions for polygons are not supported because of the issues
described in elastic#40908. The suppor will be added as soon as the
ShapeBuilder issues are resolved.

Relates to elastic#29872
@imotov imotov mentioned this issue Apr 11, 2019
Merged
@talevy
Copy link
Contributor

talevy commented Apr 17, 2019 •
edited
Loading

+1

I ran into the same feeling when debugging shapes. a lot of wkt/geojson functionality is wrapped up together and depends on s4j. This constant translation of geometries always felt awkward in tests to me. Maybe these helper methods exist in other util classes that I am not aware of

imotov added a commit that referenced this issue Apr 22, 2019
@imotov
SQL: Add ST_X, ST_Y and ST_GEOMETRY_TYPE functions (#41104)
dd07273 
Adds 3 new geosql functions. The ST_Z functions as well as ST_X and
ST_Y functions for polygons are not supported because of the issues
described in #40908. This support will be added as soon as the
ShapeBuilder issues are resolved.

Relates to #29872
imotov added a commit to imotov/elasticsearch that referenced this issue Apr 26, 2019
@imotov
Geo: Add GeoJson parser to libs/geo classes
d6ec5f2 
Adds GeoJson parser for Geometry classes defined in libs/geo. This
should allow us to bypass ShapeBuilders in geosql and is the first
step in ShapeBuilder refactoring. The provided functionality should
be sufficient for geosql parsing, but additional functionality will
be required for complete parity with ShapeBuilder parser - we need
to add handling of mapper parameters in WKT parser as well. That
will be added in a follow up PR.

Relates elastic#40908
@imotov imotov mentioned this issue Apr 26, 2019
Merged
imotov added a commit that referenced this issue Apr 29, 2019
@imotov
Geo: Add GeoJson parser to libs/geo classes (#41575)
6d3fd84 
Adds GeoJson parser for Geometry classes defined in libs/geo.

Relates #40908 and #29872
imotov added a commit to imotov/elasticsearch that referenced this issue Apr 29, 2019
@imotov
Geo: Add GeoJson parser to libs/geo classes (elastic#41575)
1411e2c 
Adds GeoJson parser for Geometry classes defined in libs/geo.

Relates elastic#40908 and elastic#29872
@imotov imotov mentioned this issue Apr 29, 2019
Merged
imotov added a commit that referenced this issue Apr 29, 2019
@imotov
Geo: Add GeoJson parser to libs/geo classes (#41575) (#41657)
10ab838 
Adds GeoJson parser for Geometry classes defined in libs/geo.

Relates #40908 and #29872
akhil10x5 pushed a commit to akhil10x5/elasticsearch that referenced this issue May 2, 2019
@imotov @akhil10x5
Geo: Add GeoJson parser to libs/geo classes (elastic#41575)
8973714 
Adds GeoJson parser for Geometry classes defined in libs/geo.

Relates elastic#40908 and elastic#29872
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this issue May 27, 2019
@imotov
Geo: Add GeoJson parser to libs/geo classes (elastic#41575)
9d6369a 
Adds GeoJson parser for Geometry classes defined in libs/geo.

Relates elastic#40908 and elastic#29872
@imotov imotov self-assigned this Jun 25, 2019
imotov added a commit to imotov/elasticsearch that referenced this issue Jun 26, 2019
@imotov
Geo: Makes coordinate validator in libs/geo plugable
3449c22 
Moves coordinate validation from Geometry constructors into
parser.

Relates elastic#40908
This was referenced Jul 5, 2019
Open
Merged
imotov added a commit that referenced this issue Jul 16, 2019
@imotov
Geo: extract dateline handling logic from ShapeBuilders (#44187)
33ad792 
Extracts dateline decomposition logic from ShapeBuilder into a separate
utility class that is used on the indexing side. The search side
will be handled as part of another PR at this time we will remove
the decomposition logic from ShapeBuilders as well. This PR also doesn't
change any existing logic including bugs.

Relates to #40908
michalperlak pushed a commit to michalperlak/elasticsearch that referenced this issue Jul 16, 2019
@imotov @michalperlak
Geo: extract dateline handling logic from ShapeBuilders (elastic#44187)
130e248 
Extracts dateline decomposition logic from ShapeBuilder into a separate
utility class that is used on the indexing side. The search side
will be handled as part of another PR at this time we will remove
the decomposition logic from ShapeBuilders as well. This PR also doesn't
change any existing logic including bugs.

Relates to elastic#40908
imotov added a commit that referenced this issue Jul 17, 2019
@imotov
Geo: extract dateline handling logic from ShapeBuilders (#44187)
cd5a334 
Extracts dateline decomposition logic from ShapeBuilder into a separate
utility class that is used on the indexing side. The search side
will be handled as part of another PR at this time we will remove
the decomposition logic from ShapeBuilders as well. This PR also doesn't
change any existing logic including bugs.

Relates to #40908
imotov added a commit to imotov/elasticsearch that referenced this issue Jul 22, 2019
@imotov
Geo: deprecate ShapeBuilder in QueryBuilders
0355d0c 
Removes unnecessary now timeline decompositions from shape builders
and deprecates ShapeBuilders in QueryBuilder in favor of libs/geo
shapes.

Relates to elastic#40908
@imotov imotov mentioned this issue Jul 22, 2019
Merged
@nknize nknize mentioned this issue Jul 24, 2019
Merged
imotov added a commit that referenced this issue Jul 24, 2019
@imotov
Geo: deprecate ShapeBuilder in QueryBuilders (#44715)
34675ca 
Removes unnecessary now timeline decompositions from shape builders
and deprecates ShapeBuilders in QueryBuilder in favor of libs/geo
shapes.

Relates to #40908
imotov added a commit that referenced this issue Jul 24, 2019
@imotov
Geo: deprecate ShapeBuilder in QueryBuilders (#44715)
f9943a3 
Removes unnecessary now timeline decompositions from shape builders
and deprecates ShapeBuilders in QueryBuilder in favor of libs/geo
shapes.

Relates to #40908
@nknize nknize mentioned this issue Jul 25, 2019
Closed
polyfractal pushed a commit to polyfractal/elasticsearch that referenced this issue Jul 29, 2019
@imotov @polyfractal
Geo: extract dateline handling logic from ShapeBuilders (elastic#44187)
5a35b67 
Extracts dateline decomposition logic from ShapeBuilder into a separate
utility class that is used on the indexing side. The search side
will be handled as part of another PR at this time we will remove
the decomposition logic from ShapeBuilders as well. This PR also doesn't
change any existing logic including bugs.

Relates to elastic#40908
polyfractal pushed a commit to polyfractal/elasticsearch that referenced this issue Jul 29, 2019
@imotov @polyfractal
Geo: deprecate ShapeBuilder in QueryBuilders (elastic#44715)
a704811 
Removes unnecessary now timeline decompositions from shape builders
and deprecates ShapeBuilders in QueryBuilder in favor of libs/geo
shapes.

Relates to elastic#40908
@rjernst rjernst added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 4, 2020
@imotov imotov removed their assignment May 5, 2020
@iverase
Copy link
Contributor

iverase commented Oct 14, 2021

@imotov in #77856 we moved all ShapeBuilder classes to a module together with the Spatial4j/JTS dependency, are we ok to close this issue?

@imotov
Copy link
Contributor Author

imotov commented Oct 14, 2021

Where do we track removal of this module?

@iverase
Copy link
Contributor

iverase commented Oct 15, 2021

Shall we open a new issue just for that with more specific objective?. I found the title of this issue misleading as the refactoring is done.

@imotov
Copy link
Contributor Author

imotov commented Oct 15, 2021

Yeah, I am ok with closing this one in favor of a more specific issue

@elasticsearchmachine
Copy link
Collaborator

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

@iverase
Copy link
Contributor

iverase commented May 15, 2023

closing in favour of #96097

@iverase iverase closed this as completed May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@iverase iverase

Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes Meta Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@rjernst @talevy @imotov @elasticmachine @iverase @elasticsearchmachine