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

remove escape percent signs from select_sql #16

Closed
wants to merge 4 commits into from

Conversation

dgoo2308
Copy link
Contributor

@dgoo2308 dgoo2308 commented Aug 1, 2021

For table and column names, % should not be written as %% in a select statement.

this results in an error when a column or table has a % character in the name.

INFO syncing vw_SkiveMonthlyStatistic full table
{"type": "SCHEMA", "stream":  ..... "SURFACE_BAD_%":  ......., "key_properties": []}
{"type": "ACTIVATE_VERSION", "stream": "dbo-vw_SkiveMonthlyStatistic", "version": 1627788239240}
INFO METRIC: {"type": "timer", "metric": "job_duration", "value": 0.3792839050292969, "tags": {"job_type": "sync_table", "database": "dbo", "table": "vw_SkiveMonthlyStatistic", "status": "failed"}}
CRITICAL (207, b"Invalid column name 'SURFACE_BAD_%%'.DB-Lib error message 20018, severity 16:\nGeneral SQL Server error: Check messages from the SQL Server\nDB-Lib error message 20018, severity 16:\nGeneral SQL Server error: Check messages from the SQL Server\n")

For table and column names, % should not be written as %% in a select statement.
Copy link

@wmattern0 wmattern0 left a comment

Choose a reason for hiding this comment

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

return statement

@wmattern0
Copy link

wmattern0 commented Jul 26, 2022

Was able to run this code against a MSSQL db with percent signs in column names after making a similar fix done here, but adding back the return stmt (line 90). Running using the code in master was getting the same error as described in the original PR comment.

@s7clarke10
Copy link
Collaborator

As a FYI, the escaping of the % has come from the original mysql code.

https://github.com/transferwise/pipelinewise-tap-mysql/blob/f08a0a1f450b826c07544af77e402ef33ff4ad62/tap_mysql/sync_strategies/common.py#L87

Having read this change, I can see why it has been requested. I do wonder if this line to escape the % is removed, whether it will have other side effects, perhaps mysql just required the % sign to be escaped because it was getting confused with Python variable substitution?

@dgoo2308
Copy link
Contributor Author

dgoo2308 commented Jul 31, 2022

As a FYI, the escaping of the % has come from the original mysql code.

https://github.com/transferwise/pipelinewise-tap-mysql/blob/f08a0a1f450b826c07544af77e402ef33ff4ad62/tap_mysql/sync_strategies/common.py#L87

Having read this change, I can see why it has been requested. I do wonder if this line to escape the % is removed, whether it will have other side effects, perhaps mysql just required the % sign to be escaped because it was getting confused with Python variable substitution?

indeed this seems to be ported from mysq, which needed the % to be escaped to %%, whereas here the MSSQL colomn names are escape with double quotes in stead of back tick in the mysql-tap.

It's clear that with the double quotes the % does not need to be replaced by %% for MSSQL-tap

@dgoo2308
Copy link
Contributor Author

@wintersrd could you consider to add this to the V2?

@wmattern0
Copy link

One issue that I found with using this tap is sending the singer records to target-snowflake(pipelinewise) causes a python error.

At some point it does string formatting on the column names and the '%' in the column name is read as a variable, not as a literal. Rather than moving forward with this tap and target combo, I am giving up and using a different pattern.

@s7clarke10
Copy link
Collaborator

@dgoo2308 , this PR has been merged into main under this change: #39 . Are you happy for this PR to be closed as it has been actioned?

@s7clarke10
Copy link
Collaborator

Closing Pull Request as this has been actioned : #39

@s7clarke10 s7clarke10 closed this Aug 15, 2023
@dgoo2308 dgoo2308 deleted the fix_double%% branch October 23, 2023 11:08
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