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 Decimal256 to ScalarValue #7048

Merged
merged 6 commits into from
Jul 25, 2023
Merged

Add Decimal256 to ScalarValue #7048

merged 6 commits into from
Jul 25, 2023

Conversation

viirya
Copy link
Member

@viirya viirya commented Jul 20, 2023

Which issue does this PR close?

Closes #7049.

Related: #7046.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jul 20, 2023
@@ -1038,6 +1054,7 @@ macro_rules! impl_op_arithmetic {
get_sign!($OPERATION),
true,
)))),
// todo: Add Decimal256 support
Copy link
Member Author

@viirya viirya Jul 20, 2023

Choose a reason for hiding this comment

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

The error DataFusion error: Internal error: Operator \+ is not implemented for types Decimal256\(None,15,2\) and Decimal256\(Some\(12300\),15,2\)\. from running

create table t as values (arrow_cast(123, 'Decimal256(5,2)'));
select AVG(column1) from t;

in decimal.slt comes from lack of implementation of Decimal256 Scalar arithmetic op.

It seems to be not easy to finish right now. So I left it as todo here.

-- This feature is not implemented: Can't create a scalar from array of type "Decimal256(3, 2)"
--arrow_cast(100, 'Decimal256(5,2)') as col_d256
arrow_cast(100, 'Decimal128(5,2)') as col_d128,
arrow_cast(100, 'Decimal256(5,2)') as col_d256
Copy link
Member Author

Choose a reason for hiding this comment

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

Now it is okay to create table using decimal256.

Comment on lines +199 to +205
query RR
SELECT
col_d128,
col_d256
FROM foo;
----
100 100.00
Copy link
Member Author

Choose a reason for hiding this comment

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

Though, I'm not sure why displayed value for decimal256 shows decimal points but decimal128 doesn't.

Needs to dig into where is the code for displaying it. But for now it seems minor to me. So just leave as is.

@viirya
Copy link
Member Author

viirya commented Jul 20, 2023

Oh, seems CI won't regen protobuf code?

EDIT: Manually regen the protobuf code.

@viirya viirya changed the title Initial Support of Decimal256 ScalarValue Add Decimal256 to ScalaValue Jul 20, 2023
@alamb alamb changed the title Add Decimal256 to ScalaValue Add Decimal256 to ScalarValue Jul 21, 2023
@viirya viirya force-pushed the init_decimal256 branch from d7002c8 to 1aaf63b Compare July 25, 2023 00:03
@viirya
Copy link
Member Author

viirya commented Jul 25, 2023

Hmm, seems the failed tests in CI are unrelated. They are failed too on main branch as I run them locally

@alamb
Copy link
Contributor

alamb commented Jul 25, 2023

I merged up from main to get the fix for #7078

I plan to merge this PR when CI passes

@alamb alamb merged commit ed045d9 into apache:main Jul 25, 2023
@viirya
Copy link
Member Author

viirya commented Jul 25, 2023

Thank you @Dandandan @alamb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions 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.

Add Decimal256 to ScalaValue
3 participants