Skip to content

Fix RegexpQueryBuilder to support an Object value#12076

Closed
alexksikes wants to merge 1 commit intoelastic:masterfrom
alexksikes:fix/regexp-builder-object-value
Closed

Fix RegexpQueryBuilder to support an Object value#12076
alexksikes wants to merge 1 commit intoelastic:masterfrom
alexksikes:fix/regexp-builder-object-value

Conversation

@alexksikes
Copy link
Copy Markdown
Contributor

The parser takes an Object value, so should the builder.

Relates to #11896

The parser takes an Object value, so should the builder.

Relates to elastic#11896
@javanna
Copy link
Copy Markdown
Contributor

javanna commented Jul 7, 2015

the change LGTM but let's wait a sec to see whether this is the way forward or not. See here

@javanna
Copy link
Copy Markdown
Contributor

javanna commented Jul 8, 2015

I discussed this PR and #12124 with @jpountz . Seems like it makes little sense (probably none) to support objects here, as only fields that are actually strings work against the regex/prefix query. It initially seemed like if the only java api was missing support for objects it should have been updated, but we are now wondering whether it would make sense to do the other way around and go back to Strings in parsers and mappers around regexp and prefix. @clintongormley do you have an opinion on this? @kimchy do you have some more history on why we moved to objects in this change back in 2012 22077d1 :) ?

@alexksikes sorry for conflicting opinions here let's see if we can sort this out now :)

@javanna
Copy link
Copy Markdown
Contributor

javanna commented Jul 9, 2015

After some discussion I think my initial idea of moving the java api builder over to supporting a string was wrong. We should rather do the other way around and move the parser etc. back to String, given that supporting objects makes very little sense. Same for #12124. Sorry @alexksikes :)

@alexksikes
Copy link
Copy Markdown
Contributor Author

I do have to agree to this as well. Accepting an Object means anything could go in, but not everything has a valid string representation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants