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

SQL field, table and schema matching #41781

Merged
merged 4 commits into from
Apr 26, 2024
Merged

SQL field, table and schema matching #41781

merged 4 commits into from
Apr 26, 2024

Conversation

piranha
Copy link
Contributor

@piranha piranha commented Apr 24, 2024

Uses new Macaw to better match Field objects used in an SQL string.

References #36911

@piranha piranha added no-backport Do not backport this PR to any branch .Team/BackendComponents also known as BEC labels Apr 24, 2024
@piranha piranha self-assigned this Apr 24, 2024
@piranha piranha requested a review from camsaul as a code owner April 24, 2024 14:20
:indirect count)))
(is (= 6
(-> (q "select v.* from venues v join checkins")
:indirect count))))))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compelling tests

Copy link
Member

@tsmacdonald tsmacdonald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice to me!

You've confirmed that our understanding of case and quotes is true for all SQL dialects we care about?

@piranha
Copy link
Contributor Author

piranha commented Apr 24, 2024

You've confirmed that our understanding of case and quotes is true for all SQL dialects we care about?

Ehmm... it's a bit shameful, but I've looked at list of supported databases and just asked ChatGPT. It says that most of dialects support SQL Standard - so unquoted names are case insensitive, quoted are case sensitive.

Copy link

replay-io bot commented Apr 24, 2024

Status Complete ↗︎
Commit 48c4945
Results
⚠️ 6 Flaky
2430 Passed

Base automatically changed from macaw-28 to master April 24, 2024 19:28
(#'query-analyzer/field-query :f.name "\"TEST\""))))
(testing "escaping inside quoted fields should be handled properly"
(is (= [:= :f.name "Perv\"e\"rse"]
;; this is "Perv""e""rse"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is indeed perverse :)

@tsmacdonald
Copy link
Member

Oh nice, the tests are legit broken with the case-matching, I think:


FAIL in metabase.models.query-field-test/table-wildcard-test (query_field_test.clj:127)

with temporary :model/Card with attributes
{:creator_id 1,
 :database_id 1,
 :dataset_query {:type :native, :native {:query "SELECT NOT_TAX, TOTAL FROM orders"}, :database 1},
 :display :table,
 :name "OHFYZTENFEEYTCUGLAQY",
 :visualization_settings {},
 :created_at #t "2024-04-25T06:38:59.146814Z[UTC]",
 :updated_at #t "2024-04-25T06:38:59.146815Z[UTC]"}

 mix of select table.* and named columns
expected: 17
  actual: 16

@piranha
Copy link
Contributor Author

piranha commented Apr 26, 2024

Oh nice, the tests are legit broken with the case-matching, I think:

yeah and it actually had your comment about o.id being wrongly guessed, so we've got our results in line. :)

@piranha piranha enabled auto-merge (squash) April 26, 2024 12:40
@piranha piranha merged commit dd66a69 into master Apr 26, 2024
107 checks passed
@piranha piranha deleted the sql-field-case-matching branch April 26, 2024 14:46
Copy link

@piranha Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/BackendComponents also known as BEC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants