-
Notifications
You must be signed in to change notification settings - Fork 53
Fix alternate schema bug in sql proxy #296
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
Fix alternate schema bug in sql proxy #296
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.
LGTM
var rs = metadata.getPrimaryKeys(null, schemaName, tableName); | ||
if (!rs.next()) | ||
throw new SQLException("No primary key found for " + table); | ||
throw new SQLException("No primary key found for " + (schemaName != null ? schemaName + "." + tableName : tableName)); |
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.
Should we make this into a function schemaName != null ? schemaName + "." + tableName : tableName
? Since it is repeated a couple of times.
@danielgerlag are there no changes required to the releation reactivator to distinguish between tables with the same name but from different schema? |
throw new SQLException("No primary key found for " + (schemaName != null ? schemaName + "." + tableName : tableName)); | ||
var mapping = new NodeMapping(); | ||
mapping.tableName = rs.getString("TABLE_SCHEM") + "." + tableName; | ||
String actualSchema = rs.getString("TABLE_SCHEM"); |
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.
Is TABLE_SCHEM a typo that we are perpetuating?
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.
TABLE_SCHEM
is part of the JDBC drivers, for returning metadata.
https://learn.microsoft.com/en-us/sql/connect/jdbc/reference/getschemas-method?view=sql-server-ver17#remarks
The reactivator does not have this issue, because we are not manually constructing a query string, the debezium library handles things. |
This pull request refactors the way tables and schemas are handled in the SQL proxy by introducing a new
TableRegistry
class and updating the table cursor logic to support schema-qualified table names. The changes improve robustness when dealing with tables from multiple schemas and ensure that only registered tables are processed, and fixes a bug where tables in the non-default schema could not be bootstrapped.Schema and Table Registry Improvements:
TableRegistry
class to manage table-to-schema mappings, parsing thetables
config and providing lookup methods for table existence and schema name. (sources/relational/sql-proxy/src/main/java/io/drasi/TableRegistry.java
)ResultStream
to instantiate and useTableRegistry
, checking if each requested table exists and retrieving its schema before creatingTableCursor
instances. Unregistered tables are logged and skipped. (sources/relational/sql-proxy/src/main/java/io/drasi/ResultStream.java
) [1] [2]Table Cursor and Query Handling:
TableCursor
to accept bothschemaName
andtableName
in its constructor, and to use these values when building SQL queries and reading table mappings. (sources/relational/sql-proxy/src/main/java/io/drasi/TableCursor.java
)TableCursor
to use fully qualified table names (including schema when present), ensuring correct querying in multi-schema environments. (sources/relational/sql-proxy/src/main/java/io/drasi/TableCursor.java
)sources/relational/sql-proxy/src/main/java/io/drasi/TableCursor.java
)