Support Offset() as an expression with no breakouts [backend]
#42132
Conversation
|
It still shows error about aggregation. Probably it should say something about that offset in customs columns is not yet supported for anything except postgres. |
|
Well, for the postgres db I got the same error |
camsaul
changed the base branch from
nest-query-mlv2
to
only-nest-expressions-in-breakouts-or-aggregations
camsaul
changed the title
Offset() as an expression with no breakouts [WIP]Offset() as an expression with no breakouts [backend]
|
@mngr this PR is only for the backend, maybe you or someone else can help with FE changes? And the PR was a "WIP" draft PR when you commented, so the behavior (and what DBs this supports or does not support) is not finalized. |
|
| @@ -109,14 +109,23 @@ | |||
| (= ref-type (first x)) | |||
| (not (contains? valid-ids (get x 2))))) | |||
|
|
|||
| (defn- stage-with-joins-and-namespaced-keys-removed | |||
There was a problem hiding this comment.
I understand that using Offset in custom column expression requires presence of an order-by clause or a breakout clause. The following video shows me trying it out:
- 0:15 - adding order by clause does not change anything, query still fails
- 0:25 - editing a custom column adds
effective-type: "type/Float"in pMBQL and BE throws because of it
Are those BE issues?
2024-05-07.18-22-41.mp4
There was a problem hiding this comment.
what branch are you on, so I can test this locally?
Btw I just fixed the effective-type error, #42323 should be fixed in this branch too
There was a problem hiding this comment.
Aforementioned issues seem to be fixed now ✔️
Sorry, I was using 42318-offset-custom-columns-frontend branch to test it.
|
@camsaul Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Follow-on to #41346
Fixes #42323
Backend support for using
offset()as a plain expression (as opposed to an aggregation or in an aggregation expression)Most of the changes to make this work are actually in #42302, this PR is mostly just a few more small tweaks and extra backend tests.
FE work will have to be done separately, I'm not currently planning on trying to tackle it so maybe someone from QC can update things once this lands