OHDSI / SqlRender

This is an R package and Java library for rendering parameterized SQL, and translating it to different SQL dialects.
https://ohdsi.github.io/SqlRender
Other
80 stars 77 forks source link

Convert permanent table references to temp table ones #374

Open dmitrys-odysseus opened 1 month ago

dmitrys-odysseus commented 1 month ago

Observed: foo.#bar is translated into foo.bar Expected behaviour: foo.#bar is translated into bar or foo_bar

Explanation: According to postges documentation (see https://www.postgresql.org/docs/current/sql-createtable.html for details), "Temporary tables exist in a special schema, so a schema name cannot be given when creating a temporary table."

Since there are some DB's that support creating temp table schema (and even kind of require that, e.g. Snowlake), untranslated sql has to contain schema specification to support these cases. So the only logical choice for postgres translation is to drop the schema or convert it to a prefix.

schuemie commented 1 month ago

So far we have always followed the convention that temp tables have no schema in OhdsiSql. What would be the motivation for doing that now? It doesn't seem like something that would translate across platforms. Why do you say that Snowflake requires this?

dmitrys-odysseus commented 1 month ago

There is another good reason for that, the original code might have to support BOTH temporary table and normal table. For example, we are using circe https://github.com/OHDSI/circe-be/blob/master/src/main/resources/resources/cohortdefinition/sql/generateCohort.sql

DELETE FROM @target_database_schema.@target_cohort_table where @cohort_id_field_name = @target_cohort_id;
INSERT INTO @target_database_schema.@target_cohort_table (@cohort_id_field_name, subject_id, cohort_start_date, cohort_end_date)

and target_cohort_table is temp table.

schuemie commented 1 month ago

I'm sorry, but that doesn't seem like a translation issue. That would change the semantics of the query.

dmitrys-odysseus commented 1 month ago

With current behaviour, it is quite impossible to write SQL that would work both with normal and temp tables. Could you please explain the point about changing semantics? If we know that postgres doesn't support temp table in schema, what would change?

schuemie commented 1 month ago

The scope of SqlRender is translation, and in translation I think we should try not to change any of the meaning. What you are proposing is using SqlRender to change the meaning of the SQL. I'm arguing that is out of scope of SqlRender.

Note that in this case it is quite easy to change the meaning of the SQL the way you want: just a simple textual find-and-replace that you can do with a single line of code. I'm just saying I don't think SqlRender is a general find-and-replace tool, it is a translation tool.

schuemie commented 1 month ago

I think what you want could easily be achieved using the render() function:

render(
  sql = "SELECT * INTO @target_database_schema.@target_cohort_table;",
  target_database_schema. = "",
  target_cohort_table = "#temp_cohort"
)
# [1] "SELECT * INTO #temp_cohort;"

Note the period at the end of target_database_schema..