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

Add Elastica\Query\Image #787

Merged
merged 2 commits into from
Feb 23, 2015
Merged

Add Elastica\Query\Image #787

merged 2 commits into from
Feb 23, 2015

Conversation

Jmoati
Copy link
Contributor

@Jmoati Jmoati commented Feb 20, 2015

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 84.44% when pulling 5f4d901 on Jmoati:master into 7f62b40 on ruflin:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 84.44% when pulling 5f4d901 on Jmoati:master into 7f62b40 on ruflin:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 84.44% when pulling 5f4d901 on Jmoati:master into 7f62b40 on ruflin:master.

@im-denisenko
Copy link
Contributor

Is this plugin alive? I see they support ES 1.1.0, and last commit was half a year ago.
PR with support for 1.4.2 was opened month ago and still has no comments.
Elastica supports ES 1.4.3 right now, how would you use it with outdated plugin?

@Jmoati
Copy link
Contributor Author

Jmoati commented Feb 20, 2015

I use https://github.com/ifgh/elasticsearch-image which is a 1.4.2 fork.
I think that it will work with 1.4.3 too.

$query->setFieldHash($field, 'BIT_SAMPLING');
$query->setFieldBoost($field, 100);

$query->setFieldImage($field, __DIR__ . '/../../../../data/test.jpg');
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx.

@ruflin
Copy link
Owner

ruflin commented Feb 20, 2015

To make sure it works with the current version, we should add an integration tests, which means we must add the plugin to our setup process. Like this we can also be sure it works with future versions.

Also we should add it then to the dependencies in the README file: https://github.com/ruflin/Elastica

@Jmoati If you need some help to get the setup done etc. we are here to help.

@im-denisenko
Copy link
Contributor

More concrete steps how to add plugin to build:

  • Add variable ES_IMAGE_PLUGIN_VER here
  • Add plugin installation here
  • Add plugin name here
  • Add plugin to table of dependencies here
  • Implement tests
  • Add BIG BOLD docblock that this query is not standard functionality of ES. Like here

@Jmoati
Copy link
Contributor Author

Jmoati commented Feb 21, 2015

I will try this as soon as i can.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.33%) to 84.73% when pulling d7ed1ef on Jmoati:master into 7f62b40 on ruflin:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 84.82% when pulling 56b5db7 on Jmoati:master into cc740b6 on ruflin:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 84.82% when pulling 56b5db7 on Jmoati:master into cc740b6 on ruflin:master.

@Jmoati
Copy link
Contributor Author

Jmoati commented Feb 23, 2015

Can you please check.

- name: install image plugin
command: >
creates=/usr/share/elasticsearch/plugins/image
/usr/share/elasticsearch/bin/plugin --url https://github.com/SibaTokyo/elasticsearch-image/releases/download/{{ ES_IMAGE_PLUGIN_VER }}/elasticsearch-image-1.4.0.zip -install image
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 "1.4.0" at the end of url also should use ES_IMAGE_PLUGIN_VER.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fixed.

@im-denisenko
Copy link
Contributor

All classes in Elastica\Query namespace can be instantiated by Elastica\QueryBuilder. You can add this query to querybuilder here (update every supported version) and here.

But this query isn't part of elasticsearch, so I'm not sure should it be added to querybuilder or not.
Guess @ruflin or @webdevsHub knows better.

@Jmoati
Copy link
Contributor Author

Jmoati commented Feb 23, 2015

Maybe, the PR can be accepted before I implement it in QueryBuilder if necessary?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 84.82% when pulling 3c867e1 on Jmoati:master into cc740b6 on ruflin:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 84.82% when pulling 3c867e1 on Jmoati:master into cc740b6 on ruflin:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 84.82% when pulling 3c867e1 on Jmoati:master into cc740b6 on ruflin:master.

ruflin added a commit that referenced this pull request Feb 23, 2015
Add Elastica\Query\Image
@ruflin ruflin merged commit b4006bd into ruflin:master Feb 23, 2015
@ruflin
Copy link
Owner

ruflin commented Feb 23, 2015

Merged. Yes we can add it to the query builder in a second step.

@Jmoati
Copy link
Contributor Author

Jmoati commented Feb 23, 2015

Thx.

@webdevsHub
Copy link
Contributor

I would prefer not to add it to Query DSL class because it is not an official elasticsearch query. Furthermore it will be difficult to update the Version classes with custom queries in the future.

However we could implement a new "plugin" DSL in QueryBuilder and put all the custom plugin stuff in there

$qb = new QueryBuilder();
$qb->plugin()->image(/*...*/);

// or
$qb->elastica()->image(/*...*/);

@ruflin
Copy link
Owner

ruflin commented Feb 24, 2015

@webdevsHub I like the "plugin DSL" idea.

@webdevsHub
Copy link
Contributor

webdevsHub commented Feb 24, 2015 via email

@ruflin
Copy link
Owner

ruflin commented Feb 25, 2015

All the plugins are listed here in the README: https://github.com/ruflin/Elastica

Geocluster and attachment are the other two.

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.

5 participants