catalog: Add find_procedures/find_procedure_columns

[detule]

Hi @lexicalunit @mloskot

This is an implementation of the SQLProcedures and SQLProcedureColumns function calls.

This PR adds:

Methods:

• catalog::find_procedures: Implementation of SQLProcedures, similar to find_tables.
• catalog::find_procedure_columns: Implementation of SQLProcedureColumns, similar to find_columns.

Classes:

• catalog::procedures: Result set from find_procedures; similar to catalog::tables.
• catalog::procedure_columns: Result set from find_columns; similar to catalog::columns.

Looking forward to hearing your feedback.

Thanks

[mloskot]

@detule This generally looks good and thanks for this update.

I don't like the proc_ abbreviation though. We generally avoid abbreviations in the interface names.
I'd simply remove proc_ completely and have procedure_columns::catalog, procedure_columns::schema, procedure_columns::procedure.

One obvious question:

• Would it be useful to re-use the columns class instead of defining the new procedure_columns class?

Technically, it is possible, but the columns interface would have to be tweaked e.g. to avoid table_ prefix which would be confusing in context of the procedures. I should have named those as columns::catalog, columns::schema, columns::table from the start as the table_ prefix is redundant. @lexicalunit I think we can still correct that and polish the interface a bit by removing or deprecating the methods with table_ prefix.

[detule]

Hey @mloskot

• Thanks - expanding proc sounds fine (pending the discussion below).

• On abstracting some of the interface in a parent class that both columns and procedure_columns can inherit from: That's a fair question - it would require some care since for example "remarks" may refer to a column that is indexed differently (and have a different nullity) in the table columns result versus the procedure columns result. We would need an internal mapping to tie attributes to indices; but that's fine, easily done.

  • Arguably we should take this a step further: both "tables", "procedures" have essentially the same interface / methods and would benefit from deprecating table|procedure_catalog in favor of a catalog method. With that in mind, does it make sense to have a generic "database_object" class / interface that "tables", "procedures", "columns", and "procedure_columns" all inherit from?

Finally, as is always the case with OOP, as slick as it may be - is this abstraction worth it? It's not like we'll have a number of database objects on the horizon - should we just keep it simple - stick to separate classes, code repetiton and all, but keep the naming uniform per your suggestion, and slap a deprecated warning in front of the table_* methods?

What do you think?

[mloskot]

@detule

On abstracting some of the interface in a parent class that both columns and procedure_columns can inherit from (...)
Finally, as is always the case with OOP, as slick as it may be - is this abstraction worth it?

I did not suggest to go the OOP way, to have a base class/interface and to inherit from it. Sorry if I wasn't clear.

I suggested to consider single concrete column class that can be used by all catalog queries/find requests that report columns.

It's not like we'll have a number of database objects on the horizon - should we just keep it simple - stick to separate classes, code repetiton and all,

Yes, I'm also in favour of simple concrete classes rather than base interface and inheritance, even if it means code repetition.

but keep the naming uniform per your suggestion, and slap a deprecated warning in front of the table_* methods?

Yes, indeed, for a bunch of similar but separate classes uniform naming is important as it makes up a common coherent interface.
That was supposed to be the major theme of my suggestion too.

[detule]

Thanks @mloskot appreciate the feedback.

In terms of re-using the columns class as-is, I was struggling with the field indexing - it is not the same for procedure- versus table-columns. For example the data_type field, for table columns, comes through in the result indexed at 4, versus for procedure columns it is indexed at 5 In addition the procedure-columns have an additional field in the result-set - column_type (this essentially is the reason why the second half of the indices are one-shifted relative to the indices in the table-columns result set).

At any rate, this is why i thought you might be suggesting an abstraction as a way to unify both. To avoid that and re-use the existing class: I guess we can add index-to-field map as a private member to the class, and when we construct using the result set, here or here, we can inject the correct one.

Is that the direction you were thinking in? Seems less abstract than injecting inheritance and additional class structure; still though we would need to write out the maps, modify the methods to lookup the index of the field they are asked to serve up - there would still be a bit of code churn.

Let me know happy to give it a shot if you think that's best.
Thanks again.

[mloskot]

I reflected on my initial idea and gave it a bit more consideration, and I think there is little point in trying to have single columns for all the cases:

1. There seem to be just three cases: SQLColumns, SQLProcedureColumns and SQLSpecialColumns, so we need just three classes for the result sets.

2. Each of the result sets should also offer access to driver-specific columns (e.g. via generic get<string>), as the documentation says, for each of the above:

   Additional columns beyond column 18 (IS_NULLABLE) can be defined by the driver.
   Additional columns beyond column 19 (IS_NULLABLE) can be defined by the driver.
   Additional columns beyond column 8 (PSEUDO_COLUMN) can be defined by the driver.

3. The result sets are quite different

Index	SQLColumns	SQLProcedureColumns	SQLSpecialColumns
1	TABLE_CAT (ODBC 1.0)	PROCEDURE_CAT (ODBC 2.0)	SCOPE (ODBC 1.0)
2	TABLE_SCHEM (ODBC 1.0)	PROCEDURE_SCHEM (ODBC 2.0)	COLUMN_NAME (ODBC 1.0)
3	TABLE_NAME (ODBC 1.0)	PROCEDURE_NAME (ODBC 2.0)	DATA_TYPE (ODBC 1.0)
4	COLUMN_NAME (ODBC 1.0)	COLUMN_NAME (ODBC 2.0)	TYPE_NAME (ODBC 1.0)
5	DATA_TYPE (ODBC 1.0)	COLUMN_TYPE (ODBC 2.0)	COLUMN_SIZE (ODBC 1.0)
6	TYPE_NAME (ODBC 1.0)	DATA_TYPE (ODBC 2.0)	BUFFER_LENGTH (ODBC 1.0)
7	COLUMN_SIZE (ODBC 1.0)	TYPE_NAME (ODBC 2.0)	DECIMAL_DIGITS (ODBC 1.0)
8	BUFFER_LENGTH (ODBC 1.0)	COLUMN_SIZE (ODBC 2.0)	PSEUDO_COLUMN (ODBC 2.0)
9	DECIMAL_DIGITS (ODBC 1.0)	BUFFER_LENGTH (ODBC 2.0)	...
10	NUM_PREC_RADIX (ODBC 1.0)	DECIMAL_DIGITS (ODBC 2.0)
11	NULLABLE (ODBC 1.0)	NUM_PREC_RADIX (ODBC 2.0)
12	REMARKS (ODBC 1.0)	NULLABLE (ODBC 2.0)
13	COLUMN_DEF (ODBC 3.0)	REMARKS (ODBC 2.0)
14	SQL_DATA_TYPE (ODBC 3.0)	COLUMN_DEF (ODBC 3.0)
15	SQL_DATETIME_SUB (ODBC 3.0)	SQL_DATA_TYPE (ODBC 3.0)
16	CHAR_OCTET_LENGTH (ODBC 3.0)	SQL_DATETIME_SUB (ODBC 3.0)
17	ORDINAL_POSITION (ODBC 3.0)	CHAR_OCTET_LENGTH (ODBC 3.0)
18	IS_NULLABLE (ODBC 3.0)	ORDINAL_POSITION (ODBC 3.0)
19	...	IS_NULLABLE (ODBC 3.0)
19	...	...

I think, we can just maintain columns, procedure_columns and special_columns classes and avoid any potential over-engineering for adaptive indexing if we wanted to have single columns.

I'd also back out of the idea of the interface unification, i.e. name method catalog instead of table_catalog or procedure_catalog, and keep the full/descriptive names.

@detule @lexicalunit thoughts?

[detule]

@mloskot agreed.

If I read your comment correctly - whats outstanding is for me to expand out some of the names I had abbreviated.

Thanks for the feedback.

[mloskot]

@detule Yes, I suggest we consistently avoid abbreviations when naming elements of public interface.

[detule]

Should be done.

Thanks again.

[mloskot]

LGTM

Thanks for the contribution!

[lexicalunit]

Very cool!

[detule]

Hi team:

Let me know if there's anything outstanding here.
Thanks

[mloskot]

@detule I'm sorry, I completely forgot to merge it. Thanks again!