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

Type support #4

Open
qw3rtman opened this issue Jul 23, 2014 · 11 comments
Open

Type support #4

qw3rtman opened this issue Jul 23, 2014 · 11 comments

Comments

@qw3rtman
Copy link

How would one go about working with types?

@qw3rtman qw3rtman changed the title Type support? Type support Jul 23, 2014
@mmanos
Copy link
Owner

mmanos commented Jul 24, 2014

Types, as used in ElasticSearch, are not specifically supported in an effort to be compatible with other search engines. In fact, for the ElasticSearch driver, a type of 'default' is passed for all documents.

You might consider using id's that contain both the type and the id (e.g. "post-1") like shown here:
https://github.com/mmanos/laravel-search#store-extra-parameters-with-a-document

Alternatively, you could store the type as a field in the document if you need to query or retrieve it at a later point.

Any suggestions on how better to support this across multiple drivers would be appreciated.

@qw3rtman
Copy link
Author

Since types are a vital component of Elasticsearch, a package without types implemented would be much less useful than one with types implemented. Due to types being so important, I suggest you fork this package to a new repository that has support solely for Elasticsearch.

Most people only end up using one or the other and not both in conjunction. Even if they were to decide to use both at the same time, they would be unable to with this package since it seems to only support one driver at a time.

Aside from that, placing the type in the ID could end up being inefficient since more (sometimes irrelevant) IDs may be indexed inadvertently during a search query.

In regards to your suggestion about storing the type as a field in each document, I feel that would not only be redundant (as it could be solved with types), but it could also lead to ID collisions if all types share the same ID implementation. Even thought they might share the same ID, a type 'post' with an ID of 7 is much different than a type 'user' with an ID of 7.

Obviously, this would go against the idea of multiple drivers, which is why I suggest that you create a new repository for this (whether it be a fork of this one or a new one completely). I would be interested in helping out since this is honestly the only feature that is stopping me from using this package. I, however, am a bit confused as to where you are setting the type to 'default' in your code.

@mmanos
Copy link
Owner

mmanos commented Jul 24, 2014

Fair enough. My solution is not ideal if you want the full capabilities of one specific search engine.

The goal of this package was to have a familiar interface to use but allow the package to connect to the engine of their choice. For example, on a dev environment you might use the Zend search driver since it has no dependencies and writes to the local filesystem. However, on production, the Elasticsearch driver might be more appropriate.

Unfortunately, this means I had to find the common denominator between all of the drivers at the expense of some specific functionality. Though, perhaps I should consider the opposite approach of adding type support even though the Zend driver does not require it.

As for where the 'default' type is set in the code, take a look in the Elasticsearch driver file here:
https://github.com/mmanos/laravel-search/blob/master/src/Mmanos/Search/Index/Elasticsearch.php#L12

@qw3rtman
Copy link
Author

In that case, maybe you could make selecting a type optional, as you do with indices. If no type is specified, the default type is used.

Search::index('index')->type('type')->insert(...

(where type('type') sets the type for the next command)

@andrewelkins
Copy link

I like the approach on making it optional. I know I would use it.

@garygreen
Copy link

I like the idea of having a separate repository solely for Elasticsearch. The agnostic driver approach is a good idea in theory but indexing engines can be so different in their API and usage that it does creates a lot of headaches. A one size fits all approach also adds a lot of barriers for little gain of interoperability. I for one pick a indexing engine and don't expect to swap it out for another without some work involved. I think those that do that are in a real minority. I would much prefer to see a package dedicated purely to Elasticsearch implementing as much of the querying, inserting types API etc as possible without feeling restricted due to other indexing engines syntax and API.

@mmanos
Copy link
Owner

mmanos commented Nov 19, 2014

Hi Gary, good points. I agree this package needs to be made more flexible going forward, though I'm not sure I want to give up on the agnostic driver approach yet either. So far I haven't done anything really complex with search, but I could see that changing in the future.

For example, I think for my next project I want to use Algolia, as it's feature and price point seem like a good fit to start with, and I would like this package to have a driver for that.

I'm not opposed to adding type support to this package and currently I plan to do so the next time I'm making changes to this package (which should be pretty soon).

Also, don't forget about the Advanced Query Callback support. You should be able to do all sorts of driver-specific stuff with that approach, as well.

@hackel
Copy link

hackel commented May 27, 2015

This feature really is essential. Unfortunately I spent far too long with this package trying to figure out how to make it work with Elasticsearch before I realized what an issue this is, and am ready to give up. Can you imagine Eloquent requiring you to put everything into a single table? I really like this approach, and especially the eloquent syntax, but it needs types support!

@mstralka
Copy link

I semi-forked this library to implement types. I haven't put it up on GitHub yet but here's what I did:

Query.php - modified the constructor to accept an optional $type parameter:

    public function __construct(Index $index, $type = null)
    {
        $this->index = $index;
        $this->query = $this->index->newQuery($type);
    }

Modified Index.php's newQuery method:

abstract public function newQuery($type = null);

Modify ElasticsearchIndex's implementation of that method (repeat this for the other drivers too, ignoring the $type field in the implementation if they can't use it):

    public function newQuery($type = null)
    {
        $params = [
            'index' => $this->indexName,
            'body' => ['query' => ['bool' => []]],
        ];
        if ($type) {
            $params['type'] = $type;
        }
        return $params;
    }

Modify Index.php's search method to add the $type as the first parameter. You would just pass null for drivers that don't support types:

    public function search($type, $field, $value, array $options = array(), $type = null)
    {
        $query = new Query($this, $type);
        return $query->search($field, $value, $options);
    }

Hope that helps

@mstralka
Copy link

I forgot one more change in ElasticsearchIndex's runQuery function:

Change:

                $response = $this->getClient()->get(
                    [
                        'index' => array_get($query, 'index'),
                        'type' => static::$default_type,
                        'id' => array_get($query, 'id'),
                    ]
                );

to:

                $params = [
                    'index' => array_get($query, 'index'),
                    'id' => array_get($query, 'id'),
                ];
                $type = array_get($query, 'type');
                if ($type) {
                    $params['type'] = $type;
                }
                $response = $this->getClient()->get($params);

@qxhy123
Copy link

qxhy123 commented Jul 6, 2016

just code like this will resolve the type issue without fork to a new repository:
$search = new \ReflectionObject(Search::index());
$search->setStaticPropertyValue('default_type', 'goods');

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

No branches or pull requests

7 participants