Use case expression type inference logic similar to annotate middleware in MLv2 #47902

lbrdnk · 2024-09-12T21:32:38Z

Closes #47887
Closes #48010
Closes #47888

Initial description (not actual anymore)

The issue is that we infer return type for case expression as most specific common ancestor. If the arguments are :type/Date and :type/DateTime the ancestor is :type/Temporal. That results in limited filter picker options.

~~This PR adds special case for arguments of aforemementioned types. The :type/Date is picked as the expression type.~~

The question is however, whether the current behavior is really erroneous. To the best of my knowledge there's no way to decide correct argument type in this case, and picking the :type/Date is best effort, that could enable some cases, but may result in error in others.

~~Following stack (in reverse) is significant for the context (ie. if you are interested in how I got to this specific change):~~

metabase.lib.util/top-level-expression-clause
metabase.lib.schema.expression/type-of :case
metabase.lib.schema.expression.conditional/best-return-type

Description

This PR solves multiple problems.

Generic filter picker

We stopped checking for the type Temporal when selecting a appropriate filter picker, and switched to Date or DateTime. The case expression in #47887 is a bit tricky. It returns DateTime value when condition is met and Date value in default case. The MLv2 expression inference code was returning most specific common ancestor in this case - Temporal.

This PR changes the MLv2 case expression inference code to follow the logic in metabase.query-processor.middleware.annotate/infer-expression-type.

Bigquery error

Bigquery bugs have a different cause. Something changed in a way we compute expressions. In older version that was done in subquery. But in the current version, the expression from reproduction is computed on the same level. The Bigquery logic for appropriate database type inference is now presented with the case expression instead of reference to field from subquery.

This PR adds metabase.driver.bigquery-cloud-sdk.query-processor/temporal-type implementation for :case expression, that follows same logic laid out in the annotate/infer-expression-type.

Long story short, that results in appropriate type reconciliation by means of metabase.driver.bigquery-cloud-sdk.query-processor/reconcile-temporal-types.

Is that a right decision to model inference by `annotate/infer-expression-type`?

Depends on context. Yes in short term. Those 2 bugs are recent regressions and this PR addresses them in relatively non-invasive manner, expanding approach that is taken already in neighboring part of the code base.

In long term, when looking for general right solution to Type system issues mentioned by Ranquild, we should keep in mind this case.

Maybe that would entail creating new type hierarchies for different contexts as noted by Metamben.

ranquild

Looks correct. Let's add tests and merge.

metamben

To me, this means that we are saying that all operations that are valid for :type/Date are valid for a case expression containing a :type/DateTime value. If that's true, then :type/Date should be an ancestor of :type/DateTime, and this hack is not needed. If that's not true, then this invites trouble.

I have the feeling we are expecting too much from a single type hierarchy.

lbrdnk · 2024-10-03T18:10:39Z

e2e/test/scenarios/filters-reproductions/filters-reproductions.cy.spec.js

+  });
+
+  it("Case expression with type/Date default value and type/DateTime case value has Date filter popover enabled (metabase#47887)", () => {
+    visitQuestionAdhoc({


I'm not sure whether this is the right approach here. Would welcome FE eyes on this.

appleby · 2024-10-11T17:13:24Z

To me, this means that we are saying that all operations that are valid for :type/Date are valid for a case expression containing a :type/DateTime value. If that's true, then :type/Date should be an ancestor of :type/DateTime, and this hack is not needed. If that's not true, then this invites trouble.

I have the feeling we are expecting too much from a single type hierarchy.

I guess this comment was about a previous iteration of this PR, but overall agree. Maybe now it's even more lax since IIUC the temporal type of the case expression now depends on the order of the clauses, so we're saying any temporal type should be equivalent?

appleby

Approved since it fixes customer issues and it's reasonable to have the QP and schema agree on how to type the expressions, but also agree with @metamben's comment.

I would suggest to update that linked Notion doc under the temporal types heading with a link pointing to this issue.

appleby · 2024-10-11T16:32:40Z

modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/query_processor.clj

+  ;; Following logic for picking a type is taken from
+  ;; the [[metabase.query-processor.middleware.annotate/infer-expression-type]].
+  (loop [[cond-or-else expr & rezt*] rezt]
+    (when (and expr (not= :else cond-or-else))


why does this exclude the :else clause?

This is the way annotate/infer-expression-type operates currently.

appleby · 2024-10-11T16:34:42Z

.../drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj

+  (mt/test-driver
+   :bigquery-cloud-sdk
+   (testing "Query with case expression with default Date and case DateTime is executable (#47888)"
+     (is (=? {:status :completed}


does it make sense to check the result rows here?

Rows are empty in this case. Point of that test is to show whether query executes without underlying jdbc driver throwing an exception. I'll add the comment about that.

…re in MLv2 (#47902) * Use type date for case expression when there are Date and DateTime args * Use logic for getting case expr type from annotate middleware * Add e2e test * Add bigquery test * Update test * cljfmt and comment * Adjust e2e test

lbrdnk · 2024-10-18T16:12:02Z

@metabase-bot backport release-x.50.x

github-actions · 2024-10-18T16:13:53Z

@lbrdnk something went wrong while creating a backport [Logs]

…re in MLv2 (#47902) * Use type date for case expression when there are Date and DateTime args * Use logic for getting case expr type from annotate middleware * Add e2e test * Add bigquery test * Update test * cljfmt and comment * Adjust e2e test

…re in MLv2 (#47902) (#48893) * Use type date for case expression when there are Date and DateTime args * Use logic for getting case expr type from annotate middleware * Add e2e test * Add bigquery test * Update test * cljfmt and comment * Adjust e2e test Co-authored-by: lbrdnk <lbrdnk@users.noreply.github.com>

…re in MLv2 (#47902) * Use type date for case expression when there are Date and DateTime args * Use logic for getting case expr type from annotate middleware * Add e2e test * Add bigquery test * Update test * cljfmt and comment * Adjust e2e test

…otate middleware in MLv2 (#47902) (#48923) * Use case expression type inference logic similar to annotate middleware in MLv2 (#47902) * Use type date for case expression when there are Date and DateTime args * Use logic for getting case expr type from annotate middleware * Add e2e test * Add bigquery test * Update test * cljfmt and comment * Adjust e2e test * Adjust e2e test

lbrdnk requested a review from camsaul as a code owner September 12, 2024 21:32

lbrdnk changed the title ~~Use type date for case expression when there are Date and DateTime args~~ [WIP] Use type date for case expression when there are Date and DateTime args Sep 12, 2024

metabase-bot bot assigned lbrdnk Sep 12, 2024

metabase-bot bot added the .Team/Querying label Sep 12, 2024

lbrdnk force-pushed the 47887-date-filter branch from 2c7e06e to ac675d0 Compare September 12, 2024 22:05

ranquild approved these changes Sep 13, 2024

View reviewed changes

metamben reviewed Sep 13, 2024

View reviewed changes

lbrdnk force-pushed the 47887-date-filter branch from ac675d0 to 7a2b818 Compare October 3, 2024 15:35

lbrdnk added 2 commits October 3, 2024 17:36

Use type date for case expression when there are Date and DateTime args

9cdbcde

Use logic for getting case expr type from annotate middleware

c8f06e9

lbrdnk force-pushed the 47887-date-filter branch from 7a2b818 to c8f06e9 Compare October 3, 2024 15:37

lbrdnk added 2 commits October 3, 2024 20:04

Add e2e test

a4ec22f

Add bigquery test

6c1cea8

lbrdnk commented Oct 3, 2024

View reviewed changes

lbrdnk changed the title ~~[WIP] Use type date for case expression when there are Date and DateTime args~~ Use type date for case expression when there are Date and DateTime args Oct 3, 2024

Update test

b89aafd

lbrdnk added the backport Automatically create PR on current release branch on merge label Oct 4, 2024

lbrdnk changed the title ~~Use type date for case expression when there are Date and DateTime args~~ Use case expression type inference logic similar to annotate middleware in MLv2 Oct 11, 2024

appleby approved these changes Oct 11, 2024

View reviewed changes

lbrdnk added 2 commits October 17, 2024 19:20

cljfmt and comment

7710f3b

Adjust e2e test

a54d4af

lbrdnk merged commit 63a3b4b into master Oct 18, 2024
132 checks passed

lbrdnk deleted the 47887-date-filter branch October 18, 2024 15:41

github-automation-metabase mentioned this pull request Oct 18, 2024

🤖 backported "Use case expression type inference logic similar to annotate middleware in MLv2" #48893

Merged

lbrdnk mentioned this pull request Oct 21, 2024

[Backport 50] Use case expression type inference logic similar to annotate middleware in MLv2 (#47902) #48923

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use case expression type inference logic similar to annotate middleware in MLv2 #47902

Use case expression type inference logic similar to annotate middleware in MLv2 #47902

lbrdnk commented Sep 12, 2024 •

edited

Loading

ranquild left a comment

metamben left a comment

lbrdnk Oct 3, 2024

appleby commented Oct 11, 2024

appleby left a comment

appleby Oct 11, 2024

lbrdnk Oct 17, 2024

appleby Oct 11, 2024

lbrdnk Oct 17, 2024 •

edited

Loading

lbrdnk commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

Use case expression type inference logic similar to annotate middleware in MLv2 #47902

Use case expression type inference logic similar to annotate middleware in MLv2 #47902

Conversation

lbrdnk commented Sep 12, 2024 • edited Loading

Initial description (not actual anymore)

Description

Generic filter picker

Bigquery error

Is that a right decision to model inference by annotate/infer-expression-type?

ranquild left a comment

Choose a reason for hiding this comment

metamben left a comment

Choose a reason for hiding this comment

lbrdnk Oct 3, 2024

Choose a reason for hiding this comment

appleby commented Oct 11, 2024

appleby left a comment

Choose a reason for hiding this comment

appleby Oct 11, 2024

Choose a reason for hiding this comment

lbrdnk Oct 17, 2024

Choose a reason for hiding this comment

appleby Oct 11, 2024

Choose a reason for hiding this comment

lbrdnk Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

lbrdnk commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

lbrdnk commented Sep 12, 2024 •

edited

Loading

Is that a right decision to model inference by `annotate/infer-expression-type`?

lbrdnk Oct 17, 2024 •

edited

Loading