From 3e9f4ac5152f63fe829f4934143ff47843dc6a13 Mon Sep 17 00:00:00 2001
From: Kamil Mielnik <kamil@kamilmielnik.com>
Date: Fri, 10 May 2024 20:39:09 +0700
Subject: [PATCH] Fix - `diagnose-expression` throws when `Offset` is nested
(#42497)
* Fix offset not working in case
* Make offset function return any
* Add a repro for #42377
* Fix order of adjustments
* Revert unrelated changes
* Remove commented code
* Revert unrelated changes
---
e2e/test/scenarios/question/offset.cy.spec.ts | 43 +++++++++++++++++++
.../v1/expressions/diagnostics.ts | 2 +-
.../v1/expressions/recursive-parser.js | 2 +-
.../metabase/lib/expressions/pratt/common.ts | 2 +-
4 files changed, 46 insertions(+), 3 deletions(-)
create mode 100644 e2e/test/scenarios/question/offset.cy.spec.ts
diff --git a/e2e/test/scenarios/question/offset.cy.spec.ts b/e2e/test/scenarios/question/offset.cy.spec.ts
new file mode 100644
index 00000000000..f20370dc1e3
--- /dev/null
+++ b/e2e/test/scenarios/question/offset.cy.spec.ts
@@ -0,0 +1,43 @@
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+import type { StructuredQuestionDetails } from "e2e/support/helpers";
+import {
+ createQuestion,
+ enterCustomColumnDetails,
+ getNotebookStep,
+ openNotebook,
+ popover,
+ restore,
+} from "e2e/support/helpers";
+
+const { ORDERS_ID } = SAMPLE_DATABASE;
+
+describe("scenarios > question > offset", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ });
+
+ it("should allow using OFFSET as a CASE argument (metabase#42377)", () => {
+ const formula = "Sum(case([Total] > 0, Offset([Total], -1)))";
+ const name = "Aggregation";
+ const questionDetails: StructuredQuestionDetails = {
+ query: {
+ "source-table": ORDERS_ID,
+ limit: 5,
+ },
+ };
+ createQuestion(questionDetails, { visitQuestion: true });
+ openNotebook();
+
+ cy.icon("sum").click();
+ getNotebookStep("summarize")
+ .findByText("Pick the metric you want to see")
+ .click();
+ popover().contains("Custom Expression").click();
+ enterCustomColumnDetails({ formula, name });
+
+ cy.on("uncaught:exception", error => {
+ expect(error.message.includes("Error normalizing")).not.to.be.true;
+ });
+ });
+});
diff --git a/frontend/src/metabase-lib/v1/expressions/diagnostics.ts b/frontend/src/metabase-lib/v1/expressions/diagnostics.ts
index 167e35a5f47..0e44cb5a833 100644
--- a/frontend/src/metabase-lib/v1/expressions/diagnostics.ts
+++ b/frontend/src/metabase-lib/v1/expressions/diagnostics.ts
@@ -228,8 +228,8 @@ function prattCompiler({
passes: [
adjustOptions,
useShorthands,
- adjustCase,
adjustOffset,
+ adjustCase,
expression =>
resolve({
expression,
diff --git a/frontend/src/metabase-lib/v1/expressions/recursive-parser.js b/frontend/src/metabase-lib/v1/expressions/recursive-parser.js
index 6c3d0bbcb41..6b4a039197e 100644
--- a/frontend/src/metabase-lib/v1/expressions/recursive-parser.js
+++ b/frontend/src/metabase-lib/v1/expressions/recursive-parser.js
@@ -372,6 +372,6 @@ export const parse = pipe(
recursiveParse,
adjustOptions,
useShorthands,
- adjustCase,
adjustOffset,
+ adjustCase,
);
diff --git a/frontend/test/metabase/lib/expressions/pratt/common.ts b/frontend/test/metabase/lib/expressions/pratt/common.ts
index 1d7089eb63b..7739f219813 100644
--- a/frontend/test/metabase/lib/expressions/pratt/common.ts
+++ b/frontend/test/metabase/lib/expressions/pratt/common.ts
@@ -25,7 +25,7 @@ interface Opts {
export function compile(source: string, type: Type, opts: Opts = {}) {
const { throwOnError } = opts;
- const passes = [adjustOptions, useShorthands, adjustCase, adjustOffset];
+ const passes = [adjustOptions, useShorthands, adjustOffset, adjustCase];
return newCompile(
parse(lexify(source), {
throwOnError,
--
GitLab