Skip to content

Conversation

@csafreen
Copy link
Contributor

Before you do a pull request, you should always file an issue and make sure the package maintainer agrees that it’s a problem, and is happy with your basic proposal for fixing it. We don’t want you to spend a bunch of time on something that we don’t think is a good idea.

Additional requirements for pull requests:

  • Adhere to the Developer Guidelines as well as the OHDSI Code Style.

  • If possible, add unit tests for new functionality you add.

  • Restrict your pull request to solving the issue at hand. Do not try to 'improve' parts of the code that are not related to the issue. If you feel other parts of the code need better organization, create a separate issue for that.

  • Make sure you pass R check without errors and warnings before submitting.

  • Always target the develop branch, and make sure you are up-to-date with the develop branch.

@csafreen csafreen requested a review from alicia-koh October 15, 2025 07:11
rJava::.jcall(batchedInsert, "V", "setDate", i, as.character(column))
} else {
rJava::.jcall(batchedInsert, "V", "setString", i, as.character(column))
column <- escapeJson(column)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can check why this change is needed? Because it looks like it will replace all the single quotes and semicolons if the data is string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cohort generator module is failing to upload the results specifically for cg_cohort_definition table.
The table has a column called "sql_command" which has the queries run during the module execution.
The queries has ";" which is causing the import to fail. Thats why removed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Single quotes also needed to be escaped before inserting into db as the column value is by default enclosed in a single quote.

Copy link
Contributor

Choose a reason for hiding this comment

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

But besides cohort gen module other ohdsi packages (not sure which) could be using this function? Wouldn't this change the data if it has single quotes and a ;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since its an insert, escaping single quote should be fine.
semicolon will most likely be in texts.
Will have to add custom check there with table name for others to not be effected.
But if the text contains semicolon then duckdb will have an issue while insert for all tables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can control this by setting an env variable from strategus plugin if Cohort generator module is called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But any insert to a table for a text column with semicolon will fail with trex connection.

@csafreen csafreen requested a review from alicia-koh October 15, 2025 08:02
@csafreen csafreen merged commit 497c24a into alp-dqd Nov 11, 2025
2 of 8 checks passed
@csafreen csafreen deleted the csafreen/set_autoCommit_true branch November 11, 2025 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants