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

TSDB: Reject the nested object fields that are configured time_series_dimension #83920

Merged
merged 11 commits into from
Feb 17, 2022
Merged

TSDB: Reject the nested object fields that are configured time_series_dimension #83920

merged 11 commits into from
Feb 17, 2022

Conversation

weizijun
Copy link
Contributor

@weizijun weizijun commented Feb 15, 2022 •
edited
Loading

Test case "tsdb/05_dimension_and_metric_in_non_tsdb_index/nested dimensions" failed is because just as @nik9000 said:
Like ships passing in the night #83799 and #83837 missed each other and
then crashed together in the master branch causing failures.

The #83799 changes will cause tsdb/05_dimension_and_metric_in_non_tsdb_index/nested failed import from #83837.
The reason is that the dimension added code is move from LuceneDocument to DocumentParserContext.
The nested type will create new LuceneDocument for each child, so the single value check will passed.
Now the check is in DocumentDimensions, and DocumentDimensions is a variable in DocumentParserContext. All fields of a doc use the same DocumentParserContext object, so the multi-nested-values doc will case the multi-valued check to fail.

To pass the test, I remove the multi-nested-values.
But it may need a discuss the action of multi-nested-dimensions-values.

Here is the conclusion:
Reject time_series_dimension fields if there are any nested objects at all.

So this PR is change to implement the feature.

Closes: #83915

@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.2.0 labels Feb 15, 2022
@weizijun weizijun mentioned this pull request Feb 15, 2022
Closed
@weizijun
Copy link
Contributor Author

@nik9000 can you help to review?

@nik9000 nik9000 added the :StorageEngine/TSDB You know, for Metrics label Feb 15, 2022
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 15, 2022
@elasticmachine
Copy link
Collaborator

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

@nik9000 nik9000 self-assigned this Feb 15, 2022
@nik9000 nik9000 added >non-issue and removed Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Feb 15, 2022
@nik9000
Copy link
Member

nik9000 commented Feb 15, 2022

I just talked with a few folks and we'd talked about rejecting fields marked with time_series_dimension if there are any nested fields are all. We figured there probably is a use for them but we don't really know what it is yet. So, for now, we should disallow it and wait for someone to tell us how it should work.

Would you be interested in making that change?

@weizijun
Copy link
Contributor Author

Would you be interested in making that change?

of course yes.
Is it the feature: reject the nested fields that are configured time_series_dimension?

@nik9000
Copy link
Member

nik9000 commented Feb 15, 2022

: reject the nested fields that are configured time_series_dimension?

Reject time_series_dimension fields if there are any nested objects at all.

@weizijun
Copy link
Contributor Author

Reject time_series_dimension fields if there are any nested objects at all.

got it.

@weizijun weizijun changed the title TSDB: fix tsdb/05_dimension_and_metric_in_non_tsdb_index/nested dimensions TSDB: Reject time_series_dimension fields if there are any nested objects at all Feb 16, 2022
@weizijun weizijun changed the title TSDB: Reject time_series_dimension fields if there are any nested objects at all TSDB: Reject the nested object fields that are configured time_series_dimension Feb 16, 2022
@weizijun
Copy link
Contributor Author

hi, @nik9000 , I have finished. Do you recommend implementing this feature in this PR, or opening another PR?

@nik9000
Copy link
Member

nik9000 commented Feb 16, 2022

hi, @nik9000 , I have finished. Do you recommend implementing this feature in this PR, or opening another PR?

I'd open another PR and close this one.

@nik9000
Copy link
Member

nik9000 commented Feb 16, 2022

I'd open another PR and close this one.

I see you already renamed everything in this PR. That's fine. I prefer a new one but it isn't worth doing now that you've done it the other way. I'll review and probably merge.

nik9000
nik9000 approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine, ok to test

@nik9000
Copy link
Member

nik9000 commented Feb 16, 2022

@elasticmachine, test this please

@weizijun
Copy link
Contributor Author

I see you already renamed everything in this PR. That's fine. I prefer a new one but it isn't worth doing now that you've done it the other way. I'll review and probably merge.

Yeah, I implement it in this PR. It will fixed #83915. Thanks @nik9000!

@weizijun
Copy link
Contributor Author

@elasticmachine update branch

@nik9000
Copy link
Member

nik9000 commented Feb 16, 2022

@elasticmachine, test this please

@weizijun
fixup
f4b06e3
@weizijun
Merge branch 'fix-none-tsdb-index-dimension-tests' of github.com:weiz…
7367932 
…ijun/elasticsearch into fix-none-tsdb-index-dimension-tests

* 'fix-none-tsdb-index-dimension-tests' of github.com:weizijun/elasticsearch: (37 commits)
  [docs] Mention JDK 17 in the Contributing docs (elastic#84018)
  Fix GeoIpDownloader startup during rolling upgrade (elastic#84000)
  Script: Fields API for Dense Vector (elastic#83550)
  Move InferenceConfigUpdate under VersionedNamedWriteable (elastic#84022)
  [ML] Fix license feature test cleanup (elastic#84020)
  Replace deprecated api in artifact transforms (elastic#84015)
  QL: Add leniency option to SQL CLI (elastic#83795)
  [Stack Monitoring] add kibana_stats version alias to -mb template (elastic#83930)
  Optimize spliterator for ImmutableOpenMap (elastic#83899)
  Feature usage actions for archive (elastic#83931)
  Use latch to speedup multi feature migration test (elastic#84007)
  Make action names available in NodeClient (elastic#83919)
  [DOCS] Re-add HTTP proxy setings from elastic#82737 (elastic#84001)
  Add CI matrix configuration for snapshot BWC versions (elastic#83990)
  Update YAML Rest tests to check for product header on all responses (elastic#83290)
  TSDB: Add time series aggs cancellation (elastic#83492)
  [DOCS] Fix percolate query headings (elastic#83988)
  [DOCS] Move tip for percolate query example (elastic#83972)
  Simplify LocalExporter cleaner function to fix failing tests (elastic#83812)
  [GCE Discovery] Correcly handle large zones with 500 or more instances (elastic#83785)
  ...
nik9000
nik9000 reviewed Feb 16, 2022
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

@elasticmachine test this please

@nik9000
Copy link
Member

nik9000 commented Feb 16, 2022

@elasticmachine, test this please

weizijun
weizijun commented Feb 16, 2022
View reviewed changes
rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/20_mapping.yml
@@ -312,7 +312,7 @@ nested dimensions:
reason: message changed in 8.2.0

- do:
catch: /cannot have nested fields when index is in \[index.mode=time_series\]/
catch: /time_series_dimension can't be configured in nested field \[nested.dim\]/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this PR will change the nested failed message. the field dimension check is before tsdb nested check.

@nik9000
Copy link
Member

nik9000 commented Feb 16, 2022

@elasticmachine, test this please

@weizijun
Copy link
Contributor Author

about the org.elasticsearch.index.mapper.NestedObjectMapperTests.testNoDimensionNestedFields, in -Dtests.iters=10000 tests, it's ok now.

@nik9000
Copy link
Member

nik9000 commented Feb 16, 2022

@elasticmachine test this please

@weizijun
Copy link
Contributor Author

@nik9000 the failed bwc test is out of date, the previous bwc test is ok. I don't change the code that will affect the bwc test

@nik9000 nik9000 merged commit c84c7d4 into elastic:master Feb 17, 2022
@elasticsearchmachine
Copy link
Collaborator

@weizijun please enable the option "Allow edits and access to secrets by maintainers" on your PR. For more information, see the documentation.

@nik9000
Copy link
Member

nik9000 commented Feb 17, 2022

@weizijun please enable the option "Allow edits and access to secrets by maintainers" on your PR. For more information, see the documentation.

This seems like an error.

@nik9000
Copy link
Member

nik9000 commented Feb 17, 2022

This seems like an error.

@weizijun do you recall if you'd enabled "maintainer can push to this branch"? I think for you it's always true. We're just trying to figure out what's up with our tools.

@weizijun
Copy link
Contributor Author

@weizijun do you recall if you'd enabled "maintainer can push to this branch"? I think for you it's always true. We're just trying to figure out what's up with our tools.

I don't change it, but I see the flag is disabled. I will notice it next time.

@nik9000
Copy link
Member

nik9000 commented Feb 17, 2022

I don't change it, but I see the flag is disabled. I will notice it next time.

Thanks! My guess is that it sets it to false once it's merged. We're going to try and set the bot to not comment on closed PRs. There really isn't any point regardless of what github's doing.

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Feb 18, 2022
@weizijun
Merge branch 'upstream/master' into rollup-index-improvement
4db3c09 
* upstream/master: (167 commits)
  Mute FrozenSearchableSnapshotsIntegTests#testCreateAndRestorePartialSearchableSnapshot
  Mute LdapSessionFactoryTests#testSslTrustIsReloaded
  Fix spotless violation from last commit
  Mute GeoGridTilerTestCase#testGeoGridSetValuesBoundingBoxes_UnboundedGeoShapeCellValues
  Small formatting clean up (elastic#84144)
  Always re-run Feature migrations which have encountered errors (elastic#83918)
  [DOCS] Clarify `orientation` usage for WKT and GeoJSON polygons (elastic#84025)
  Group field caps response by index mapping hash (elastic#83494)
  Shrink join queries in slow log (elastic#83914)
  TSDB: Reject the nested object fields that are configured time_series_dimension (elastic#83920)
  [DOCS] Remove note about partial response from Bulk API docs (elastic#84053)
  Allow regular data streams to be migrated to tsdb data streams. (elastic#83843)
  [DOCS] Fix `ignore_unavailable` parameter definition (elastic#84071)
  Make Metadata extend AbstractCollection (elastic#83791)
  Add API specs for OpenID Connect APIs
  Revert "Clean up for superuser role name references (elastic#83627)" (elastic#84096)
  Update Lucene analysis base url (elastic#84094)
  Avoid null threadContext in ResultDeduplicator (elastic#84093)
  Use static empty store files metadata (elastic#84034)
  Preserve context in snapshotDeletionListeners (elastic#84089)
  ...

# Conflicts:
#	x-pack/plugin/rollup/build.gradle
probakowski pushed a commit to probakowski/elasticsearch that referenced this pull request Feb 23, 2022
@weizijun @probakowski
TSDB: Reject the nested object fields that are configured time_series…
ec2ecc9 
…_dimension (elastic#83920)

At the moment we really don't know what configuring a
`time_series_dimension` should *do* when there are nested documents.
So, for now, we're going to disable it. One day when someone has a good
idea of how it should work we can build that. But for now we don't want
to guess wrong and then lock us into some annoying behavior that no one
needs but we have to support for backwards compatibility reasons.

Closes: elastic#83915
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees

@nik9000 nik9000

Labels
external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue :StorageEngine/TSDB You know, for Metrics v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tsdb/05_dimension_and_metric_in_non_tsdb_index/nested dimensions
4 participants
@weizijun @elasticmachine @nik9000 @elasticsearchmachine