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.

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

Type conversion #14

Merged
7 commits merged into from
Dec 8, 2021
Merged

Type conversion #14

7 commits merged into from
Dec 8, 2021

Conversation

ghost
Copy link

@ghost ghost commented Dec 4, 2021

Description

Modified the ODBC Driver to recognize support for conversion of OpenSearch types:

  • Integer
  • Booleam
  • String
  • Double
  • Float
  • Long
  • Date

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ghost ghost requested review from kylepbit, Yury-Fridlyand and raymond-lum December 4, 2021 01:02
kylepbit
kylepbit reviewed Dec 7, 2021
View reviewed changes
sql-odbc/src/sqlodbc/info.c Outdated
#endif /* UNICODE_SUPPORT */
len = sizeof(SQLUINTEGER);
value = 0; /* CONVERT is unavailable */
break;

case SQL_CONVERT_INTEGER: /* ODBC 1.0 */
len = sizeof(SQLUINTEGER);
value = SQL_CVT_BIT | SQL_CVT_WVARCHAR | SQL_CVT_DOUBLE | SQL_CVT_BIGINT | SQL_CVT_REAL;
Copy link

Choose a reason for hiding this comment

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

I think you're also missing the ability to convert to the same type, ex integer -> integer, and so on?

Copy link

Choose a reason for hiding this comment

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

Also ,should what about SQL_CVT_VARCHAR in addition to SQL_CVT_WVARCHAR?

Copy link

Choose a reason for hiding this comment

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

Should SQL_CVT_SMALLINT or SQL_CVT_TINYINT also be here? (I'm assuming those are valid constants, it's possible they are not).

Copy link
Author

Choose a reason for hiding this comment

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

I will add conversion for the same types.
SQLGetTypeInfo does not include the value for type SQL_CVT_VARCHAR so I assume it is not supported.

Copy link
Author

Choose a reason for hiding this comment

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

SMALLINT and TINYINT are byte and short for OpenSearch and casting to byte, short, SMALLINT, and TINYINT do not work.

Copy link

Choose a reason for hiding this comment

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

I see SQL_CVT_VARCHAR here: https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgetinfo-function?view=sql-server-ver15 ?

Copy link

Choose a reason for hiding this comment

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

Note that my comment around SMALLINT and TINYINT are for all conversions, are they unsupported for all?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I meant I could not find the varchar value when I run SQLGetTypeInfo in ODBCTest
image

Copy link
Author

Choose a reason for hiding this comment

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

As of right now, I don't think they are supported.
image

Yury-Fridlyand
Yury-Fridlyand requested changes Dec 7, 2021
View reviewed changes
Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

I thinks you need to split changed into two different branches. One branch should contain changes in the driver only and the second one - changes in PBI connector.

Yury-Fridlyand
Yury-Fridlyand approved these changes Dec 8, 2021
View reviewed changes
Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

LGFM.

kylepbit
kylepbit approved these changes Dec 8, 2021
View reviewed changes
sql-odbc/src/IntegrationTests/ITODBCInfo/test_odbc_info.cpp Outdated
TEST_SQL_GET_INFO_UINT_MASK(SQLConvertDecimal, SQL_CONVERT_DECIMAL, 0);
TEST_SQL_GET_INFO_UINT_MASK(SQLConvertDouble, SQL_CONVERT_DOUBLE, 0);
// 8409288 = 0x008050C8 = SQL_CVT_INTEGER | SQL_CVT_REAL | SQL_CVT_DOUBLE | SQL_CVT_BIT | SQL_CVT_BIGINT | SQL_CVT_WVARCHAR
TEST_SQL_GET_INFO_UINT_MASK(SQLConvertDouble, SQL_CONVERT_DOUBLE, 8409288);
Copy link

Choose a reason for hiding this comment

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

Instead of documenting this via a comment, you can simply put the bitmask into the value.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in 284376e

Change values to bitmasks
284376e 
Signed-off-by: Guian Gumpac <[email protected]>
@ghost ghost merged commit 8b4e6d2 into powerbi-main Dec 8, 2021
@ghost ghost deleted the type-conversion branch December 8, 2021 19:37
matthewryanwells pushed a commit that referenced this pull request Aug 17, 2023
@MitchellGale @Yury-Fridlyand @GumpacG
[Spotless] Applying Google Code Format for protocol files #14 (opense…
1744d8a 
…arch-project#1971)

* Spotless apply on protocol

Signed-off-by: Mitchell Gale <[email protected]>

* added ignorefailures

Signed-off-by: Mitchell Gale <[email protected]>

* Update protocol/src/main/java/org/opensearch/sql/protocol/response/format/RawResponseFormatter.java

Co-authored-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Mitchell Gale <[email protected]>

* Apply suggestions from code review

Co-authored-by: Guian Gumpac <[email protected]>
Signed-off-by: Mitchell Gale <[email protected]>

---------

Signed-off-by: Mitchell Gale <[email protected]>
Signed-off-by: Mitchell Gale <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Guian Gumpac <[email protected]>
MitchellGale added a commit that referenced this pull request Aug 22, 2023
@MitchellGale @Yury-Fridlyand @GumpacG
[Spotless] Applying Google Code Format for protocol files #14 (opense…
b3029c7 
…arch-project#1971)

* Spotless apply on protocol

Signed-off-by: Mitchell Gale <[email protected]>

* added ignorefailures

Signed-off-by: Mitchell Gale <[email protected]>

* Update protocol/src/main/java/org/opensearch/sql/protocol/response/format/RawResponseFormatter.java

Co-authored-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Mitchell Gale <[email protected]>

* Apply suggestions from code review

Co-authored-by: Guian Gumpac <[email protected]>
Signed-off-by: Mitchell Gale <[email protected]>

---------

Signed-off-by: Mitchell Gale <[email protected]>
Signed-off-by: Mitchell Gale <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Guian Gumpac <[email protected]>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raymond-lum raymond-lum Awaiting requested review from raymond-lum

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kylepbit @Yury-Fridlyand