Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Field list / filtering / grouping does not work with a native model that has comments #35039

Closed
zbodi74 opened this issue Oct 25, 2023 · 7 comments · Fixed by #39201 or #39311
Closed
Labels
.Backend Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/GUI Query builder catch-all, including simple mode Querying/Native The SQL/native query editor .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. .Team/QueryProcessor :hammer_and_wrench: Type:Bug Product defects .Wanted: MLv2 Issues that will be fixed (or easier to fix, or possible to fix) when we have MLv2

Comments

@zbodi74
Copy link

zbodi74 commented Oct 25, 2023

Describe the bug

The query builder does not show source fields for models based on native queries, when the query has trailing comments.

To Reproduce

  1. Create a native model with the following SQL
select * from products
-- where true=false

make sure there is no trailing space in the last line

  1. Create a GUI question based on the model.
  2. (!) The field list in the column selector is empty.
  3. (!) Summarize and Group also do not offer options with source fields.

Expected behavior

It should work as before.

Logs

2023-10-25 09:08:30,674 DEBUG middleware.log :: POST /api/dataset 202 [ASYNC: completed] 68.9 ms (16 DB calls) App DB connections: 0/15 Jetty threads: 4/50 (6 idle, 0 queued) (125 total active threads) Queries in flight: 0 (0 queued); h2 DB 1 connections: 0/1 (0 threads blocked)
2023-10-25 09:08:30,722 DEBUG middleware.log :: GET /api/database/1 200 2.8 ms (3 DB calls) App DB connections: 0/15 Jetty threads: 5/50 (6 idle, 0 queued) (125 total active threads) Queries in flight: 0 (0 queued)
2023-10-25 09:08:33,864 DEBUG middleware.log :: GET /api/alert/question/9 200 1.3 ms (1 DB calls) App DB connections: 0/15 Jetty threads: 5/50 (6 idle, 0 queued) (125 total active threads) Queries in flight: 0 (0 queued)
2023-10-25 09:09:23,535 INFO api.dataset :: Source query for this query is Card 9
2023-10-25 09:09:23,546 INFO middleware.fetch-source-query :: Trimming trailing comment from card with id 9
2023-10-25 09:09:23,550 INFO middleware.fetch-source-query :: Trimming trailing comment from card with id 9
2023-10-25 09:09:23,554 WARN middleware.add-implicit-clauses :: Warning: cannot determine fields for an explicit `source-query` unless you also include `source-metadata`.
Query: {:native "select * from products\n"}

Information about your Metabase installation

1.47.5

Severity

P1 - it has been reported in the wild. There is a workaround but it is also a loss off common functionality.

Additional context

image
@zbodi74 zbodi74 added Type:Bug Product defects Querying/Native The SQL/native query editor Querying/GUI Query builder catch-all, including simple mode .Needs Triage .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. labels Oct 25, 2023
@ranquild ranquild added .Team/QueryProcessor :hammer_and_wrench: Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness .Wanted: MLv2 Issues that will be fixed (or easier to fix, or possible to fix) when we have MLv2 .Backend and removed .Needs Triage labels Oct 26, 2023
@ranquild ranquild self-assigned this Nov 8, 2023
@ranquild
Copy link
Contributor

ranquild commented Nov 8, 2023

@zbodi74 I cannot reproduce in 1.47.5 or in master. Could you please double check the steps?

@ranquild ranquild removed their assignment Nov 8, 2023
@oisincoveney
Copy link
Contributor

I'm also unable to reproduce this in 0.47.5 or master.

P1 - it has been reported in the wild. There is a workaround but it is also a loss off common functionality.

@zbodi74 Could you point us where people might be experiencing this? It might help us reproduce the issue more easily

@zbodi74
Copy link
Author

zbodi74 commented Nov 8, 2023

@ranquild, @oisincoveney - here is a model that shows the problem: https://stats.metabase.com/collection/1360-gh-35039 .
The error did not occur right after I created the model. I then updated SQL and saved the model a second time, and now it shows the problem.

@uladzimirdev
Copy link
Contributor

