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

Read_group: A New Hope #110737

Conversation

ryv-odoo
Copy link
Contributor

@ryv-odoo ryv-odoo commented Jan 23, 2023

Rationale

The method read_group() was designed to be used by the web client to efficiently compute aggregations grouped by one or more fields. However, more and more developers have been using it from the backend to make computations more efficient (avoid doing the aggregation in Python). Unfortunately, the API was designed for the web client, which adds a lot of boilerplate when used in the Python (list of dict with misleading key names).

Method _read_group() was created to improve the performance of read_group() for backend use (4ef0c00), but didn't change the API and based the implementation on read_group() itself.

This rewrites _read_group() from scratch with a new API to make it easier to use from the backend. Also, split the method to make it easy to override and add custom behavior.

New API of _read_group()

The purpose of the method is unchanged: get field aggregations specified by aggregates grouped by the given groupby fields where records are filtered by the domain.

Parameters

  • Rename fields into aggregates; parameter aggregates is a list of explicit aggregate specifications (no implicit aggregator coming from group_operator).
  • Parameter groupby becomes the second parameter (just before aggregates). It is much more readable for the returning value. Also the groupby specification for date/datetime fields should be always explicit (no default 'month').
  • New parameter having, which is a domain-like list to filter the result depending of aggregate values. It allows to simplify some code and avoid to fetch unnecessary data from the database.
  • Parameter orderby is renamed into order, to be consistent with method search(). Order terms should be an explicit aggregate specification or groupby specification (+ ASC/DESC).
  • Remove parameter lazy, which was only used for the webclient and doesn't have any sense to be used in backend.

Returned value

The returned value is a list of tuples containing, in order, the group values and aggregate values (flatten). For instance, if you group by ['foo', 'bar'] and aggregate ['baz:sum', 'quux:max'], the result will contain tuples like (foo_value, bar_value, baz_sum_value, quux_max_value).

If a group is relational field, its corresponding value will be a recordset (with a correct prefetch set that includes all returned values). For other fields, the returned values are similar to the ones you get from a record: no labels anymore, date/datetime values are not transformed strings, NULL values are returned as False, etc.

Performance

Minor performance improvements:

  • The aggregate __count generates COUNT(*) instead COUNT(<table>.id) (which generates useless null checks from PostgreSQL). Moreover, COUNT(*) is only added when we explicitly ask for __count.
  • Remove the implicit min(<table>.id) aggregate added previously to the query at each read_group.
  • The recordsets with correct prefetch set are returned for relational group values: it adds a very small cost of instantiation, but avoids manual browsing and bad field prefetching in business code.
  • Don't include extra values generated for the webclient: __domain, __range, __fold, __context.
  • A falsy domain simply doesn't make any query at all, just like search. It is very convenient with domains like [(..., 'in', self.ids)] where self.ids can be empty (it happens a lot in case of onchange methods).

Performance regression:

  • The values of aggregation array_agg are now ordered by id, which is less efficient but it ensures a deterministic result.

Performance Testing

Performance tests show that the new version/usage is faster in general.
With a populated database (--size=medium + some tweaks) and with all these changes,
the performance of the _read_group/read_group calls, for 2280 calls, is in average +- 9.17 % faster.

  • 388 (on 2280) cases are statically slower (mean = -7.66 %, median = -5.17%) than before for several reasons:
    - more restricted access due to the commit named "simplify security of read_group", which in some case generates extra queries.
    - small overhead to instantiate recordsets;
    - array_agg is now deterministic;
    - noise.
  • 862 cases are statically faster (mean = +181.08 %, median = +15.82%) than before.

Next step

The second part of this task will be focused on the API of public method read_group():

  • Make it more consistent with this new _read_group.
  • Simplify the parsing of data (always return raw values aside the labelled ones).
  • Avoid useless aggregations.

Enterprise: https://github.com/odoo/enterprise/pull/38639

@robodoo
Copy link
Contributor

robodoo commented Jan 23, 2023

@ryv-odoo ryv-odoo force-pushed the master-new-read-group-refactor-tuple-version-ryv branch from 92af71d to 0c9e6ef Compare January 23, 2023 16:50
@C3POdoo C3POdoo added the RD research & development, internal work label Jan 23, 2023
@ryv-odoo ryv-odoo force-pushed the master-new-read-group-refactor-tuple-version-ryv branch 4 times, most recently from 269ddc8 to db9dbb2 Compare January 30, 2023 16:39
@ryv-odoo ryv-odoo force-pushed the master-new-read-group-refactor-tuple-version-ryv branch 5 times, most recently from 659e20a to c9043ef Compare February 7, 2023 06:35
@ryv-odoo ryv-odoo force-pushed the master-new-read-group-refactor-tuple-version-ryv branch 4 times, most recently from 56e1469 to 08036c0 Compare February 17, 2023 10:39
@ryv-odoo ryv-odoo force-pushed the master-new-read-group-refactor-tuple-version-ryv branch 9 times, most recently from 8b5525a to 597563d Compare February 22, 2023 08:24
@ryv-odoo ryv-odoo force-pushed the master-new-read-group-refactor-tuple-version-ryv branch 5 times, most recently from 364099d to 269fbc4 Compare March 6, 2023 16:06
@jjscarafia
Copy link
Contributor

Uou! Thanks a lot @ryv-odoo !

rco-odoo pushed a commit to odoo-dev/odoo that referenced this pull request Jan 9, 2024
_read_group_check_field_access_rights() is a superfluous hook of the
_read_group() refactor (odoo#110737). But
this hook isn't very useful compared to check_field_access_rights(),
it only checks the usage of fields used in _read_group().

- For 'res.users': Check the security in _read_group_select() and
_read_group_groupby() instead. We cannot forbid USER_PRIVATE_CHECK in
check_field_access_rights() because we want to obfuscate this field,
and not throw an AccessError when reading it.
- For 'project.task': override check_field_access_rights() to cover all
cases and override _determine_fields_to_fetch() to avoid reading an
inaccessible field and having an accidental AccessError.
robodoo pushed a commit that referenced this pull request Jan 9, 2024
_read_group_check_field_access_rights() is a superfluous hook of the
_read_group() refactor (#110737). But
this hook isn't very useful compared to check_field_access_rights(),
it only checks the usage of fields used in _read_group().

- For 'res.users': Check the security in _read_group_select() and
_read_group_groupby() instead. We cannot forbid USER_PRIVATE_CHECK in
check_field_access_rights() because we want to obfuscate this field,
and not throw an AccessError when reading it.
- For 'project.task': override check_field_access_rights() to cover all
cases and override _determine_fields_to_fetch() to avoid reading an
inaccessible field and having an accidental AccessError.

closes #146438

Related: odoo/enterprise#53409
Signed-off-by: Raphael Collet <rco@odoo.com>
robodoo pushed a commit that referenced this pull request Jan 10, 2024
_read_group_check_field_access_rights() is a superfluous hook of the
_read_group() refactor (#110737). But
this hook isn't very useful compared to check_field_access_rights(),
it only checks the usage of fields used in _read_group().

- For 'res.users': Check the security in _read_group_select() and
_read_group_groupby() instead. We cannot forbid USER_PRIVATE_CHECK in
check_field_access_rights() because we want to obfuscate this field,
and not throw an AccessError when reading it.
- For 'project.task': override check_field_access_rights() to cover all
cases and override _determine_fields_to_fetch() to avoid reading an
inaccessible field and having an accidental AccessError.

closes #146438

Related: odoo/enterprise#53409
Signed-off-by: Raphael Collet <rco@odoo.com>
ryv-odoo added a commit to odoo-dev/odoo that referenced this pull request Jan 15, 2024
As of odoo#110737, 'group_by_no_leaf' is
no longer used. Previously it was used to change the name of the
'__count' aggregate of read_group() but had no effect anywhere (and the
webclient just didn't care).

Remove this remnant.
robodoo pushed a commit that referenced this pull request Jan 16, 2024
As of #110737, 'group_by_no_leaf' is
no longer used. Previously it was used to change the name of the
'__count' aggregate of read_group() but had no effect anywhere (and the
webclient just didn't care).

Remove this remnant.

closes #147735

Related: odoo/enterprise#53488
Signed-off-by: Raphael Collet <rco@odoo.com>
xmglord added a commit to vauxoo-dev/addons-vauxoo that referenced this pull request Apr 1, 2024
- Simplify the way to define modifiers as states, required, readonly,
  invisible and column_invisible as part of [1].
- Remove owl="1" from OWL templates, as it's not needed anymore as all
  of them are OWL now as part of [2].
- Change qty_done to quantity and picked = True as part of [3].
- Adapt use of read_group to _read_group as part of [4].

[1] odoo/odoo#104741
[2] odoo/odoo#130467
[3] odoo/odoo#137864
[4] odoo/odoo#110737
xmglord added a commit to vauxoo-dev/addons-vauxoo that referenced this pull request Apr 1, 2024
- Simplify the way to define modifiers as states, required, readonly,
  invisible and column_invisible as part of [1].
- Remove owl="1" from OWL templates, as it's not needed anymore as all
  of them are OWL now as part of [2].
- Change qty_done to quantity and picked = True as part of [3].
- Adapt use of read_group to _read_group as part of [4].

[1] odoo/odoo#104741
[2] odoo/odoo#130467
[3] odoo/odoo#137864
[4] odoo/odoo#110737
xmglord added a commit to vauxoo-dev/addons-vauxoo that referenced this pull request Apr 1, 2024
- Simplify the way to define modifiers as states, required, readonly,
  invisible and column_invisible as part of [1].
- Remove owl="1" from OWL templates, as it's not needed anymore as all
  of them are OWL now as part of [2].
- Change qty_done to quantity and picked = True as part of [3].
- Adapt use of read_group to _read_group as part of [4].

[1] odoo/odoo#104741
[2] odoo/odoo#130467
[3] odoo/odoo#137864
[4] odoo/odoo#110737
xmglord added a commit to vauxoo-dev/addons-vauxoo that referenced this pull request Apr 3, 2024
- Simplify the way to define modifiers as states, required, readonly,
  invisible and column_invisible as part of [1].
- Remove owl="1" from OWL templates, as it's not needed anymore as all
  of them are OWL now as part of [2].
- Change qty_done to quantity and picked = True as part of [3].
- Adapt use of read_group to _read_group as part of [4].

[1] odoo/odoo#104741
[2] odoo/odoo#130467
[3] odoo/odoo#137864
[4] odoo/odoo#110737
xmglord added a commit to vauxoo-dev/addons-vauxoo that referenced this pull request Apr 4, 2024
- Simplify the way to define modifiers as states, required, readonly,
  invisible and column_invisible as part of [1].
- Remove owl="1" from OWL templates, as it's not needed anymore as all
  of them are OWL now as part of [2].
- Change qty_done to quantity and picked = True as part of [3].
- Adapt use of read_group to _read_group as part of [4].

[1] odoo/odoo#104741
[2] odoo/odoo#130467
[3] odoo/odoo#137864
[4] odoo/odoo#110737
xmglord added a commit to vauxoo-dev/addons-vauxoo that referenced this pull request Apr 4, 2024
- Simplify the way to define modifiers as states, required, readonly,
  invisible and column_invisible as part of [1].
- Remove owl="1" from OWL templates, as it's not needed anymore as all
  of them are OWL now as part of [2].
- Change qty_done to quantity and picked = True as part of [3].
- Adapt use of read_group to _read_group as part of [4].

[1] odoo/odoo#104741
[2] odoo/odoo#130467
[3] odoo/odoo#137864
[4] odoo/odoo#110737
luisg123v pushed a commit to Vauxoo/addons-vauxoo that referenced this pull request Apr 4, 2024
- Simplify the way to define modifiers as states, required, readonly,
  invisible and column_invisible as part of [1].
- Remove owl="1" from OWL templates, as it's not needed anymore as all
  of them are OWL now as part of [2].
- Change qty_done to quantity and picked = True as part of [3].
- Adapt use of read_group to _read_group as part of [4].

[1] odoo/odoo#104741
[2] odoo/odoo#130467
[3] odoo/odoo#137864
[4] odoo/odoo#110737
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
16.3 RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet