Skip to content
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

Convert permanent table references to temp table ones #374

Closed
dmitrys-odysseus opened this issue Aug 14, 2024 · 6 comments
Closed

Convert permanent table references to temp table ones #374

dmitrys-odysseus opened this issue Aug 14, 2024 · 6 comments

Comments

@dmitrys-odysseus
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

@dmitrys-odysseus
Copy link
Author

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
Copy link
Member

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 schuemie changed the title Postres Temp table handling Convert permanent table references to temp table ones Aug 15, 2024
@schuemie
Copy link
Member

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..

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

No branches or pull requests

2 participants