Fix round double for postgres#5368
Conversation
|
I'm not sure what I should add here to make this pull request complete. One thing I'm in doubt about is how much effort should be put into verifying with various dialects? For instance I have checked various queries directly in a Postgres and a RisingWave engine (making sure that |
georgesittas
left a comment
georgesittas
left a comment
There was a problem hiding this comment.
One thing I'm in doubt about is how much effort should be put into verifying with various dialects? For instance I have checked various queries directly in a Postgres and a RisingWave engine (making sure that round(double precision, integer) doesn't work and that the proposed changes are needed to transpile correctly) but I haven't done the same in Redshift and Materialize, instead relying on their official docs.
That's fine, the testing you described suffices for this use case.
FYI, this is a breaking change, since it changes the AST's structure. Otherwise, the PR looks good, I don't expect any issues from preemptively casting the argument for postgres if we can't infer it.
| exp.Rand: rename_func("RANDOM"), | ||
| exp.RegexpLike: lambda self, e: self.binary(e, "~"), | ||
| exp.RegexpILike: lambda self, e: self.binary(e, "~*"), | ||
| exp.Round: _round, |
There was a problem hiding this comment.
Nit: Can we get rid of this entry and move _round under the Generator class, named as round_sql? I'd also inline _to_decimal.
|
I'll get this in and take it to the finish line. Thanks! |
2 participants
Fixing #5366