Add synthetic source support for geo_shape via fallback implementation#108881
Add synthetic source support for geo_shape via fallback implementation#108881lkts merged 4 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Hi @lkts, I've created a changelog YAML for you. |
| @Override | ||
| protected SyntheticSourceSupport syntheticSourceSupport(boolean ignoreMalformed) { | ||
| throw new AssumptionViolatedException("not supported"); | ||
| // Almost like GeoShapeType but no circles |
There was a problem hiding this comment.
Why do we skip circles? Also, shall we use GeoShapeType and skip the unsupported types in the switch?
There was a problem hiding this comment.
This field does not support circles https://www.elastic.co/guide/en/elasticsearch/reference/current/geo-shape.html#input-structure.
There was a problem hiding this comment.
I'd still use GeoShapeType and map the behavior for circle to a valid one, e.g. point. That way, this won't get out of sync if another type is added to GeoShapeType. Mostly nit.
There was a problem hiding this comment.
Let me follow-up on that.
| index: test | ||
| id: "3" | ||
| body: | ||
| shape: ["POINT (-77.03653 38.897676)", "potato", "POINT (-71.34 41.12)"] |
There was a problem hiding this comment.
Add a test were tha point is partially valid, e.g. (-71.34, 1000) ?
There was a problem hiding this comment.
👍 (basically a yml test with ignore malformed)
|
Looks good overall, adding @craigtaverner to double-check the tests. |
martijnvg
left a comment
There was a problem hiding this comment.
Left two comments. LGTM otherwise.
| } | ||
|
|
||
| @Override | ||
| protected SyntheticSourceMode syntheticSourceMode() { |
There was a problem hiding this comment.
Note that it should be relatively easy to natively support synthetic source for GeoShapeWithDocValuesFieldMapper, because this field mapper can store shapes in an accurate and more efficient format as well. We should then default the store attribute to true, like we did for other field types as well. We can do this in a follow up.
There was a problem hiding this comment.
One benefit of current implementation is that it covers ignore_malformed automatically. We'll need to add "custom" support together with such change. I'll create an issue and we can prioritize it based on usage.
modules/parent-join/build.gradle
Outdated
| restResources { | ||
| restApi { | ||
| include '_common', 'bulk', 'cluster', 'nodes', 'indices', 'index', 'search' | ||
| include '_common', 'bulk', 'cluster', 'get', 'nodes', 'indices', 'index', 'search' |
There was a problem hiding this comment.
Why is this change needed? The new yaml tests does do get api requests, but that is in a different module then this change is.
| index: test | ||
| id: "3" | ||
| body: | ||
| shape: ["POINT (-77.03653 38.897676)", "potato", "POINT (-71.34 41.12)"] |
There was a problem hiding this comment.
👍 (basically a yml test with ignore malformed)
|
buildkite test this please |
This PR enables
geo_shapemapper to use fallback synthetic source infrastructure and as such adds synthetic source support for this field type.Note that
geo_shapefields are supported by ESQL but only using source to load data. ESQL will not work forgeo_shapefields with synthetic source, tracked by #108883.