Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

ODBC: Adding BASIC & AWS_SIGV4 auth in M Connector #610

Merged

Conversation

rupal-bq
Copy link
Contributor

Issue #595

Description of changes:

  • Added BASIC auth
  • Added AWS_SIGV4 auth
  • Added unit test for checking the connection
  • Added error handling

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Rupal Mahajan added 19 commits July 18, 2020 21:01
- add docs
- add unit test
- add errors
- enable direct query
…e/odbc/pbi

# Conflicts:
#	sql-odbc/.gitignore
#	sql-odbc/src/PowerBIConnector/OdfeSqlOdbcPBIConnector.mproj
#	sql-odbc/src/PowerBIConnector/OdfeSqlOdbcPBIConnector.pq
#	sql-odbc/src/PowerBIConnector/OdfeSqlOdbcPBIConnector.query.pq
#	sql-odbc/src/PowerBIConnector/bin/Release/OdfeSqlOdbcPBIConnector.mez

// Add support for `LIMIT` and `OFFSET` clauses (rather than `TOP`)
AstVisitor = [
// format is "LIMIT [<skip>,]<take>" - ex. LIMIT 2,10 or LIMIT 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you verify whether ODFE SQL supports this [skip,]take syntax? If it doesn't, then we may want to change this to LIMIT <take> OFFSET <skip>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[skip,]take syntax is supported

GroupByCapabilities = ODBC[SQL_GB][SQL_GB_COLLATE]
]),

SQLGetInfo = Diagnostics.LogValue("SQLGetInfo_Options", [
Copy link
Contributor

Choose a reason for hiding this comment

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

These are values that we can change at the driver level. Unless those changes break other applications, lets set Capabilities and GetInfo values there instead

Can you remove these lines (SQLCapabilities & SQLGetInfo) and verify if we get the same connection behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed SQLCapabilities & SQLGetInfo since all required values are defined at the driver level

]
],

OdbcDatasource = Odbc.DataSource(ConnectionString & Server & CredentialConnectionString, [
Copy link

Choose a reason for hiding this comment

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

Shouldn't this concatenate EncryptedConnectionString too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

],

// Enable Encryption
SupportsEncryption = true,
Copy link

Choose a reason for hiding this comment

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

Note that if you set this to true, PBI will always try encrypted connections. You need to add an error handler that tells PBI to try an unencrypted connection if it gets an SSL error. See the ImpalaODBC example on the PowerQuery SDK github.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will include that in separate PR since some changes are required in driver code to return proper error code for SSL.

@chloe-zh chloe-zh merged commit 9dac0ac into opendistro-for-elasticsearch:develop Jul 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants