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.

Sign up for GitHub

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

Jump to bottom

fix(search): search on aggregated results in HAVING clause #524

Merged
merged 6 commits into from
Mar 14, 2024
Merged

fix(search): search on aggregated results in HAVING clause #524

merged 6 commits into from
Mar 14, 2024

Conversation

patricebender
Copy link
Member

@patricebender patricebender commented Mar 11, 2024 •
edited
Loading

Although this use case is probably not really common or makes much sense, there were some failing cds tests which should be fixed by this.

If the query contains a group by, the search is performed on the queries
columns. If one of those columns is an aggregate function,
we must put the search term in the HAVING clause instead of WHERE.
Since the logical processing order of SQL queries is FROM -> WHERE -> GROUP BY -> HAVING -> SELECT -> ORDER BY the aggregated results are not accessible in the WHERE clause.

This led to an error:

> Uncaught SqliteError: misuse of aggregate: COUNT() in:
SELECT
 COUNT(Books.stock) AS foo
FROM
  sap_capire_bookshop_Books AS Books
WHERE
  (IFNULL(INSTR(LOWER(COUNT(stock)), LOWER(?)), 0))
GROUP BY Books.title

With this change, the searchable columns of the query are
checked against the aggregate functions of the ANSI SQL standard.
If one is found and the query contains a group by (which should then
always be the case), the search expression is put into the HAVING clause.

larslutz96 and others added 2 commits March 8, 2024 12:52
@larslutz96
Update search.test.js
20fb0df
@patricebender
fix(search): search on aggregated results in HAVING clause
90926d4 
If the query contains a `group by`, the search is performed on the queries
columns. If one of those columns is an aggregate function,
we must put the search term in the `HAVING` clause instead of `WHERE`.
Since the logical processing order of SQL queries is `FROM -> WHERE -> GROUP BY -> HAVING -> SELECT -> ORDER BY` the aggregated results are not accessible in the `WHERE` clause.

This led to an error:

```sql
> Uncaught SqliteError: misuse of aggregate: COUNT() in:
SELECT
            COUNT(Books.stock) AS foo
        FROM
            sap_capire_bookshop_Books AS Books
        WHERE
            (IFNULL(INSTR(LOWER(COUNT(stock)), LOWER(?)), 0))
        GROUP BY
            Books.title
```

With this change, the searchable columns of the query are
checked against the  aggregate functions of the ANSI SQL standard.
If one is found and the query contains a `group by` (which should then
always be the case), the search expression is put into the `HAVING` clause.

--> fixes some cds tests
@BobdenOs
Copy link
Contributor

BobdenOs commented Mar 11, 2024 •
edited
Loading

@patricebender this change does not make any sense to me. A $search is looking for a string. So why would we try to search inside the result of a sum ?

Also if the $search is applied after the group by it might create the wrong result. My expectation would be that we only search in fields that are "search relevant". Which only makes sense to do with source columns. Not with calculated or aggregated columns. Also as the HANA contains function (which is supposed to be the productive $search) implementation does not support calculated columns.

@patricebender
Copy link
Member Author

@patricebender this change does not make any sense to me. A $search is looking for a string. So why would we try to search inside the result of a sum ?

Also if the $search is applied after the group by it might create the wrong result.

This was requested by @larslutz96 because some CDS tests are failing → In the legacy stack there is such a mechanism in place. Maybe Lars can elaborate a little further.

I aggree that there are probably no real use cases for this, but at least there is no sql error anymore. And it should not break what has worked before, does it?

@BobdenOs
Copy link
Contributor

Seems to work with the current search implementation. As it is not yet using the new contains function. Also it is explicitly being cast to a string code

So if this is really the intended behavior we can apply this change. @johannes-vogel could you please verify this is the intended behavior?

@patricebender patricebender enabled auto-merge (squash) March 12, 2024 16:05
@patricebender patricebender merged commit 61d348e into main Mar 14, 2024
4 checks passed
@patricebender patricebender deleted the fix-search-with-aggregation-in-groupBy branch March 14, 2024 10:03
@cap-bots cap-bots mentioned this pull request Mar 14, 2024
Merged
patricebender added a commit that referenced this pull request Mar 22, 2024
@cap-bots @johannes-vogel @patricebender
chore: release main (#494)
cfd7d57 
🤖 I have created a release *beep* *boop*
---


<details><summary>db-service: 1.7.0</summary>

##
[1.7.0](db-service-v1.6.4...db-service-v1.7.0)
(2024-03-22)


### Added

* also support lowercase matchespattern function
([#528](#528))
([6ea574e](6ea574e))
* forUpdate and forShareLock
([#148](#148))
([99a1170](99a1170))
* **hana:** drop prepared statements after end of transaction
([#537](#537))
([b1f864e](b1f864e))
* **orderby:** allow to disable collations with
[@cds](https://github.com/cds).collate: false
([#492](#492))
([820f971](820f971))


### Fixed

* **cqn2sql:** Smart quoting of columns inside UPSERT rows
([#519](#519))
([78fe10b](78fe10b))
* deep delete for views without navigation
([#434](#434))
([3ebc9c2](3ebc9c2))
* Getting rid of quirks mode
([#514](#514))
([c9aa6e8](c9aa6e8))
* issue with reused select cqns
([#505](#505))
([916d175](916d175))
* joins without columns are rejected
([#535](#535))
([eb9beda](eb9beda))
* **search:** dont search non string aggregations
([#527](#527))
([c87900c](c87900c))
* **search:** search on aggregated results in HAVING clause
([#524](#524))
([61d348e](61d348e))
</details>

<details><summary>sqlite: 1.6.0</summary>

##
[1.6.0](sqlite-v1.5.1...sqlite-v1.6.0)
(2024-03-22)


### Added

* forUpdate and forShareLock
([#148](#148))
([99a1170](99a1170))
* **hana:** drop prepared statements after end of transaction
([#537](#537))
([b1f864e](b1f864e))


### Fixed

* **`sqlite`:** use keyword list from compiler
([#526](#526))
([a227c61](a227c61))
</details>

<details><summary>postgres: 1.6.0</summary>

##
[1.6.0](postgres-v1.5.1...postgres-v1.6.0)
(2024-03-22)


### Added

* also support lowercase matchespattern function
([#528](#528))
([6ea574e](6ea574e))
* forUpdate and forShareLock
([#148](#148))
([99a1170](99a1170))


### Changed

* use new cds build API @sap/cds-dk &gt;= 7.5.0
([#508](#508))
([ef22ebe](ef22ebe))
</details>

<details><summary>hana: 0.1.0</summary>

##
[0.1.0](hana-v0.0.6...hana-v0.1.0)
(2024-03-22)


### Added

* also support lowercase matchespattern function
([#528](#528))
([6ea574e](6ea574e))
* forUpdate and forShareLock
([#148](#148))
([99a1170](99a1170))
* **hana:** drop prepared statements after end of transaction
([#537](#537))
([b1f864e](b1f864e))


### Fixed

* **`hana`:** use keyword list from compiler
([#525](#525))
([c6993d9](c6993d9))
* **hana:** improve search inside where clause detection
([#538](#538))
([51b8af3](51b8af3))
* **hana:** reduce service manager calls for failing tenants
([#533](#533))
([e95fd17](e95fd17))
* issue with reused select cqns
([#505](#505))
([916d175](916d175))
* joins without columns are rejected
([#535](#535))
([eb9beda](eb9beda))
* mass insert for unknown entities
([#540](#540))
([f2ea4af](f2ea4af))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: Johannes Vogel <[email protected]>
Co-authored-by: Patrice Bender <[email protected]>
@cap-bots cap-bots mentioned this pull request Jul 18, 2024
Merged
This was referenced Jan 28, 2025
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BobdenOs BobdenOs BobdenOs left review comments

@johannes-vogel johannes-vogel johannes-vogel approved these changes

@danjoa danjoa Awaiting requested review from danjoa danjoa is a code owner

@stewsk stewsk Awaiting requested review from stewsk stewsk is a code owner

@larslutz96 larslutz96 Awaiting requested review from larslutz96

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@patricebender @BobdenOs @johannes-vogel @larslutz96