[MBQL lib] Reject offset in expressions with no sort order
#42662
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @bshepherdson and the rest of your teammates on |
|
snoe
approved these changes
May 14, 2024
bshepherdson
force-pushed
the
mblib-offset-diagnose-expression-require-order-by
branch
from
4253423
to
2eb1c3c
Compare
ranquild
approved these changes
May 14, 2024
bshepherdson
force-pushed
the
mblib-offset-diagnose-expression-require-order-by
branch
from
2eb1c3c
to
07ed12e
Compare
camsaul
approved these changes
May 15, 2024
- 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.
bshepherdson
force-pushed
the
mblib-offset-diagnose-expression-require-order-by
branch
from
07ed12e
to
00bb26e
Compare
bshepherdson
deleted the
mblib-offset-diagnose-expression-require-order-by
branch
kamilmielnik
added a commit
that referenced
this pull request
May 21, 2024
* [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>
github-actions bot
pushed a commit
that referenced
this pull request
May 21, 2024
* [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>
metabase-bot bot
added a commit
that referenced
this pull request
May 21, 2024
* [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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using
offsetas an aggregation is fine, but it doesn't work as anexpression unless the query has an order-by.
Part of #42318.