Skip to content

Synthetic _source: support histogram field#89833

Merged
elasticsearchmachine merged 6 commits intoelastic:mainfrom
nik9000:synthetic_source_histogram
Sep 7, 2022
Merged

Synthetic _source: support histogram field#89833
elasticsearchmachine merged 6 commits intoelastic:mainfrom
nik9000:synthetic_source_histogram

Conversation

@nik9000
Copy link
Copy Markdown
Member

@nik9000 nik9000 commented Sep 6, 2022

Adds support for the histogram field type to synthetic _source.

image

@nik9000
Synthetic _source: support histogram field
469d354 
Adds support for the `histogram` field type to synthetic _source.
@nik9000 nik9000 requested a review from romseygeek September 6, 2022 16:44
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 6, 2022

Documentation preview:

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 6, 2022
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Sep 6, 2022

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

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Sep 6, 2022

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

@nik9000 nik9000 added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Sep 6, 2022
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Sep 6, 2022

Pinging @elastic/es-search (Team:Search)

@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Sep 6, 2022
@axw axw mentioned this pull request Sep 7, 2022
Closed
romseygeek
romseygeek requested changes Sep 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nik9000! I left a couple of comments

...n/analytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java
@Override
public DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf) throws IOException {
BinaryDocValues docValues = DocValues.getBinary(leafReader, fieldType().name());
if (docValues == null) {
Copy link
Copy Markdown
Contributor

@romseygeek romseygeek Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DocValues.getBinary() will return an empty impl rather than null. Call leafReader.getBinary() instead?

Copy link
Copy Markdown
Contributor

@romseygeek romseygeek Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which makes me think, do we have a base test that checks that an index with synthetic source enabled will return null here when no documents contain a given field? I think that would be helpful in detecting these sort of issues and might catch performance regressions too.

Copy link
Copy Markdown
Member Author

@nik9000 nik9000 Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We make sure that we spit out nothing but we don't detect the null. I'll have a look.

@nik9000 nik9000 requested a review from a team as a code owner September 7, 2022 15:31
@nik9000 nik9000 requested review from aleksmaus and michalpristas and removed request for a team September 7, 2022 15:31
@nik9000 nik9000 force-pushed the synthetic_source_histogram branch from fd53c00 to a90ba7c Compare September 7, 2022 15:38
@nik9000 nik9000 requested a review from romseygeek September 7, 2022 15:39
@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented Sep 7, 2022

This is ready for another round @romseygeek !

romseygeek
romseygeek approved these changes Sep 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nik9000 nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 7, 2022
@elasticsearchmachine elasticsearchmachine merged commit b667aa3 into elastic:main Sep 7, 2022
@nik9000 nik9000 deleted the synthetic_source_histogram branch September 7, 2022 16:26
@nik9000 nik9000 mentioned this pull request Sep 7, 2022
Closed
50 tasks
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Sep 8, 2022
@weizijun
Merge branch 'main' into rollup-remove-value-check
6578d6d 
* main: (175 commits)
  Fix integration test on Windows (elastic#89894)
  Avoiding the use of dynamic map keys in the cluster_formation results of the stable master health indicator (elastic#89842)
  Mute org.elasticsearch.tracing.apm.ApmIT.testCapturesTracesForHttpTraffic (elastic#89891)
  Fix typos in audit event types (elastic#89886)
  Synthetic _source: support histogram field (elastic#89833)
  [TSDB] Rename rollup public API to downsample  (elastic#89809)
  Format script values access (elastic#89780)
  [DOCS] Simplifies composite aggregation recommendation (elastic#89878)
  [DOCS] Update CCS compatibility matrix for 8.3 (elastic#88906)
  Fix memory leak when double invoking RestChannel.sendResponse (elastic#89873)
  [ML] Add processor autoscaling decider (elastic#89645)
  Update disk-usage.asciidoc (elastic#89709) (elastic#89874)
  Add allocation deciders in createComponents (elastic#89836)
  Mute flaky H3LatLonGeometryTest.testIndexPoints (elastic#89870)
  Fix typo in get-snapshot-status-api doc (elastic#89865)
  Picking master eligible node at random in the master stability health indicator (elastic#89841)
  Do not reuse the client after a disruption elastic#89815 (elastic#89866)
  [ML] Distribute trained model allocations across availability zones (elastic#89822)
  Increment clientCalledCount before onResponse (elastic#89858)
  AwaitsFix for elastic#89867
  ...
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Sep 8, 2022
@weizijun
Merge branch 'main' into rollup-use-smile-to-replace-map
ba10533 
* main: (175 commits)
  Fix integration test on Windows (elastic#89894)
  Avoiding the use of dynamic map keys in the cluster_formation results of the stable master health indicator (elastic#89842)
  Mute org.elasticsearch.tracing.apm.ApmIT.testCapturesTracesForHttpTraffic (elastic#89891)
  Fix typos in audit event types (elastic#89886)
  Synthetic _source: support histogram field (elastic#89833)
  [TSDB] Rename rollup public API to downsample  (elastic#89809)
  Format script values access (elastic#89780)
  [DOCS] Simplifies composite aggregation recommendation (elastic#89878)
  [DOCS] Update CCS compatibility matrix for 8.3 (elastic#88906)
  Fix memory leak when double invoking RestChannel.sendResponse (elastic#89873)
  [ML] Add processor autoscaling decider (elastic#89645)
  Update disk-usage.asciidoc (elastic#89709) (elastic#89874)
  Add allocation deciders in createComponents (elastic#89836)
  Mute flaky H3LatLonGeometryTest.testIndexPoints (elastic#89870)
  Fix typo in get-snapshot-status-api doc (elastic#89865)
  Picking master eligible node at random in the master stability health indicator (elastic#89841)
  Do not reuse the client after a disruption elastic#89815 (elastic#89866)
  [ML] Distribute trained model allocations across availability zones (elastic#89822)
  Increment clientCalledCount before onResponse (elastic#89858)
  AwaitsFix for elastic#89867
  ...

# Conflicts:
#	x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Sep 8, 2022
@weizijun
Merge branch 'main' into get-ilm-support-wildcard
a6a9344 
* main: (283 commits)
  Fix integration test on Windows (elastic#89894)
  Avoiding the use of dynamic map keys in the cluster_formation results of the stable master health indicator (elastic#89842)
  Mute org.elasticsearch.tracing.apm.ApmIT.testCapturesTracesForHttpTraffic (elastic#89891)
  Fix typos in audit event types (elastic#89886)
  Synthetic _source: support histogram field (elastic#89833)
  [TSDB] Rename rollup public API to downsample  (elastic#89809)
  Format script values access (elastic#89780)
  [DOCS] Simplifies composite aggregation recommendation (elastic#89878)
  [DOCS] Update CCS compatibility matrix for 8.3 (elastic#88906)
  Fix memory leak when double invoking RestChannel.sendResponse (elastic#89873)
  [ML] Add processor autoscaling decider (elastic#89645)
  Update disk-usage.asciidoc (elastic#89709) (elastic#89874)
  Add allocation deciders in createComponents (elastic#89836)
  Mute flaky H3LatLonGeometryTest.testIndexPoints (elastic#89870)
  Fix typo in get-snapshot-status-api doc (elastic#89865)
  Picking master eligible node at random in the master stability health indicator (elastic#89841)
  Do not reuse the client after a disruption elastic#89815 (elastic#89866)
  [ML] Distribute trained model allocations across availability zones (elastic#89822)
  Increment clientCalledCount before onResponse (elastic#89858)
  AwaitsFix for elastic#89867
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romseygeek romseygeek romseygeek approved these changes

@aleksmaus aleksmaus Awaiting requested review from aleksmaus aleksmaus was automatically assigned from elastic/elastic-agent-control-plane

@michalpristas michalpristas Awaiting requested review from michalpristas michalpristas was automatically assigned from elastic/elastic-agent-control-plane

Assignees

No one assigned

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types :StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Search Meta label for search team v8.5.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nik9000 @elasticsearchmachine @romseygeek