Skip to content

Conversation

@Sevenannn
Copy link
Contributor

Which issue does this PR close?

This PR addresses Float64 unparser issue producing an invalid CAST COL TO DOUBLE for Postgres.

Rationale for this change

The equivalence of arrow Expr::FLOAT64 type is DOUBLE PRECISION in Postgres, however it's currently converted into DOUBLE, which causes Postgres query failures

What changes are included in this PR?

See "What issue does this PR close"

Are these changes tested?

Yes, unit tests added and passed.

Are there any user-facing changes?

CustomDialectBuilde supports use_double_precision_for_float64 that can be used to specify whether DOUBLE vs DOUBLE PRECISION data type should be used for Float64 unparsing.

@github-actions github-actions bot added the sql SQL Planner label Jul 16, 2024

// Does the dialect use DOUBLE PRECISION to represent Float64 rather than DOUBLE?
// E.g. Postgres uses DOUBLE PRECISION instead of DOUBLE
fn use_double_precision_for_float64(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly to #11494 (comment) what do you think about making this simply return the AST type directly?

Suggested change
fn use_double_precision_for_float64(&self) -> bool {
fn use_double_precision_for_float64(&self) -> sqlparser::ast::DataType {

That is likely a more flexible API -- this formulation seems very narrow for postgres vs MySQL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @alamb , thanks for the feedback! Directly return AST type seems clearer and more flexible to me. I update the changes in my newest commit, please take a look when you are available:

  • Return sqlparser::ast::DataType instead of bool
  • Change the method name from use_double_precision_for_float64 to float64_ast_dtype

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @Sevenannn

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Look good to me -- thanks @Sevenannn

I think this PR has a conflict but once that is resolved I think it will be good to go.

Thank you for the contribution!

@alamb
Copy link
Contributor

alamb commented Jul 18, 2024

Nudge that this PR needs a merge up from main to resolve conflicts before I can merge it

Screenshot 2024-07-18 at 6 05 10 AM

@Sevenannn
Copy link
Contributor Author

Hi @alamb, thanks for the review! the conflicts are resolved.

@alamb
Copy link
Contributor

alamb commented Jul 19, 2024

Thanks again @Sevenannn

@alamb alamb merged commit 827d0e3 into apache:main Jul 19, 2024
@phillipleblanc phillipleblanc deleted the qianqian/float64-unparsing branch July 22, 2024 00:37
Lordworms pushed a commit to Lordworms/arrow-datafusion that referenced this pull request Jul 23, 2024
…ache#11495)

* Add dialect param to use double precision for float64 in Postgres

* return ast data type instead of bool

* Fix errors in merging

* fix
wiedld pushed a commit to influxdata/arrow-datafusion that referenced this pull request Jul 31, 2024
…ache#11495)

* Add dialect param to use double precision for float64 in Postgres

* return ast data type instead of bool

* Fix errors in merging

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants