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.

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

Restore support for the include/pattern syntax. #23140

Merged
merged 4 commits into from
Feb 15, 2017

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Feb 13, 2017

Closes #22933

@jpountz
Copy link
Contributor Author

jpountz commented Feb 14, 2017

@markharwood Would you mind reviewing this PR?

@markharwood
Copy link
Contributor

Just tried this regex which works OK:

GET test/_search
{
	"size":0,
	"aggs":{
		"terms":{
			"terms":{
				"field":"foo.keyword",
				"include":"b.*",                
				"exclude":"a.*"				
			}
		}
	}
}

.. but this equivalent syntax fails to parse:

GET test/_search
{
	"size":0,
	"aggs":{
		"terms":{
			"terms":{
				"field":"foo.keyword",
				"include":{
					"pattern":"ba.*"                    
				},
				"exclude":{ 
					"pattern":"a.*"
				}
			}
		}
	}
}

@jpountz
Copy link
Contributor Author

jpountz commented Feb 14, 2017

Thanks @markharwood I should have been more surprised that tests passed on the first time that I ran them, I probably ran gradle from the wrong directory, tests were completely broken! :) I just pushed a new commit that should fix it.

@@ -120,6 +121,9 @@ public static IncludeExclude parseInclude(XContentParser parser, QueryParseConte
"Unknown parameter in Include/Exclude clause: " + currentFieldName);
}
}
if (pattern != null) {
return new IncludeExclude(pattern, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe throw an exception here if partition criteria supplied?
Partitioning not supported when using regex-based filtering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

@jpountz
Copy link
Contributor Author

jpountz commented Feb 15, 2017

@markharwood Can you give it another look?

@markharwood
Copy link
Contributor

LGTM

@jpountz jpountz merged commit 1b2cc16 into elastic:5.x Feb 15, 2017
@jpountz jpountz deleted the fix/include_pattern branch February 15, 2017 13:55
jpountz added a commit that referenced this pull request Feb 15, 2017
jpountz added a commit that referenced this pull request Feb 20, 2017
@jpountz jpountz added the v5.2.2 label Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants