Skip to content

[TASK] Querybuilder refactoring#91

Merged
skurfuerst merged 5 commits intoFlowpack:masterfrom
daniellienert:querybuilder-refactoring
Sep 24, 2015
Merged

[TASK] Querybuilder refactoring#91
skurfuerst merged 5 commits intoFlowpack:masterfrom
daniellienert:querybuilder-refactoring

Conversation

@daniellienert
Copy link
Copy Markdown
Contributor

Refactor the queryBuilder and the result object as a basis for further extensions and improvements.
ElasticSearchQueryBuilder::fetch() now returns an array including the nodes and the other meta data results. The ElasticSearchQueryResult still provides the nodes through its ArrayAccess implementation but also returns additional data like aggregations and suggestions through specific getters.

Also added a first functional test. All tests are failing right now due to existing bugs.

Jira: IN-4

To further extend the result with aggregation and suggestion meta data,
the QueryBuilder is refactored to not only return the nodes but
an array containing different elasticsearch result parts
@daniellienert daniellienert changed the title Querybuilder refactoring [TASK] Querybuilder refactoring Sep 24, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

any reason why we convert the result to a different format and not just say $this->result = $hits?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It should rather be $this->result = $treatedContent as we want to return the complete result including the e.g. aggregates. But that would fine.

@skurfuerst
Copy link
Copy Markdown
Contributor

Hey Daniel,

See my nitpick :-) It would be great to have a small clarification whether the "array-based API" should be public or not; I suppose it should NOT be public. Correct?

All the best,
Sebastian

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing type..

@aertmann
Copy link
Copy Markdown
Contributor

The direction looks good. As we discussed yesterday I'd still like to make it easier to achieve two things.

Getting the meta result, which is currently not possible as I see it. You can only get certain things like aggregations. Having methods for specific meta results could be okay, however there should be a way to get the whole raw result and then it might not make sense having convenience methods like getAggregations but just use getResult()['aggregations'](the treatedContent from the ES result).

@aertmann
Copy link
Copy Markdown
Contributor

Addendum on last comment, discussed with Daniel and since ES only seems to have two special return keys aggregations and suggest it can be okay to have convenience methods for those, however internally I'd still suggest to not make that explicit and store the whole result and use that instead.

skurfuerst added a commit that referenced this pull request Sep 24, 2015
@skurfuerst skurfuerst merged commit b3cbc65 into Flowpack:master Sep 24, 2015
@daniellienert daniellienert deleted the querybuilder-refactoring branch September 24, 2015 12:47
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