Support `Offset()` in custom columns [frontend] #42326

kamilmielnik · 2024-05-07T11:16:07Z

How to verify

Create a new question based on Orders table
Add a custom column with custom expression, e.g. Offset([Total], -3)
Include order by clause (or a breakout) in the query, e.g. sort by order id
Run the query

…o offset-part-2

…t function

- Rename identifiers in isCompatible - Make isCompatible accept a clause object instead of just the type - Handle offset as a special case in isCompatible

kamilmielnik · 2024-05-08T11:51:49Z

frontend/src/metabase-lib/v1/expressions/config.ts

@@ -303,6 +291,18 @@ export const MBQL_CLAUSES: MBQLClauseMap = {
    args: ["expression", "expression"], // ideally we'd alternate boolean/expression
    multiple: true,
  },
+  offset: {
+    displayName: `Offset`,
+    type: "any", // ideally we'd dynamically infer it from the first argument


I moved the definition from aggregations section closer to somewhat similar functions (coalesce and case) and changed the return type to "any", since offset will return the same type as it receives as the first argument (which is "any").

kamilmielnik · 2024-05-08T11:53:34Z

frontend/src/metabase-lib/v1/expressions/resolver.js

-// and b is the inferred type of the argument
-const isCompatible = (a, b) => {
-  if (a === "any") {
+const isCompatible = (expectedType, inferredType) => {


I renamed a & b to something more meaningful (which was previously conveyed in a comment).

This reverts commit e63790b.

kamilmielnik · 2024-05-09T12:03:27Z

frontend/src/metabase-lib/v1/expressions/diagnostics.ts

      adjustOffset,
+      adjustCase,


The order matters. This fixes #42377

…umns-frontend

kamilmielnik · 2024-05-15T07:15:36Z

frontend/src/metabase-lib/v1/expressions/resolver.js

-const isCompatible = (a, b) => {
-  if (a === "any") {
+const isCompatible = (expectedType, inferredType) => {
+  if (expectedType === "any" || inferredType === "any") {


The second part of the condition is new, the rest of the function works the same as before

replay-io · 2024-05-15T07:26:34Z

Status	Complete ↗︎
Commit	`688721e`
Results	⚠️ 1 Flaky ✅ 2507 Passed

romeovs · 2024-05-15T10:49:57Z

The code looks good!

Works as expected in the when there is a order by clause, but running the query breaks when it is absent.

* [MBQL lib] Reject broken uses of `offset` in expressions, filters (#42662) - Using `offset` in custom expressions is supported only when there is an order-by defined on that stage. - Using `offset` in custom filters is not supported at all, currently. * Support `Offset()` in custom columns [frontend] (#42326) * Only nest expressions referenced in breakouts or aggregations * Support Offset() as expression with no breakouts * Test fixes 🔧 * Oracle test update * Improved Oracle test * Test update 🔧 * Fix busted test * Add naive support for Offset() in expressions * Introduce MBQLClauseFunctionReturnType * Add "window" to MBQLClauseFunctionReturnType and use it for the offset function * Handle offset type inference * Remove unused export * Use "expression" instead of "window" return type - Rename identifiers in isCompatible - Make isCompatible accept a clause object instead of just the type - Handle offset as a special case in isCompatible * Use any type * Rename expectedArgumentType to expectedType * Format code * Sort types * Do not suggest offset function in filters * Fix offset not working in case * Revert "Do not suggest offset function in filters" This reverts commit e63790b. * Fix order of adjustments --------- Co-authored-by: Cam Saul <github@camsaul.com> Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> * Disable offsets in filters expressions * Group existing aggregations-specific tests * Remove repro for a closed issue * Use findByText instead of contains * Add a test for filter expressions * Add a test for aggregation expressions suggestions * Disable offsets in filters expressions (#42755) * Add a test for custom column suggestions * Add "typing" to enterCustomColumnDetails * Use enterCustomColumnDetails, improve assertions * Add more assertions * Optimize queries * Add typing for offset expressions * Add a repro for #42764 * Add a TODO * Add a TODO * Add a TODO * Use NumericLiteral * Post-merge fixes * Update test * Add tests for other custom expressions * Test drills * Format code * Update test name * Add an assertion * Add assertions for the preview * Unskip fixed issue --------- Co-authored-by: Braden Shepherdson <braden@metabase.com> Co-authored-by: Cam Saul <github@camsaul.com> Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com>

* [MBQL lib] Reject broken uses of `offset` in expressions, filters (#42662) - Using `offset` in custom expressions is supported only when there is an order-by defined on that stage. - Using `offset` in custom filters is not supported at all, currently. * Support `Offset()` in custom columns [frontend] (#42326) * Only nest expressions referenced in breakouts or aggregations * Support Offset() as expression with no breakouts * Test fixes 🔧 * Oracle test update * Improved Oracle test * Test update 🔧 * Fix busted test * Add naive support for Offset() in expressions * Introduce MBQLClauseFunctionReturnType * Add "window" to MBQLClauseFunctionReturnType and use it for the offset function * Handle offset type inference * Remove unused export * Use "expression" instead of "window" return type - Rename identifiers in isCompatible - Make isCompatible accept a clause object instead of just the type - Handle offset as a special case in isCompatible * Use any type * Rename expectedArgumentType to expectedType * Format code * Sort types * Do not suggest offset function in filters * Fix offset not working in case * Revert "Do not suggest offset function in filters" This reverts commit e63790b. * Fix order of adjustments --------- * Disable offsets in filters expressions * Group existing aggregations-specific tests * Remove repro for a closed issue * Use findByText instead of contains * Add a test for filter expressions * Add a test for aggregation expressions suggestions * Disable offsets in filters expressions (#42755) * Add a test for custom column suggestions * Add "typing" to enterCustomColumnDetails * Use enterCustomColumnDetails, improve assertions * Add more assertions * Optimize queries * Add typing for offset expressions * Add a repro for #42764 * Add a TODO * Add a TODO * Add a TODO * Use NumericLiteral * Post-merge fixes * Update test * Add tests for other custom expressions * Test drills * Format code * Update test name * Add an assertion * Add assertions for the preview * Unskip fixed issue --------- Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com> Co-authored-by: Braden Shepherdson <braden@metabase.com> Co-authored-by: Cam Saul <github@camsaul.com> Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com>

camsaul and others added 11 commits May 6, 2024 23:26

Only nest expressions referenced in breakouts or aggregations

c98cb0c

Support Offset() as expression with no breakouts

0e032bc

Merge branch 'only-nest-expressions-in-breakouts-or-aggregations' int…

b99b38b

…o offset-part-2

Test fixes 🔧

289fd3e

Merge branch 'only-nest-expressions-in-breakouts-or-aggregations' int…

cc1cb98

…o offset-part-2

Oracle test update

ac3f9a8

Improved Oracle test

acbf163

Merge branch 'only-nest-expressions-in-breakouts-or-aggregations' int…

5674086

…o offset-part-2

Test update 🔧

7bc8ce3

Fix busted test

e5ddb86

Add naive support for Offset() in expressions

d003b0d

kamilmielnik added .Frontend no-backport Do not backport this PR to any branch .Team/QueryingComponents labels May 7, 2024

kamilmielnik self-assigned this May 7, 2024

Base automatically changed from offset-part-2 to master May 7, 2024 20:05

kamilmielnik added 7 commits May 8, 2024 14:14

Merge branch 'master' into 42318-offset-custom-columns-frontend

2b14acc

Introduce MBQLClauseFunctionReturnType

3f59fba

Add "window" to MBQLClauseFunctionReturnType and use it for the offse…

86a1899

…t function

Handle offset type inference

3c000b6

Remove unused export

b33a4c1

Use "expression" instead of "window" return type

1c47576

- Rename identifiers in isCompatible - Make isCompatible accept a clause object instead of just the type - Handle offset as a special case in isCompatible

Use any type

0f5ea47

kamilmielnik commented May 8, 2024

View reviewed changes

Rename expectedArgumentType to expectedType

2173b66

kamilmielnik commented May 8, 2024

View reviewed changes

kamilmielnik added 4 commits May 8, 2024 18:54

Format code

1262ad1

Sort types

6e33309

Do not suggest offset function in filters

e63790b

Fix offset not working in case

5ddbe34

Revert "Do not suggest offset function in filters"

f1f0b02

This reverts commit e63790b.

kamilmielnik commented May 9, 2024

View reviewed changes

kamilmielnik mentioned this pull request May 9, 2024

diagnose-expression throws when Offset is nested #42377

Closed

kamilmielnik added 2 commits May 10, 2024 16:17

Merge branch 'master' into 42318-offset-custom-columns-frontend

3eb2f03

Fix order of adjustments

8c25b4f

kamilmielnik changed the base branch from master to offset-custom-expressions May 15, 2024 07:13

kamilmielnik marked this pull request as ready for review May 15, 2024 07:13

Merge branch 'offset-custom-expressions' into 42318-offset-custom-col…

688721e

…umns-frontend

kamilmielnik commented May 15, 2024

View reviewed changes

kamilmielnik requested a review from a team May 15, 2024 07:15

ranquild approved these changes May 15, 2024

View reviewed changes

ranquild merged commit 74db2b3 into offset-custom-expressions May 15, 2024
110 checks passed

ranquild deleted the 42318-offset-custom-columns-frontend branch May 15, 2024 19:43

This was referenced May 16, 2024

Support Offset() in custom columns #42318

Closed

Offset in custom expressions #42751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `Offset()` in custom columns [frontend] #42326

Support `Offset()` in custom columns [frontend] #42326

kamilmielnik commented May 7, 2024 •

edited

kamilmielnik May 8, 2024 •

edited

kamilmielnik May 8, 2024 •

edited

kamilmielnik May 9, 2024

kamilmielnik May 15, 2024

replay-io bot commented May 15, 2024 •

edited

romeovs commented May 15, 2024 •

edited

Support Offset() in custom columns [frontend] #42326

Support Offset() in custom columns [frontend] #42326

Conversation

kamilmielnik commented May 7, 2024 • edited

How to verify

kamilmielnik May 8, 2024 • edited

Choose a reason for hiding this comment

kamilmielnik May 8, 2024 • edited

Choose a reason for hiding this comment

kamilmielnik May 9, 2024

Choose a reason for hiding this comment

kamilmielnik May 15, 2024

Choose a reason for hiding this comment

replay-io bot commented May 15, 2024 • edited

romeovs commented May 15, 2024 • edited

Support `Offset()` in custom columns [frontend] #42326

Support `Offset()` in custom columns [frontend] #42326

kamilmielnik commented May 7, 2024 •

edited

kamilmielnik May 8, 2024 •

edited

kamilmielnik May 8, 2024 •

edited

replay-io bot commented May 15, 2024 •

edited

romeovs commented May 15, 2024 •

edited