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

feat(postgres): Support DIV() func for integer division #3602

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

VaggelisD
Copy link
Collaborator

Fixes #3601

This PR introduces support for Postgres's DIV() function which performs integer division. It mirrors the BigQuery implementation which stores the args in exp.IntDiv and generates it back to the function representation.

Postgres Div

@georgesittas
Copy link
Collaborator

georgesittas commented Jun 5, 2024

Both this and BigQuery's implementation are not correct besides roundtripping.

Specifically, the return type is not necessary an integer in either of these dialects (BQ docs, Postgres docs), so the semantics may change when transpiling DIV to other dialects.

For example, try transpiling this from Postgres into SQLite and you'll notice that the source and resulting queries produce different results:

-- Postgres, results in 0.5 because the type of the RHS is NUMERIC and so we're actually doing a float division
SELECT 1 / DIV(4, 2)
-- SQLite, results in 0
SELECT 1 / CAST(4 / 2 AS INTEGER)

@georgesittas
Copy link
Collaborator

Transpiling from BigQuery may work because it doesn't use typed division semantics, but that's coincidental.

@VaggelisD
Copy link
Collaborator Author

This is still a WIP, will attempt to implement the proper semantics for exp.IntDiv in BQ/Duckdb/Postgres as well such that transpiling between each other preserves the result type.

@VaggelisD VaggelisD force-pushed the vaggelisd/postgres_div branch from 6d065b2 to 0df3d1b Compare June 6, 2024 15:37
@VaggelisD VaggelisD force-pushed the vaggelisd/postgres_div branch from 0df3d1b to 94e9a6d Compare June 6, 2024 15:39
@VaggelisD
Copy link
Collaborator Author

Sorry for the force pushes, had to resolve conflicts and also amended some changes by accident. Leaving the following for future reference:

From my search, it is Postgres, DuckDB and BigQuery which support integer division. Out of those, Postgres's DIV() always results in a DECIMAL; In DuckDB and BigQuery, the types of the operands can alter the result type.

For this reason, it's not possible to safely transpile DuckDB's/BigQuery's exp.IntDiv without type inference. As for Postgres, this PR always casts exp.IntDiv to a decimal and removes the cast if it's roundtripped.

@georgesittas georgesittas requested a review from tobymao June 6, 2024 16:38
"postgres": "1 / DIV(4, 2)",
},
write={
"sqlite": "1 / CAST(CAST(CAST(4 AS REAL) / 2 AS INTEGER) AS REAL)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The CAST(4 AS REAL) here looks a bit weird, I don't think it should've been added. The root cause is that the IntDiv generator doesn't properly populate the typed and safe args, disregarding the dialect's semantics. It seems like that method was added a long time ago, so this is most likely like an oversight.

Copy link
Collaborator

@georgesittas georgesittas Jun 7, 2024

Choose a reason for hiding this comment

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

I don't think it should've been added

This is because Postgres uses typed division, so since we're generating a Div as part of IntDiv's generation, the typed division semantics should be respected. See for example how we don't add a cast to REAL here:

>>> import sqlglot
>>> sqlglot.transpile("1/2", "postgres", "sqlite")
['1 / 2']

Copy link
Collaborator

Choose a reason for hiding this comment

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

May be best to address this in a followup PR after all. I played around a bit and it seems like we'll need to think more about how to parse / generate IntDiv.

@georgesittas georgesittas merged commit 4b30b87 into main Jun 7, 2024
6 checks passed
@georgesittas georgesittas deleted the vaggelisd/postgres_div branch June 7, 2024 15:55
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.

parsing div() for postgresql throws error
2 participants