-
Notifications
You must be signed in to change notification settings - Fork 14
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
fix: cqn transformation is OData agnostic #990
base: main
Are you sure you want to change the base?
Conversation
db-service/lib/cqn4sql.js
Outdated
const wildcardIndex = column.expand.findIndex(e => e === '*') | ||
if (wildcardIndex === -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const wildcardIndex = column.expand.findIndex(e => e === '*') | |
if (wildcardIndex === -1) { | |
if (column.expand.includes('*')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that mean that:
- For
SELECT Items {*} from Orders group by Items.ID
we would not skip the expand? - For
SELECT Items { ID } from Orders group by Items.ID
we would skip the expand?
→ still seems wrong
As discussed we need to revisit the way we translate OData |
Moreover, and more importantly, we have to ensure that we correctly implement group by, also for things which OData cannot express. In particular, for expands of to-many associations like that: SELECT from Orders {
ID, sum (Items.product.price * Items.quantity) as total,
Items { product.name, quantity }
} group by ID |
Agree 👍
Also agree, however I would make vote for a two step approach:
|
@johannes-vogel I reverted the special handling of expand + group by which we added due to issues of OData $apply + $expand. Now there are some tests failing (the ones Daniel already commented on) I think the next step would be for the runtime to patch the OData adapter in a way, that the CQNs are generated so that the tests will work again, even without cqn4sql doing some magic for those cases. Will you dispatch that? |
… the column this is a workaround for an issue reported in cap/issues#17907 --> we need to put all this logic in the OData protocol adapter, this coding should not be necessary for the DB. This initiative has already started in #990
this resolves an `Uncaught Object.defineProperty called on non-object` dump this is a workaround for an issue reported in cap/issues#17907 --> we need to put all this logic in the OData protocol adapter, this coding should not be necessary for the DB. This initiative has already started in #990
No description provided.