Implement synthetic source support for range fields #107081

lkts · 2024-04-04T00:29:45Z

This PR adds basic synthetic source support for range fields. There are
following notable properties of synthetic source produced:

Ranges are always normalized to be inclusive on both ends (this is how
they are stored).
Duplicates are removed (same as for other fields).
Original order of ranges is not preserved. Ranges (except IP ranges) are always sorted in order of from and to values. For IP ranges order is unspecified.
IP ranges are always expressed as a range of IPs while it could
have been originally provided as a CIDR.

This PR only implements retrieval of data for source reconstruction from
doc values similar to fields already supporting synthetic source.

Contributes to #106460.

This PR adds basic synthetic source support for range fields. There are following notable properties of synthetic source produced: * Ranges are always normalized to be inclusive on both ends (this is how they are stored). * Original order of ranges is not preserved. * Date ranges are always expressed in epoch millis, format is not preserved. * IP ranges are always expressed as a range of IPs while it could have been originally provided as a CIDR. This PR only implements retrieval of data for source reconstruction from doc values.

elasticsearchmachine · 2024-04-04T00:31:56Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

elasticsearchmachine · 2024-04-04T00:31:57Z

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

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

lkts · 2024-04-05T17:08:33Z

@elasticmachine update branch

docs/reference/mapping/types/range.asciidoc

lkts · 2024-04-05T22:56:44Z

This is ready for review.

docs/reference/mapping/types/range.asciidoc

salvatore-campagna · 2024-04-08T08:00:44Z

docs/reference/mapping/types/range.asciidoc

+    "lte": 299
+  }
+}
+----


Maybe we should add this to the list of limitations for synthetic source? docs/reference/mapping/fields/synthetic-source.asciidoc

This is specific to ranges and i followed existing docs that have specific examples on type page (for example ip).

I am open to moving it though.

What I mean is that in the docs about synthetic source, where we provide limitations we should mention also this one, not necessarily including an example but just explaining it so that users have it together with all other limitations.

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/range/20_synthetic_source.yml

server/src/main/java/org/elasticsearch/common/network/InetAddresses.java

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

server/src/test/java/org/elasticsearch/index/mapper/IpRangeFieldMapperTests.java

salvatore-campagna · 2024-04-08T08:26:25Z

Thanks for your PR, this looks great...very comprehensive test coverage. I left a few comments.

lkts · 2024-04-08T18:04:27Z

@elasticmachine update branch

martijnvg · 2024-04-09T06:59:57Z

docs/reference/mapping/types/range.asciidoc

+configuration. Synthetic `_source` cannot be used together with
+<<copy-to,`copy_to`>> or with <<doc-values,`doc_values`>> disabled.
+
+Synthetic source always sorts values and removes duplicates for all `range` fields except `ip_range` . Ranges are sorted by their lower bound and then by upper bound. For example:


Maybe move these documented limitations to a paragraph in synthetic-source.asciidoc (at the end of the file)? I think all the limitations are mentioned there.

Or maybe summarize the limitations in synthetic-source.asciidoc and keep the extensive examples here?

We have such examples that are specific to a field type for other fields like ip, that's why i added it here.

I think we already have a general statement that order is not preserved in synthetic-source.

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

lkts · 2024-04-11T16:58:25Z

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/range/20_synthetic_source.yml

+        ip_range: { "gte": "::", "lte": "10.10.10.10" }
+  - match:
+      hits.hits.7._source:
+        ip_range: { "gte": "10.10.10.10", "lte": "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff" }


This mix of IPv6 and IPv4 is pretty unfortunate but i don't see a good way to address that. We could look at other end of range and try to format it in the same way but it's pretty brittle in my opinion.

Actually our docs say that we support mixed ranges so maybe this is okay?

I think when it come to this the semantic is something like:

apply gte to all IPv4 fields as if you only had the gte defined

apply the lte to all IPv6 fields as if you only had the lte defined

It is like saying for IPv4 fields the range is [10.10.10.10, null] while for IPv6 fields the range is [null, ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff]. Does it make sense?

Yes that would be ideal, let me see if i can do that.

lkts · 2024-04-13T02:45:58Z

@elasticmachine update branch

lkts · 2024-04-17T17:45:08Z

@elasticmachine update branch

lkts · 2024-04-17T18:27:10Z

@elasticmachine update branch

salvatore-campagna · 2024-04-23T12:18:30Z

I left a couple of comments...everything else looks good.

lkts · 2024-04-23T20:25:16Z

@elasticmachine update branch

#107910) This reverts commit acb5139 (a part of #107081). This commit impacts search behaviour for IP range fields and so needs to be reverted.

lkts requested review from martijnvg and salvatore-campagna April 4, 2024 00:29

elasticsearchmachine added v8.14.0 needs:triage Requires assignment of a team area label labels Apr 4, 2024

lkts added >feature Team:StorageEngine :StorageEngine/Mapping The storage related side of mappings labels Apr 4, 2024

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Apr 4, 2024

Update docs/changelog/107081.yaml

9c834b5

lkts commented Apr 4, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/RangeType.java Outdated Show resolved Hide resolved

lkts added 3 commits April 4, 2024 15:50

Format dates as dates, enhance tests, address todos

fe84671

Added rest tests

e15e8e3

Apply spotless

5e030e6

lkts mentioned this pull request Apr 4, 2024

Support synthetic source for all field types #106460

Open

9 tasks

Fix rest tests

0ad6361

elasticmachine and others added 4 commits April 5, 2024 19:09

Merge branch 'main' into feature/range_field_synthetic_source

00428c5

Handle copyTo same as other field types

4289996

iter

371bce9

Added documentation

09f9e9c

lkts commented Apr 5, 2024

View reviewed changes

docs/reference/mapping/types/range.asciidoc Outdated Show resolved Hide resolved

salvatore-campagna reviewed Apr 8, 2024

View reviewed changes

Merge branch 'main' into feature/range_field_synthetic_source

81746f0

martijnvg reviewed Apr 9, 2024

View reviewed changes

Fix default value tests

3fbb8c3

lkts commented Apr 11, 2024

View reviewed changes

Use record instead of tuple

9f4853f

lkts mentioned this pull request Apr 11, 2024

Allow decoding IP ranges from doc values #107376

Merged

lkts added 2 commits April 11, 2024 17:38

Tweak double and float tests

e000336

Fix date range tests

27a5328

Merge branch 'main' into feature/range_field_synthetic_source

7665635

lkts requested review from salvatore-campagna and martijnvg April 13, 2024 04:22

lkts mentioned this pull request Apr 16, 2024

Add synthetic source support for binary fields #107549

Merged

elasticsearchmachine added v8.15.0 and removed v8.14.0 labels Apr 17, 2024

elasticmachine and others added 3 commits April 17, 2024 19:45

Merge branch 'main' into feature/range_field_synthetic_source

bca218e

Don't mention copy_to in docs

2d3a9f7

Fix tests for 8.15

dea612d

Merge branch 'main' into feature/range_field_synthetic_source

2a6bbbf

elasticmachine and others added 4 commits April 23, 2024 22:25

Merge branch 'main' into feature/range_field_synthetic_source

9ddd9fe

Mention range adjustment in main synthetic source doc

a2fdc53

Format default values of IP ranges to match other range bound

acb5139

Tweak documentation

e8edaad

salvatore-campagna approved these changes Apr 24, 2024

View reviewed changes

lkts merged commit cde894a into elastic:main Apr 24, 2024
14 checks passed

lkts deleted the feature/range_field_synthetic_source branch April 24, 2024 18:33

lkts mentioned this pull request Apr 25, 2024

Revert "Format default values of IP ranges to match other range bound" #107910

Merged

lkts added a commit that referenced this pull request Apr 25, 2024

Revert "Format default values of IP ranges to match other range bound" (

4ef8b38

#107910) This reverts commit acb5139 (a part of #107081). This commit impacts search behaviour for IP range fields and so needs to be reverted.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement synthetic source support for range fields #107081

Implement synthetic source support for range fields #107081

lkts commented Apr 4, 2024 •

edited

elasticsearchmachine commented Apr 4, 2024

elasticsearchmachine commented Apr 4, 2024

lkts commented Apr 5, 2024 •

edited

lkts commented Apr 5, 2024

salvatore-campagna Apr 8, 2024

lkts Apr 13, 2024

lkts Apr 13, 2024

salvatore-campagna Apr 15, 2024

salvatore-campagna commented Apr 8, 2024

lkts commented Apr 8, 2024

martijnvg Apr 9, 2024

martijnvg Apr 9, 2024

lkts Apr 9, 2024

lkts Apr 9, 2024

lkts Apr 11, 2024

lkts Apr 11, 2024

salvatore-campagna Apr 15, 2024

lkts Apr 23, 2024

lkts commented Apr 13, 2024

lkts commented Apr 17, 2024

lkts commented Apr 17, 2024

salvatore-campagna commented Apr 23, 2024

lkts commented Apr 23, 2024

Implement synthetic source support for range fields #107081

Implement synthetic source support for range fields #107081

Conversation

lkts commented Apr 4, 2024 • edited

elasticsearchmachine commented Apr 4, 2024

elasticsearchmachine commented Apr 4, 2024

lkts commented Apr 5, 2024 • edited

lkts commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna commented Apr 8, 2024

lkts commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkts commented Apr 13, 2024

lkts commented Apr 17, 2024

lkts commented Apr 17, 2024

salvatore-campagna commented Apr 23, 2024

lkts commented Apr 23, 2024

lkts commented Apr 4, 2024 •

edited

lkts commented Apr 5, 2024 •

edited