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

Use sqlalchemy's 'quote' function to quote table names and fix table quoting in cfr exporter #172

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

seut
Copy link
Member

@seut seut commented Jun 13, 2024

See commits.

Closes #168.

@seut seut requested a review from amotl June 13, 2024 15:32
@seut seut removed the request for review from amotl June 13, 2024 15:40
@seut
Copy link
Member Author

seut commented Jun 13, 2024

Moving back to draft as there seem to be issues.

@seut seut marked this pull request as draft June 13, 2024 15:41
@seut seut marked this pull request as ready for review June 13, 2024 16:07
@amotl
Copy link
Member

amotl commented Jun 13, 2024

Moving back to draft as there seem to be issues.

It might be totally unrelated, but it also might be related to unfortunate side-effects of shortcomings within the test suite, as only three test cases are failing. Let me bring in this patch first, and then have another look after rebasing.

@seut
Copy link
Member Author

seut commented Jun 13, 2024

It might be totally unrelated, but it also might be related to unfortunate side-effects of shortcomings within the test suite, as only three test cases are failing. Let me bring in this patch first, and then have another look after rebasing.

* [Dialect: Fix `get_table_names()` reflection method sqlalchemy-cratedb#10](https://github.com/crate-workbench/sqlalchemy-cratedb/pull/10)

Yes I think it's unrelated. This PR should not have issues anymore now.

@seut seut requested a review from amotl June 13, 2024 16:10
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.33%. Comparing base (6be34c3) to head (642b025).

Files Patch % Lines
cratedb_toolkit/util/database.py 78.57% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #172       +/-   ##
===========================================
- Coverage   80.71%   68.33%   -12.38%     
===========================================
  Files          68       68               
  Lines        2712     2716        +4     
===========================================
- Hits         2189     1856      -333     
- Misses        523      860      +337     
Flag Coverage Δ
influxdb ?
main 68.33% <81.25%> (+0.04%) ⬆️
mongodb ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines 10 to 11
assert database.quote_ident("my_schema.my_table") == "my_schema.my_table"
assert database.quote_ident("my-schema.my_table") == '"my-schema".my_table'
Copy link
Member

@amotl amotl Jun 13, 2024

Choose a reason for hiding this comment

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

What about this case and friends? a) Does it work? b) Does it deserve a dedicated test case?

assert database.quote_ident('"my_schema"."my_table"') == "my_schema.my_table"

Copy link
Member Author

Choose a reason for hiding this comment

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

The current logic will not remove such valid quotes, why should it?

@amotl
Copy link
Member

amotl commented Jun 13, 2024

Unrelated to this patch specifically, but absolutely related to the same topic, see also that upcoming patch for the SQLAlchemy dialect implementation.

We will be happy to have your voice on the relevant discussion, when applicable.

@seut
Copy link
Member Author

seut commented Jun 14, 2024

I think we should rename the quote_ident function to quote_relation_name to express it's difference from DB's usual quote_ident functions.
As written at crate/sqlalchemy-cratedb#69 (comment), a quote or quote_identifier function is generally not able and intended to work on full-qualified (relation/column) names but only on the unqualified identifier part.

@seut
Copy link
Member Author

seut commented Jun 14, 2024

@amotl
I've added a sanity check at the quoting function and also renamed it to express better what it does, pls check.

@seut seut requested a review from amotl June 14, 2024 07:52
@seut seut merged commit 38e6097 into main Jun 14, 2024
7 checks passed
@seut seut deleted the s/quoted_name branch June 14, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[infra] Improve quote_table_name to accept full-qualified table identifiers
2 participants