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

Keep the float information in scalar_to_sql #12609

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

Sevenannn
Copy link
Contributor

@Sevenannn Sevenannn commented Sep 24, 2024

Which issue does this PR close?

No correlated issue

Rationale for this change

The unparser omits the float scalar value information, causing incorrect sql from unparser.

When sending a query like

select * from inventory where inv_warehouse_sk between 3.0/2.0 AND 4.0/2.0

to Datafusion, the query plan related to the scalar value is

Filter: CAST(inventory.inv_warehouse_sk AS Float64) BETWEEN Float64(3) / Float64(2) AND Float64(4) / Float64(2)`. 

However, the unparser omits the Float64() information, causing wrong query from unparser

SELECT "inventory"."inv_date_sk", "inventory"."inv_item_sk", "inventory"."inv_warehouse_sk", "inventory"."inv_quantity_on_hand" FROM "inventory" WHERE (CAST("inventory"."inv_warehouse_sk" AS DOUBLE PRECISION) BETWEEN (3 / 2) AND (4 / 2))

This will cause incorrect data in some engine, for example, in postgres 3 / 2 is 1 and 3.0/2.0 is 1.5

What changes are included in this PR?

  • Add formatting step to add .0 for float values without decimal digits
  • Add tests to cover the changes

Are these changes tested?

Yes

Are there any user-facing changes?

User will receive float scalar value that correctly preserves the float information.

@github-actions github-actions bot added the sql SQL Planner label Sep 24, 2024
@Sevenannn Sevenannn marked this pull request as ready for review September 24, 2024 19:05
Copy link
Contributor

@phillipleblanc phillipleblanc left a comment

Choose a reason for hiding this comment

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

Thanks @Sevenannn! I think this PR is fine to merge as is. I do have a comment about how we can keep the generated SQL a bit nicer to read for humans, while still solving the issue this PR wants to solve.

datafusion/sql/src/unparser/expr.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@phillipleblanc phillipleblanc left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

Copy link
Contributor

@goldmedal goldmedal left a comment

Choose a reason for hiding this comment

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

Thanks @Sevenannn it looks good to me 👍

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 @Sevenannn and @phillipleblanc and @goldmedal for the reivews

@alamb alamb merged commit 4a3c09a into apache:main Sep 25, 2024
25 checks passed
bgjackma pushed a commit to bgjackma/datafusion that referenced this pull request Sep 25, 2024
* Keep the float information in scalar_to_sql

* Format float value instead of using string, and update test case
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.

4 participants