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

Catalog function fixes #6

Merged
merged 5 commits into from
May 11, 2018
Merged

Catalog function fixes #6

merged 5 commits into from
May 11, 2018

Conversation

bpintea
Copy link
Collaborator

@bpintea bpintea commented May 10, 2018

This PR introduces two changes:

  • when the client-app asks for current catalog (SQLGetConnectAttr(attribute=SQL_ATTR_CURRENT_CATALOG)), the driver will now ask ES/SQL for the list of catalogs (using the new SYS CATALOGS) and return the first result as the "current" one. (There's currently always one single catalog returned, the cluster the plug-in runs on.)

  • the table type parameter given in SQLTables() can be a list of quoted or unquoted tokens. ES/SQL supports the quoted version only (second point in SQL: SYS TABLES adjustments for ODBC: TYPE argument elasticsearch#30398). The driver will now do the quoting of the tokens if needed.

bpintea added 4 commits May 9, 2018 19:25
The application can get(/set) the current catalog of a connection. 'SYS
CATALOGS' query is used now to ask for the list of available catalogs
and the first entry is returned as "current".
must have been removed by mistake, instead of training WS.
The spec allows for this argument to contain a list of table types to
filter by. The list of types can be given as quoted or unquoted
tokens. ES/SQL only suppots the quoted synthax, so do the quoting in the
driver.
Note: no sanity check done by the driver, though (i.e. mixed
quoted/unquoted or invalid characters). This isn't done for the other
arguments either.
@bpintea bpintea requested review from droberts195 and edsavage May 10, 2018 12:49
@bpintea bpintea mentioned this pull request May 10, 2018
Closed
#define SYS_CATALOGS \
"SYS CATALOGS"

/* SYS TABLES synthax tokens; these need to stay broken down, since this
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: no 'h' in syntax

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks.


/* In this argument, "each value can be enclosed in single quotation
* marks (') or unquoted" => quote if not quoted (see GH#30398). */
if (! wcsnstr(type, cnt_typ, L'\'')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that some of the tokens could be quoted already but not others? If that is possible then this won't work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is, but that would be an illegal syntax.

The driver does a simple scan for ' in the received parameter and if none is found, it applies the quoting. If some tokens would be quoted but others not, the driver would pass the argument as is to the plug-in (which would then fail the query due to wrong syntax).
The application could generate illegal SQL in many ways (ex. 'TABLE',,'ALIAS'). But the driver doesn't validate these, since ES/SQL will anyways.

SQLWCHAR wbuf[sizeof(SQL_TABLES) + 2 * ESODBC_MAX_IDENTIFIER_LEN];
SQLWCHAR *table, *schema, *catalog;
size_t cnt_tab, cnt_sch, cnt_cat, pos;
/* b/c declaring an array with a const doesn't work with MSVC's compiler */
Copy link
Contributor

@droberts195 droberts195 May 10, 2018

Choose a reason for hiding this comment

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

Yes, with Visual Studio you're stuck with C90 aka C89. The background is here in case you didn't know why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Nice article.

if (pos <= 0) {
ERRH(stmt, "failed to print 'tables' catalog SQL.");
RET_HDIAGS(stmt, SQL_STATE_HY000);
#if 1 // GH#30398
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be removed before merging the PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, I have removed it.

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@edsavage edsavage left a comment

Choose a reason for hiding this comment

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

Also LGTM

@bpintea bpintea merged commit 0ac9b79 into elastic:master May 11, 2018
@bpintea bpintea deleted the feature/catalog_fixes branch May 11, 2018 10:04
bpintea added a commit that referenced this pull request Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants