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

Feat/pg semicolon support #599

Merged
merged 5 commits into from
Apr 9, 2024

Conversation

Jordan-M-Young
Copy link
Contributor

Implementing support for finishing postgres queries with a semicolon per this issue.

Changes include:
- replace statements to remove semicolon from query in connectorx-python/connectorx/init.py (read_sql)
- Tests to confirm that command terminating semicolons are being removed.

@Jordan-M-Young Jordan-M-Young marked this pull request as draft April 4, 2024 16:37
@Jordan-M-Young Jordan-M-Young marked this pull request as ready for review April 4, 2024 18:59
@Jordan-M-Young
Copy link
Contributor Author

@wangxiaoying I think this one is ready for review. Let me know if/what changes are needed. Thanks in advance!

@wangxiaoying
Copy link
Contributor

Hi @Jordan-M-Young , thanks for the PR!

I think my concern is that ';' may also appears in other parts of the query, such as string matching. For example: select s from t where s like '%abc;'. In this case, simply replace ";" with "" will change the semantic of the sql query.

I would suggest to only replace the ";" if it is the last valid character in the sql query. Such as select s from t; or select s from t;[spaces] but not select s from t where s like '%abc;'

@Jordan-M-Young
Copy link
Contributor Author

@wangxiaoying Thanks for the suggestion. I believe my newest commit should allay that concern. If you wouldn't mind taking another look I'd appreciate it.

@wangxiaoying wangxiaoying merged commit e1b050c into sfu-db:main Apr 9, 2024
1 check passed
@wangxiaoying
Copy link
Contributor

wangxiaoying commented Apr 9, 2024

Thank you @Jordan-M-Young ! I have merged the PR.

@@ -377,3 +385,11 @@ def reconstruct_pandas(df_infos: Dict[str, Any]):
)
df = pd.DataFrame(block_manager)
return df


def remove_ending_semicolon(query: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit cumbersome. Could be rewritten to:

if query.endswith(';'):
    query = query[:-1]
return query

Copy link
Contributor

@wangxiaoying wangxiaoying Apr 11, 2024

Choose a reason for hiding this comment

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

Thanks @surister , indeed this way can be better. Can you open a PR for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

You got it.

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.

3 participants