Skip to content

Conversation

@stepansergeevitch
Copy link
Collaborator

Added support for bytea type (both parsing and formatting)

@stepansergeevitch stepansergeevitch self-assigned this Feb 8, 2023
@stepansergeevitch stepansergeevitch changed the title Fir 16953 bytea support feat: Fir 16953 bytea support Feb 8, 2023
Copy link
Contributor

@ptiurin ptiurin left a comment

Choose a reason for hiding this comment

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

Otherwise, looks good

Comment on lines 226 to 237
BYTEA_PREFIX = "\\x"


def _parse_bytea(str_value: str) -> bytes:
return bytes.fromhex(str_value[len(BYTEA_PREFIX) :])
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check if the string starts with the correct prefix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My idea was that it might be a pretty heavy operation on a large scale (since string slice is basically copy). Yet, I think we should be fine in a general case, we have less performant pieces anyway

Comment on lines 508 to 511
data = "bytea_123"

await c.execute(
"INSERT INTO test_bytea_roundtrip VALUES (1, ?)", (Binary(data),)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a couple of special characters here as well, just to make sure we handle them correctly. e.g. '\n', '\', ヽ༼ຈل͜ຈ༽ノ

'decimal(38,30)) as "decimal", '
"cast(null as int) as nullable"
"cast(null as int) as nullable, "
"'abc123'::bytea as bytea"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a heads-up, unquoted identifiers are not supposed to work and SQL team has just released a fix. We should change this to as "bytea" and any other new types that worked this way.

@stepansergeevitch stepansergeevitch force-pushed the FIR-16953-bytea-support branch from 1078992 to 258112a Compare March 1, 2023 08:58
@stepansergeevitch stepansergeevitch force-pushed the FIR-16953-bytea-support branch from c026bf7 to 5f2660e Compare March 10, 2023 09:44
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@stepansergeevitch stepansergeevitch merged commit 2e70667 into main Mar 10, 2023
@stepansergeevitch stepansergeevitch deleted the FIR-16953-bytea-support branch March 10, 2023 09:56
stepansergeevitch added a commit that referenced this pull request Mar 10, 2023
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