Fix `type/PK` -> `type/Name` and `type/FK` field remapping in the QB by ranquild · Pull Request #45047 · metabase/metabase

ranquild · 2024-07-02T15:16:33Z

Fixes #45063
Fixes #7231
Fixes #37955

BE changes:

Hydrates name_field for query metadata.

MBQL lib API changes:

Fixes dimensions parsing. This makes external, i.e. type/FK remapping work out of the box.
Makes type/PK -> type/Name field remapping work.

FE changes:

Makes SearchValuePicker handle mixed remapping options. Adds loading state.
Note that selected values are displayed as raw values and this is out of scope for this PR.

How to verify type/PK -> type/Name remapping:

Make sure that in Table Metadata for People.Name Entity Name semantic type is set
Set Filtering on this field for People.ID to A list of all values
Open People table. Click on ID -> Filter on this column. You should see a list of remapped values.
Set Filtering on this field for People.ID to Search box
Open People table. Click on ID -> Filter on this column. Start typing some name. You should get autocomplete. Please note that the placeholder Search by ID is misleading and this is something that we need to fix separately.

How to verify type/FK -> column remapping:

Open Table Metadata -> Orders.Product ID -> Click Use original value -> Use foreign key -> Select Products.Title.
Set Filtering on this field to A list of all values
Open Orders table. Click on Product ID -> Filter on this column. You should see a list of product names.
Do the same with Search box.

bshepherdson

These changes look sensible to me.

replay-io · 2024-07-02T20:05:01Z

Status	Complete ↗︎
Commit	`1b81e26`
Results	⚠️ 4 Flaky ✅ 2781 Passed

ranquild · 2024-07-03T01:02:59Z

src/metabase/lib/js/metadata.cljs

  `:internal`."
  [dimensions]
  (when-let [dimension (m/find-first (fn [dimension]
-                                       (#{"external" "internal"} (object-get dimension "type")))


dimension is a CLJS map here, not JS object; that's why all object-get calls return nil

kamilmielnik · 2024-07-11T10:00:01Z

...d/src/metabase/querying/components/FilterValuePicker/SearchValuePicker/SearchValuePicker.tsx

+      skip:
+        fieldId === searchFieldId ||
+        selectedValues.length === 0 ||
+        searchValue.length !== 0,


Is this to avoid CLS while user is typing? Or to optimize?
Definitely deserves a comment.

But maybe we don't need it?

kamilmielnik · 2024-07-11T10:00:36Z

...d/src/metabase/querying/components/FilterValuePicker/SearchValuePicker/SearchValuePicker.tsx

  );

-  const options = useMemo(() => getFieldOptions(fieldValues), [fieldValues]);
+  const options = useMemo(


Sometimes remapped values turn back to raw values.
In the video I'm using commas to apply subsequent values.

2024-07-11.16-58-45.mp4

2024-07-11.17-13-53.mp4

^ native inputs are not changed here, it's a different component

@kamilmielnik the only reason I can think of why values can turn into raw values:

search response -> pick value

we can remap the value because search results are still here

we send a new remapping request to the newly selected values

you start typing something new

there is a new search response

if the search response becomes available before the remapping response, in this interval you would see unmapped values

I might be able to fix this but this would make the implementation really really much more difficult.

OK, I made it out of scope of this PR. I cannot make it work cleanly after several days. Please take a look again, and read the PR description again.

e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js

kamilmielnik · 2024-07-11T10:07:34Z

e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js

+  });
+
+  describe("type/PK -> type/Name remapping (metabase#45063)", () => {
+    it("should work with questions", () => {


Should we assert metrics as well?

Metrics are going to be reworked soon. It's better not to test them right now

kamilmielnik · 2024-07-11T10:08:48Z

e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js

  });
 });

+describe("issue 45063", () => {


I don't see a repro for #7231

there is - you can actually find values

Let's add a (metabase#7231) tag somewhere (cy.log?) for it.
Also for #37955

github-actions · 2024-07-11T19:35:37Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 7e6adfa...1b81e26.

Notify	File(s)
@kdoh	frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx

…ed-values-remapping

ranquild · 2024-07-12T03:31:12Z

src/metabase/lib/field.cljc

@@ -787,15 +787,16 @@
 (mu/defn ^:private remapped-field :- [:maybe ::lib.schema.metadata/column]


PK values are not remapped to type/Name fields. But we allow searching by Name. That's why remapped-field checks for FKs only and is effectively unchanged.

kamilmielnik

FE code looks good, approving.
Please consider the UX suggestion 🙏

Note that selected values are displayed as raw values and this is out of scope for this PR.

Do we have this reported as a separate issue?

kamilmielnik · 2024-07-12T07:07:09Z

e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js

  });
 });

+describe("issue 45063", () => {


Let's add a (metabase#7231) tag somewhere (cy.log?) for it.
Also for #37955

kamilmielnik · 2024-07-12T07:12:18Z

...d/src/metabase/querying/components/FilterValuePicker/SearchValuePicker/SearchValuePicker.tsx

+  const searchOptions = canSearch ? getFieldOptions(fieldValues) : [];
+  const visibleOptions = getFilteredOptions(
+    searchOptions,
+    searchValue,
+    selectedValues,
+  );


I'm not confident it's a good idea to have FE-side filtering on top of BE-side filtering.
I see where this is coming from but it makes for a bad UX in some of the cases.
I tried it without getFilteredOptions and I think it works way more smooth this way.
Please consult product/design team if you disagree.

There's an annoying flicker of dropdown content on subsequent searches (but not the first one).

2024-07-12.14-08-12.mp4

I reported it separately: #45473

Fix remapped search values

3eec521

ranquild self-assigned this Jul 2, 2024

metabase-bot bot added the .Team/Querying label Jul 2, 2024

ranquild added 4 commits July 2, 2024 11:21

Fix remapping

8f7c1d7

Fix remapped values for "search" fields

4b03f31

Fix remapping

aedd110

Fix

1ce2277

ranquild added the backport Automatically create PR on current release branch on merge label Jul 2, 2024

ranquild added 11 commits July 2, 2024 14:20

Revert BE for now

06d061f

Revert FE

c39bc97

Revert FE

4d919a7

Fix

54ef350

Revert FE

578a0ec

Remove stub ids

c5c4eac

Fix

ef54acb

Fix

f5b7af1

Fix

e1aba09

Remove stub ids

e593e12

Fix

c5c3bb2

bshepherdson reviewed Jul 2, 2024

View reviewed changes

ranquild changed the title ~~Basic support for search values remapping~~ [WIP] Basic support for search values remapping Jul 2, 2024

ranquild marked this pull request as ready for review July 2, 2024 19:52

ranquild requested a review from camsaul as a code owner July 2, 2024 19:52

ranquild added 3 commits July 2, 2024 17:25

Fix

0d553b7

Fix

e5fdf9c

Fix dimension parsing in CLJS

5c15125

ranquild commented Jul 3, 2024

View reviewed changes

ranquild marked this pull request as draft July 3, 2024 01:03

ranquild changed the title ~~[WIP] Basic support for search values remapping~~ Fix type/FK field remapping in the QB Jul 3, 2024

Fixes

d6a2d72

kamilmielnik reviewed Jul 11, 2024

View reviewed changes

ranquild added 2 commits July 11, 2024 15:27

Fix options

68f6401

Fix loading state

542b7ad

ranquild added 4 commits July 11, 2024 15:39

Fix loading state

07150ed

An attempt

cb1f807

An attempt

55c7199

An attempt

ee7a48f

ranquild requested review from a team and kamilmielnik July 12, 2024 02:33

ranquild added 9 commits July 11, 2024 22:48

Merge remote-tracking branch 'refs/remotes/origin/master' into select…

2eb9bf7

…ed-values-remapping

another attempt

7957956

Another attempt

0e7122f

Fix tests

68872a2

Fix tests

3b8c596

Fix tests

5b65162

Fix tests

c3d87d0

Fix tests

355cac6

Fix tests

fbd34cd

ranquild commented Jul 12, 2024

View reviewed changes

ranquild added 3 commits July 12, 2024 00:02

Fix tests

42d0465

Fix lint

47a3502

Fix

1b81e26

kamilmielnik approved these changes Jul 12, 2024

View reviewed changes

ranquild merged commit 53955c2 into master Jul 12, 2024

ranquild deleted the selected-values-remapping branch July 12, 2024 11:19

github-automation-metabase mentioned this pull request Jul 12, 2024

🤖 backported "Fix type/PK -> type/Name and type/FK field remapping in the QB" #45467

Merged

kamilmielnik mentioned this pull request Jul 12, 2024

Filter search results flicker #45473

Closed

ranquild mentioned this pull request Jan 21, 2025

Foreign Key Remapping Causes Confusion #8599

Closed

		@@ -787,15 +787,16 @@
		(mu/defn ^:private remapped-field :- [:maybe ::lib.schema.metadata/column]

Conversation

ranquild commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bshepherdson left a comment

Choose a reason for hiding this comment

Uh oh!

replay-io bot commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranquild Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranquild Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranquild Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamilmielnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ranquild commented Jul 2, 2024 •

edited

Loading

replay-io bot commented Jul 2, 2024 •

edited

Loading

ranquild Jul 11, 2024 •

edited

Loading

ranquild Jul 12, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading

ranquild Jul 12, 2024 •

edited

Loading