-
Notifications
You must be signed in to change notification settings - Fork 98
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
feat(go/adbc/driver/snowflake): Add support for table constraints when calling GetObjects #1455
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on your first jab at the Go Driver. The table constraints implementation appears to be partial as compared to the spec. To ensure that the table constraints data is being stored and returned correctly it might be beneficial to include integration test for this functionality.
@@ -701,10 +701,16 @@ func prepareTablesSQL(matchingCatalogNames []string, catalog *string, dbSchema * | |||
if query != "" { | |||
query += " UNION ALL " | |||
} | |||
query += `SELECT * FROM "` + strings.ReplaceAll(catalog_name, "\"", "\"\"") + `".INFORMATION_SCHEMA.TABLES` | |||
query += `SELECT T.*, K.constraint_name, K.constraint_type FROM "` + strings.ReplaceAll(catalog_name, "\"", "\"\"") + `".INFORMATION_SCHEMA.TABLES AS T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the schema, constraint_column_names
and, constraint_column_usage
need to be included, however, I couldn't locate them in the queries.
[
constraint_column_names | list not null | (2)
constraint_column_usage | list<USAGE_SCHEMA> | (3)
](https://github.com/apache/arrow-adbc/blob/2afbbb7d4d471d21fe20eed976bf4c0b20151732/go/adbc/driver/snowflake/connection.go#L210C1-L217C56)
@@ -640,7 +639,8 @@ func (c *cnxn) getColumnsMetadata(ctx context.Context, matchingCatalogNames []st | |||
err = rows.Scan(&data.TblType, &data.Dbname, &data.Schema, &data.TblName, &data.ColName, | |||
&data.OrdinalPos, &data.IsNullable, &data.DataType, &data.NumericPrec, | |||
&data.NumericPrecRadix, &data.NumericScale, &data.IsIdent, &data.IdentGen, | |||
&data.IdentIncrement, &data.CharMaxLength, &data.CharOctetLength, &data.DatetimePrec, &data.Comment) | |||
&data.IdentIncrement, &data.CharMaxLength, &data.CharOctetLength, &data.DatetimePrec, &data.Comment, | |||
&data.ConstraintName, &data.ConstraintType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TableInfo struct doesn't currently capture table constraints.
I couldn't locate them being appended to tableConstraintsBuilder which seems to be marked as unimplemented.
AND T.table_schema = K.table_schema | ||
AND T.table_name = K.table_name UNION ALL SELECT T.table_type, | ||
C.*, | ||
K.constraint_name, K.constraint_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a test similar to TestMetadataGetObjectsColumnsXdbc will be helpful to verify the table constraints are returned correctly. Adding tests in the CSharp layer would also work.
6fca3fa
to
2afbbb7
Compare
…flakeTableConstraints
When Snowflake Go driver calls GetObjects against tables and columns, it will now also return table constraints (name and type) if any are found related to the table.
The tables and columns SQL queries were modified to include a left outer join against INFORMATION_SCHEMA.TABLE_CONSTRAINTS to retrieve constraint information. The metadata object also had to be updated to include the constraint fields.
Pre-commit had to be updated as I could not commit until it was done, and the previous versions of the pre-commit dependencies kept returning errors when I tried to execute them.