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.

Sign up for GitHub

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

Jump to bottom

fix conversion of varchar to binary varbinary and vice versa #1957

Merged

Deepesh125 merged 32 commits into babelfish-for-postgresql:BABEL_3_X_DEV from tanscorpio7:BABEL_1940

Nov 10, 2023

Contributor

tanscorpio7 commented Oct 26, 2023 •

edited

Description

Binary data type simply stores hex codes but when transformed to and from varchar, use of correct encoding becomes important. For example the symbol '™' is stored as
0xE284A2 in UTF-8 enc. while
0x99 in Win encoding
From users perspective they must see the hex value which is congruent to their server encoding.

To fix this we do necessary encoding to source data in varchar <--> varbinary internal functions.
Length checks should be done when data (hex string) is in server encoding.

We must also handle the case for string literal to binary types. So we explicitly call the varcharvarbinary conversion when a string literal is being casted to binary data type.

Also added CAST functions for sys.BBF_VARBINARY to sys.BBF_BINARY and vice versa
these cast are being used in geography and geometry cast functions
made necessary changes to geography and geometry casts as well
i.e. CAST (CAST ($1 AS sys.VARCHAR) AS sys.bbf_varbinary) --> CAST ($1 AS sys.bbf_varbinary)

Engine PR: babelfish-for-postgresql/postgresql_modified_for_babelfish#248

Extension PR: #1957

Issues Resolved

[BABEL-1940]

Sign-off

Signed-off-by: Tanzeel Khan tzlkhan@amazon.com
Co-authored-by: Rohit Bhagat rohitbgt@amazon.com

Check List

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

tanscorpio7 added 3 commits

October 26, 2023 04:43


          initial commit

982793f

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          fix indent

745aaeb

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          allow 0x00 when encoding to UTF-8

d9391a2

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

tanscorpio7 requested review from Deepesh125 and sumitj824

October 26, 2023 09:16

Contributor

sumitj824 commented Oct 26, 2023

Please add tests.

tanscorpio7 added 2 commits

October 26, 2023 14:00


          add tests

cd3bbfc

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          correct upgrade tests for babel_function_string

d2db3fb

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

Deepesh125 reviewed

View reviewed changes

contrib/babelfishpg_common/src/varbinary.c Outdated Show resolved Hide resolved

Deepesh125 reviewed

View reviewed changes

test/JDBC/expected/BABEL_1940.out Outdated Show resolved Hide resolved

sumitj824 reviewed

View reviewed changes

test/JDBC/expected/BABEL_1940.out Outdated Show resolved Hide resolved


          update tests

3011a28

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

Deepesh125 requested changes

View reviewed changes

contrib/babelfishpg_common/src/encoding/mb/conv.c Outdated Show resolved Hide resolved

contrib/babelfishpg_tsql/src/pltsql_coerce.c Outdated Show resolved Hide resolved

contrib/babelfishpg_tsql/src/pltsql_coerce.c Outdated Show resolved Hide resolved

test/JDBC/input/babel_function_string-before-15-5-or-14-10-vu-verify.sql Show resolved Hide resolved

tanscorpio7 added 3 commits

November 2, 2023 14:38


          Merge branch 'BABEL_3_X_DEV' into BABEL_1940

a8ddfae


          use correct func to build typename

c05919d

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          empty commit rerun actions

7f9da36

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

tanscorpio7 requested a review from Deepesh125

November 2, 2023 16:25

Deepesh125 requested review from rohit01010 and sumitj824

November 3, 2023 07:15


          release syscache tuples

e033ba4

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

tanscorpio7 mentioned this pull request

release sys cache reference after string literal hook babelfish-for-postgresql/postgresql_modified_for_babelfish#248

Merged

1 task

tanscorpio7 added 3 commits

November 3, 2023 09:30


          empty commit rerun actions post engine changes

e948fd0

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          rerun actions with empty commit

cdd4e35

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          add few more tests

62729cf

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

Deepesh125 reviewed

View reviewed changes

test/JDBC/expected/BABEL_1940.out Outdated Show resolved Hide resolved

Contributor

rohit01010 commented Nov 3, 2023

1> SELECT CONVERT(VARBINARY(10), CONVERT(VARCHAR(10), 0x330033))
2> go
Msg 33557097, Level 16, State 1, Server BABELFISH, Line 1
invalid byte sequence for encoding "UTF8": 0x00

Getting this above error with the changes from this PR, Kindly resolve and add this query to test-cases

tanscorpio7 added 4 commits

November 7, 2023 09:41


          rerun actions with empty commit

a193211

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          Merge remote-tracking branch 'upstream/BABEL_3_X_DEV' into BABEL_1940

d2e85f6


          empty commit

d16c126

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          reject null byte

5f4507c

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

Deepesh125 reviewed

View reviewed changes

contrib/babelfishpg_common/src/varbinary.c Show resolved Hide resolved

Deepesh125 requested changes

View reviewed changes

contrib/babelfishpg_common/src/varbinary.c Show resolved Hide resolved

contrib/babelfishpg_common/src/varbinary.c Outdated Show resolved Hide resolved

test/JDBC/expected/BABEL_1940.out Show resolved Hide resolved

tanscorpio7 added 5 commits

November 9, 2023 20:08


          fix typmod for binary data type

25fda37

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          remove typmod fix for now

b768e82

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          push correct test out file

d944eb0

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          just comments

f2e7f81

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>


          add some varbinary binary test case

278c0d5

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

Deepesh125 previously approved these changes

View reviewed changes

contrib/babelfishpg_common/src/varbinary.c Outdated Show resolved Hide resolved

contrib/babelfishpg_common/src/varbinary.c Outdated Show resolved Hide resolved


          some minor refactoring

0bbf3c8

Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

tanscorpio7 dismissed Deepesh125’s stale review via

0bbf3c8

November 10, 2023 07:44

tanscorpio7 changed the title ~~Varchar Varbinary inter conversion~~ fix conversion of varchar to binary varbinary and vice versa

tanscorpio7 requested a review from Deepesh125

November 10, 2023 09:10

Deepesh125 approved these changes

View reviewed changes

tanscorpio7 changed the title ~~fix conversion of varchar to binary varbinary and vice versa~~ proper release sys cache reference after string literal hook

tanscorpio7 changed the title ~~proper release sys cache reference after string literal hook~~ release sys cache reference after string literal hook

tanscorpio7 changed the title ~~release sys cache reference after string literal hook~~ fix conversion of varchar to binary varbinary and vice versa