I'm not able to reproduce it as well @zbodi74 could you please explain a bit more

I then updated SQL and saved the model a second time, and now it shows the problem.

@zbodi74
Copy link
Author

zbodi74 commented Nov 8, 2023

I couldn't figure out why but sometimes the error occurs immediately after I create the model, while other times it appears only after I update and save it for the second time.

@lbrdnk
Copy link
Contributor

lbrdnk commented Feb 15, 2024

I'm able to reproduce on both 1.47.5 and master d419e9a.

Loom with reproduction that seems to show the error reliably also on master.


One of the requests made during QB init is GET http://localhost:30034/api/card/<id>. In erroneous case response is missing result_metadata. That's further to be investigated.

kamilmielnik added a commit that referenced this issue Feb 27, 2024
kamilmielnik added a commit that referenced this issue Feb 28, 2024
* Keep result-metadata of native queries if possible

Part of #37009.

* Fix typo

* Add a fallback for when resultMetadata is not available

* Revert "Add a fallback for when resultMetadata is not available"

This reverts commit 5ad3a8b.

* Move resultsMetadata logic into getSubmittableQuestion

* Add a fallback for when resultMetadata is not available

* Add explanatory comment

* Guard against missing resultsMetadata

* Add repro for #35039

* Simplify the test

* Add an explanatory comment

* Fix test

* Rename promise to runQuestionPromise

---------

Co-authored-by: Tamás Benkő <tamas@metabase.com>
@kamilmielnik kamilmielnik added this to the 0.49 milestone Feb 28, 2024
kamilmielnik added a commit that referenced this issue Feb 28, 2024
* Keep result-metadata of native queries if possible

Part of #37009.

* Fix typo

* Add a fallback for when resultMetadata is not available

* Revert "Add a fallback for when resultMetadata is not available"

This reverts commit 5ad3a8b.

* Move resultsMetadata logic into getSubmittableQuestion

* Add a fallback for when resultMetadata is not available

* Add explanatory comment

* Guard against missing resultsMetadata

* Add repro for #35039

* Simplify the test

* Add an explanatory comment

* Fix test

* Rename promise to runQuestionPromise

---------

Co-authored-by: Tamás Benkő <tamas@metabase.com>
kamilmielnik added a commit that referenced this issue Feb 28, 2024
* Keep result-metadata of native queries if possible

Part of #37009.

* Fix typo

* Add a fallback for when resultMetadata is not available

* Revert "Add a fallback for when resultMetadata is not available"

This reverts commit 5ad3a8b.

* Move resultsMetadata logic into getSubmittableQuestion

* Add a fallback for when resultMetadata is not available

* Add explanatory comment

* Guard against missing resultsMetadata

* Add repro for #35039

* Simplify the test

* Add an explanatory comment

* Fix test

* Rename promise to runQuestionPromise

---------

Co-authored-by: Tamás Benkő <tamas@metabase.com>
kamilmielnik added a commit that referenced this issue Feb 28, 2024
* Keep result-metadata of native queries if possible

Part of #37009.

* Fix typo

* Add a fallback for when resultMetadata is not available

* Revert "Add a fallback for when resultMetadata is not available"

This reverts commit 5ad3a8b.

* Move resultsMetadata logic into getSubmittableQuestion

* Add a fallback for when resultMetadata is not available

* Add explanatory comment

* Guard against missing resultsMetadata

* Add repro for #35039

* Simplify the test

* Add an explanatory comment

* Fix test

* Rename promise to runQuestionPromise

---------

Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
Co-authored-by: Tamás Benkő <tamas@metabase.com>
@kamilmielnik
Copy link
Contributor

Reopening. Fix will be reverted. See Slack discussion.

@kamilmielnik kamilmielnik reopened this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Backend Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/GUI Query builder catch-all, including simple mode Querying/Native The SQL/native query editor .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. .Team/QueryProcessor :hammer_and_wrench: Type:Bug Product defects .Wanted: MLv2 Issues that will be fixed (or easier to fix, or possible to fix) when we have MLv2
Projects
None yet
6 participants