Skip to content

Improve numerice_coercion and reuse in both compare op and math op - #8385

Closed
jayzhan211 wants to merge 11 commits into
apache:mainfrom
jayzhan211:fix-numeric-coercion
Closed

jayzhan211 wants to merge 11 commits into
apache:mainfrom
jayzhan211:fix-numeric-coercion

Conversation

@jayzhan211

@jayzhan211 jayzhan211 commented Dec 1, 2023 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Ref #8302

Rationale for this change

  1. Fix numeric coercion
  • Support f16 and fix f32 coercion
    Some type coercion seems incorrect for me, and f16 is not considered.

Something like, but not only

Prev PR
(u64, f32) -> f32 (u64, f32) -> f64
(i64, f32) -> f32 (i64, f32) -> f64
  • exact_numeric_coercion
    To differentiate the current possible lossy numeric_coercion. I think it might be helpful somewhere, but keep it todo!() until we need it.
  1. use common coercion for compare op and math op since I dont think we need two version for these two.

What changes are included in this PR?

Are these changes tested?

Since arrow-rs does not support f16 yet, I didn't add test for f16, just ensure the existing test does not fail.

Add math op coercion in slt. some test is not possible to move to slt yet, so keep it there.

Are there any user-facing changes?

additional context

Decimal coercion is different in compare op and math op. Not sure if we can have common coercion for them.

@github-actions github-actions Bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Dec 1, 2023
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
jayzhan211 force-pushed the fix-numeric-coercion branch from 8506795 to 0f6e235 Compare December 2, 2023 02:07
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211 jayzhan211 changed the title Suupport f16 to numeric_coercion and make it pub Use common non_decimal_numerice_coercion for both compare op and math op Dec 2, 2023
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@github-actions github-actions Bot added the optimizer Optimizer rules label Dec 2, 2023
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@github-actions github-actions Bot added the physical-expr Changes to the physical-expr crates label Dec 2, 2023
@jayzhan211 jayzhan211 changed the title Use common non_decimal_numerice_coercion for both compare op and math op Improve numerice_coercion and reuse in both compare op and math op Dec 2, 2023
@jayzhan211
jayzhan211 marked this pull request as ready for review December 2, 2023 05:53
@alamb

alamb commented Dec 3, 2023

Copy link
Copy Markdown
Contributor

Thanks @jayzhan211 -- I think this PR needs some careful review to ensure it doesn't introduce subtle bugs / logic errors. I will try and find time but I worry about this PR introducing regressions for no (practical) benefit.

Could you please:

  1. Add the new tests as a separate PR (to better document the current behavior)
  2. Explain what, if any, issue this is solving (e.g. what queries run with this PR that did not before, or what cases work better than they did)

Thank you

@jackwener

Copy link
Copy Markdown
Member

If I remember correctly, we currently do not support float16, so it's challenging for us to test these scenarios.
Therefore, I do not recommend add float16 in type coercion for now.

@jayzhan211

Copy link
Copy Markdown
Contributor Author

If I remember correctly, we currently do not support float16, so it's challenging for us to test these scenarios. Therefore, I do not recommend add float16 in type coercion for now.

yes, f16 is not fully support in arrow-rs, so I did not add test

@jayzhan211
jayzhan211 marked this pull request as draft December 4, 2023 00:48
@jayzhan211 jayzhan211 closed this Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants