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

Add dialect param to use double precision for float64 in Postgres #11495

Merged
merged 5 commits into from
Jul 19, 2024

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
23 checks passed
@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.

None yet

2 participants