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

v.db.select: fix escaping SQL columns names with SQL standard double quotes #3614

Closed
wants to merge 4 commits into from

Conversation

tmszi
Copy link
Member

@tmszi tmszi commented Apr 17, 2024

Fix escaping SQL columns names with SQL standard double quotes if v.db.select module columns param arg is used.

Example for testing purpose.

v.db.select module with columns param arg should sucessfully return columns data in the case when column name order is DB reserved keyword:

GRASS hybas/PERMANENT:~ > v.db.select hybas_lake_eu_lev01_v1c column=cat,order
cat|order
1|0

@tmszi tmszi added this to the 8.4.0 milestone Apr 17, 2024
@tmszi tmszi self-assigned this Apr 17, 2024
@github-actions github-actions bot added vector Related to vector data processing C Related code is in C module labels Apr 17, 2024
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

This is a issue in couple places and I'm happy to see this addressed at least here. On the other hand, I'm not sure how the different systems handle quoting. It seems that while standard is a double quote, what is enabled by default and what double quote actually means may differ. More analysis may be required.

The previous implementation was passing the values as is, so one could pass the quoting characters and it would work. This will not work with the current code in this PR.

A test would be really good.

vector/v.db.select/main.c Outdated Show resolved Hide resolved
vector/v.db.select/main.c Outdated Show resolved Hide resolved
@marisn
Copy link
Contributor

marisn commented Apr 17, 2024

On the other hand, I'm not sure how the different systems handle quoting. It seems that while standard is a double quote, what is enabled by default and what double quote actually means may differ. More analysis may be required.

Most of systems GRASS supports are SQL standards compliant. MySQL compliance is addressed in #3612. This PR also needs an update to documentation – mention that quoting column parameter is not necessary any more.

At the moment only DBF driver is broken – it needs some magick in sqlp.l file (any lex experts reading this?)

@github-actions github-actions bot added CI Continuous integration temporal Related to temporal data processing Python Related code is in Python database Related to database management libraries tests Related to Test Suite labels Apr 18, 2024
@tmszi tmszi force-pushed the fix-v_db_select-escaping-cols-names branch from efd580c to f5ba5ba Compare April 18, 2024 06:07
@tmszi
Copy link
Member Author

tmszi commented Apr 18, 2024

A test would be really good.

Test has been added.

@tmszi
Copy link
Member Author

tmszi commented Apr 19, 2024

With this changes another v.dissolve module pytests fails. According my analysis, with automatically enclose column names with double quotes when v.db.select module param columns and group is used, it will require much more code changes (handle SQL func, module JSON output format) and increase complexity of code.

If v.db.select require to use columns or group param with column name which is DB backend reserved keyword it will be much easier enclose this column with double quotes on the param arg level as before.

Example with order column name:

GRASS hybas/PERMANENT:~ > v.db.select map=data_point column='cat,"order",count("order")' format=json
WARNING: column 'count("order")' : type BIGINT is stored as integer (4
         bytes) some data may be damaged
{"info":
{"columns":[
{"name":"cat","sql_type":"INTEGER","is_number":true},
{"name":"order","sql_type":"INTEGER","is_number":true},
{"name":"count("order")","sql_type":"INTEGER","is_number":true}
]},
"records":[
{"cat":1,"order":1,"count("order")":2}
]}

@tmszi tmszi closed this Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C Related code is in C CI Continuous integration database Related to database management libraries module Python Related code is in Python temporal Related to temporal data processing tests Related to Test Suite vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants