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

JTR-143-funciones-de-agrupado #21

Merged
merged 2 commits into from May 17, 2022
Merged

Conversation

Nataniel4
Copy link
Contributor

LINK AL TICKET
https://fizzmod.atlassian.net/browse/JTR-143

LINK A LA HISTORIA
https://fizzmod.atlassian.net/browse/JTR-142

DESCRIPCIÓN DEL REQUERIMIENTO

DESCRIPCIÓN DE LA SOLUCIÓN

  • Se agregaron dos metodos:
    • group: Para agrupar resultados por campo o campos
    • facet: Para obtener el conteo de items por campo, o campos combinados

CÓMO SE PUEDE PROBAR
⚠️ Esto ya fue super probado en un Solr corriendo en local, se puede hacer lo mismo si aun asi queres probarlo.
imagen

Eso es todo amigos 🚪🏃

```

### ***async*** `facet(model, [parameters])`
Get facet counts by field or pivot

Choose a reason for hiding this comment

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

Sería bueno tener una pequeña definición de pivot, porque no queda muy clara a simple vista la diferencia con field como para poder determinar cuál se usa

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lo explique lo mejor que pude con los ejemplos y con como devuelve los datos segun como envies ese param, no pude encontrar mejor manera de explicarlo
image
image

@@ -57,6 +57,60 @@ class Query {
};
}

static facet(params) {

const {

Choose a reason for hiding this comment

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

Por qué destructuras las props de params acá en una variable y no directamente en el parámetro cuando recibes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quedo así porque algunas propiedades las saca desde el params porque crea variables con el mismo nombre pero segun lo que haya en params

if(pivot && typeof pivot !== 'string' && !Array.isArray(pivot))
throw new SolrError(`Facet pivot must be a string or array, received: ${typeof pivot}.`, SolrError.codes.INVALID_PARAMETERS);

const filters = params.filters ? { filter: Filters.build(params.filters, fields) } : {};

Choose a reason for hiding this comment

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

Puedes destructurar filters y order en la variable de más arriba (o en los parámetros del método) para usarla directamente como las demás props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Si, lo probe pero no me gusto como quedaba porque igual tengo que crear las variables filters y order y seria mas confuso el codigo 😭

Copy link

@mdanelutti mdanelutti May 4, 2022

Choose a reason for hiding this comment

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

retruco un poco lo que comenta Karen por un lado sumar order y filters al destructuring y omitir instanciar filters y order, y hacerlo directo en el objeto del return

return {
	query: '*:*',
	offset: (page * limit) - limit,
	limit,
	params: {
		facet: true,
		...field && { 'facet.field': field },
		...pivot && { 'facet.pivot': pivot }
	},
	...filters && { filter: Filters.build(filters, fields) },
	...order && { sort: this._getSorting(order) }
};

esta lógica puede aplicar de igual forma para group()

Opción 2: tener un método que genere filters y order entonces lo que es Filters.build() y this._getSorting() lo tenes en un solo lugar, terminando en algo así como, ...orderAndFilter(params) en el return


groups.forEach(({ groupValue, doclist }) => {
formattedGroups[field].groups[groupValue] = { count: doclist.numFound, items: this.format(doclist.docs) };
});

Choose a reason for hiding this comment

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

acá un opcional, podes hacer que sea un reduce y asignarlo directo en groups

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me parecio re mercenario meter un reduce dentro de un reduce si igual tengo que asignar la data al acumulador del reduce principal 😭

lib/solr.js Outdated

const endpoint = Endpoint.create(Endpoint.presets.get, this.url, this.core);

const page = params.page || 1;

Choose a reason for hiding this comment

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

si tenemos DEFAULT_LIMIT y DEFAULT_GROUP_LIMIT, por que no tenemos DEFAULT_START_PAGE? 😜

Copy link

@mdanelutti mdanelutti left a comment

Choose a reason for hiding this comment

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

hay dos cosas opcionales y un cambio mínimo

@Nataniel4 Nataniel4 requested a review from mdanelutti May 4, 2022 18:51
@juanhapes juanhapes merged commit a2e001f into master May 17, 2022
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.

None yet

4 participants