Deepesh125 pushed a commit to babelfish-for-postgresql/postgresql_modified_for_babelfish that referenced this pull request


          fix conversion of varchar to binary varbinary and vice versa (#248)

0f583ef

Release syscache reference when returning not null nodes from string literal hook to avoid cache reference leak

Engine PR: #248
Extension PR: babelfish-for-postgresql/babelfish_extensions#1957

Task: 1940
Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

Deepesh125 merged commit 62546c5 into babelfish-for-postgresql:BABEL_3_X_DEV

28 checks passed

tanscorpio7 added a commit to tanscorpio7/postgresql_modified_for_babelfish that referenced this pull request


          fix conversion of varchar to binary varbinary and vice versa (babelfi…

3ec0382

…sh-for-postgresql#248)

Release syscache reference when returning not null nodes from string literal hook to avoid cache reference leak

Engine PR: babelfish-for-postgresql#248
Extension PR: babelfish-for-postgresql/babelfish_extensions#1957

Task: 1940
Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

tanscorpio7 mentioned this pull request

release sys cache reference after string literal hook babelfish-for-postgresql/postgresql_modified_for_babelfish#254

Merged

1 task

Deepesh125 pushed a commit to babelfish-for-postgresql/postgresql_modified_for_babelfish that referenced this pull request


          fix conversion of varchar to binary varbinary and vice versa (#254)

474e445

Release syscache reference when returning not null nodes from string literal hook to avoid cache reference leak

Engine PR: #248
Extension PR: babelfish-for-postgresql/babelfish_extensions#1957

Task: 1940
Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

tanscorpio7 mentioned this pull request

fix upgrade script for new bbf_binary casts #2010

Merged

1 task

Sairakan pushed a commit to amazon-aurora/postgresql_modified_for_babelfish that referenced this pull request


          fix conversion of varchar to binary varbinary and vice versa (babelfi…

3377d2d

…sh-for-postgresql#248)

Release syscache reference when returning not null nodes from string literal hook to avoid cache reference leak

Engine PR: babelfish-for-postgresql#248
Extension PR: babelfish-for-postgresql/babelfish_extensions#1957

Task: 1940
Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

Sairakan pushed a commit to amazon-aurora/postgresql_modified_for_babelfish that referenced this pull request


          fix conversion of varchar to binary varbinary and vice versa (babelfi…

5dad5ed

…sh-for-postgresql#248)

Release syscache reference when returning not null nodes from string literal hook to avoid cache reference leak

Engine PR: babelfish-for-postgresql#248
Extension PR: babelfish-for-postgresql/babelfish_extensions#1957

Task: 1940
Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

deepakshi-mittal pushed a commit to amazon-aurora/postgresql_modified_for_babelfish that referenced this pull request


          fix conversion of varchar to binary varbinary and vice versa (babelfi…

0e21a2f

…sh-for-postgresql#248)

Release syscache reference when returning not null nodes from string literal hook to avoid cache reference leak

Engine PR: babelfish-for-postgresql#248
Extension PR: babelfish-for-postgresql/babelfish_extensions#1957

Task: 1940
Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

priyansx pushed a commit to amazon-aurora/postgresql_modified_for_babelfish that referenced this pull request


          fix conversion of varchar to binary varbinary and vice versa (babelfi…

1081b7a

…sh-for-postgresql#248)

Release syscache reference when returning not null nodes from string literal hook to avoid cache reference leak

Engine PR: babelfish-for-postgresql#248
Extension PR: babelfish-for-postgresql/babelfish_extensions#1957

Task: 1940
Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

KushaalShroff reviewed

View reviewed changes

contrib/babelfishpg_common/sql/binary.sql

Comment on lines +49 to +54

+              CREATE CAST (sys.BBF_BINARY AS sys.BBF_VARBINARY)
+              WITHOUT FUNCTION AS IMPLICIT;
+              CREATE CAST (sys.BBF_VARBINARY AS sys.BBF_BINARY)
+              WITHOUT FUNCTION AS IMPLICIT;

Contributor

KushaalShroff Nov 28, 2023

No upgrade script changes for these?

tanscorpio7 deleted the BABEL_1940 branch

December 12, 2023 08:28

staticlibs pushed a commit to wiltondb/postgresql_modified_for_babelfish that referenced this pull request


          fix conversion of varchar to binary varbinary and vice versa (babelfi…

…sh-for-postgresql#248)

Release syscache reference when returning not null nodes from string literal hook to avoid cache reference leak

Engine PR: babelfish-for-postgresql#248
Extension PR: babelfish-for-postgresql/babelfish_extensions#1957

Task: 1940
Signed-off-by: Tanzeel Khan <tzlkhan@amazon.com>

staticlibs pushed a commit to wiltondb/babelfish_extensions that referenced this pull request


          fix conversion of varchar to binary varbinary and vice versa (babelfi…

6449ff1

…sh-for-postgresql#1957)

Binary data type simply stores hex codes but when transformed to and from varchar, use of correct encoding becomes important when we cast to and from string data types. For example the symbol '™' is stored as 0xE284A2 in UTF-8 encoding while 0x99 in WIN1252 encoding. From users perspective they must see the hex value which is congruent to their server encoding.

To fix this we do necessary encoding to source data in varchar <--> varbinary internal functions. Length checks should be done when data (hex string) is in server encoding.

We must also handle the case for string literal to binary types. So we explicitly call the varcharvarbinary conversion when a string literal is being casted to binary data type.

Also added CAST functions for sys.BBF_VARBINARY to sys.BBF_BINARY and vice versa. These cast are being used in geography and geometry data types cast functions. Made necessary changes to geography and geometry casts as well
i.e. CAST (CAST ($1 AS sys.VARCHAR) AS sys.bbf_varbinary) --> CAST ($1 AS sys.bbf_varbinary)

Engine PR: babelfish-for-postgresql/postgresql_modified_for_babelfish#248
Extension PR: babelfish-for-postgresql#1957

Task: BABEL-1940
Signed-off-by: Tanzeel Khan tzlkhan@amazon.com
Co-authored-by: Rohit Bhagat rohitbgt@amazon.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment