-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Support Offset()
as an expression with no breakouts [backend]
#42132
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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