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

Fixed replacing reserved words. #722

Merged
merged 5 commits into from
Nov 16, 2014
Merged

Fixed replacing reserved words. #722

merged 5 commits into from
Nov 16, 2014

Conversation

jkbnerad
Copy link
Contributor

I solved problem with uppercase queries.

Fixed:
"TIMBERLAND Mens" was converted to "TIMBERL&& Mens"

@@ -63,7 +63,7 @@ public static function escapeTerm($term)
*/
public static function replaceBooleanWords($term)
{
$replacementMap = array('AND'=>'&&', 'OR'=>'||', 'NOT'=>'!');
$replacementMap = array(' AND '=>' && ', ' OR '=>' || ', ' NOT '=>' !');
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't be a regular expression better for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean something like

$reservedWords = array('/\sAND\s/u' => ' && ', '/\sOR\s/u' => ' || ', '/\sNOT\s/u' => ' !');
$result = preg_replace(array_keys($reservedWords), array_values($reservedWords), $term);

?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it might make no difference for ES, but I was thinking

$reservedWords = array('/\s+AND\s+/u' => ' && ', '/\s+OR\s+/u' => ' || ', '/\s+NOT\s+/u' => ' !');

Copy link
Owner

Choose a reason for hiding this comment

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

I didn't study the Regexp in detail. Would the main difference be performance or is there an other difference? After the ! no space is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin no space is needed

curl -XGET 'http://localhost:9200/_all/_search?q=hello+!kitty' is correct

I tested performance Regexp vs. strtr and differences were minimal. Regex is better for multibyte white spaces.

@fprochazka:

I prefer (w/o \s+)

$reservedWords = array('/\sAND\s/u' => ' && ', '/\sOR\s/u' => ' || ', '/\sNOT\s/u' => ' !');

because your solution (with \s+) replace prefered words & "repair" query (blue    AND red => blue && red). And it is the task for analyzer, I mean.

@ruflin
Copy link
Owner

ruflin commented Nov 13, 2014

@jkbnerad Can you also update the changes.txt?

@jkbnerad
Copy link
Contributor Author

@ruflin Yes.

ruflin added a commit that referenced this pull request Nov 16, 2014
Fixed replacing reserved words.
@ruflin ruflin merged commit 5fe627f into ruflin:master Nov 16, 2014
@ruflin
Copy link
Owner

ruflin commented Nov 16, 2014

Merged, thx.

@jkbnerad
Copy link
Contributor Author

Thank you.

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.

3 participants