Skip to content

FilterPathBasedFilter support match fieldname with dot #83178

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.

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

Merged

Conversation

mushao999
Copy link
Contributor

Current FilterPathBasedFilter does not support match fieldName with dot.
This PR merges the changes of #83148 and #83152 together to add the ability of matching fieldName with dot to the FilterPathBasedFilter.

@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.1.0 labels Jan 26, 2022
@nik9000 nik9000 self-requested a review January 26, 2022 18:30
@nik9000 nik9000 self-assigned this Jan 26, 2022
@nik9000 nik9000 added the :Core/Infra/Core Core issues without another label label Jan 26, 2022
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jan 26, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@nik9000
Copy link
Member

nik9000 commented Jan 27, 2022

@elasticmachine, test this please

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.

I think you've added two bits of my other PR that you should remove. I commented on the files. I think they are the right thing to change, but I want to get them reviewed by other folks. But I can review the xcontent change. I think it fine - though I'd prefer a non-recursive solution. I added a couple other test cases, one that blows up in a recursive solution.

I see now what you are doing with prefix matching - it's for if the pattern fully matches. That makes sense to me. I added some more test cases to the abstract filtering test case thingy for that too.

@mushao999
Copy link
Contributor Author

@nik9000 I've rollback the changes you comment and add some tests for double wildcard in AbstractXContentFilteringTestCase.

@nik9000
Copy link
Member

nik9000 commented Jan 28, 2022

@elasticmachine, test this please

@nik9000
Copy link
Member

nik9000 commented Jan 28, 2022

Thanks for your changes so far! I think one of the tests is still going to fail because the solution is recursive, but otherwise I'm happy with it. Could you try running JsonXContentFilteringTests and see what that does? I thought the test I pushed would fail. At least, it did on my laptop yesterday.

@nik9000
Copy link
Member

nik9000 commented Jan 28, 2022

I talked with @mushao999 about the recursion here. He mentioned that the test that I added actually stack overflows on other code! Ooops. But he's going to zap his recursion to make me happy. I'm thankful for that.

@nik9000
Copy link
Member

nik9000 commented Jan 28, 2022

@elasticmachine, test this please

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.

For now we're going to let the recursion live. We both want to zap it, but don't have any good ideas at the moment.

@nik9000 nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 28, 2022
@elasticsearchmachine elasticsearchmachine merged commit 2c03af3 into elastic:master Jan 28, 2022
mushao999 added a commit to mushao999/elasticsearch that referenced this pull request Jan 29, 2022
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Jan 31, 2022
* upstream/master: (100 commits)
  Avoid duplicate _type fields in v7 compat layer (elastic#83239)
  Bump bundled JDK to 17.0.2+8 (elastic#83243)
  [DOCS] Correct header syntax (elastic#83275)
  Add unit tests for indices.recovery.max_bytes_per_sec default values (elastic#83261)
  [DOCS] Add note that write indices are not replicated (elastic#82997)
  Add notes on indexing to kNN search guide (elastic#83188)
  Fix get-snapshot-api :docs:integTest (elastic#83273)
  FilterPathBasedFilter support match fieldname with dot (elastic#83178)
  Fix compilation issues in example-plugins (elastic#83258)
  fix ClusterStateListener javadoc (elastic#83246)
  Speed up Building Indices Lookup in Metadata (elastic#83241)
  Mute whole suite for elastic#82502 (elastic#83252)
  Make PeerFinder log messages happier (elastic#83222)
  [Docs] Add supported _terms_enum field types (elastic#83244)
  Add an aggregator for IPv4 and IPv6 subnets (elastic#82410)
  [CI] Fix 70_time_series/default sort yaml test failures (elastic#83217)
  Update test-failure Issue Template to include "needs:triage" label elastic#83226
  Add an index->step cache to the PolicyStepsRegistry (elastic#82316)
  Improve support for joda datetime to java datetime transition in Painless (elastic#83099)
  Fix joda migration for week based methods in Painless (elastic#83232)
  ...

# Conflicts:
#	x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/TransportRollupAction.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Core Core issues without another label >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Core/Infra Meta label for core/infra team v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants