fix: add support for Decimal128 and Decimal256 types in interval arithmetic #14126

waynexia · 2025-01-14T13:10:39Z

…hmetic

Which issue does this PR close?

Closes #14124

Rationale for this change

Fixes the panic when decimal computation overflows

What changes are included in this PR?

This patch contains two fixes:

fix the logic of checking if a value is negative.
implement get_extreme_value for decimal 128 and 256 types

Are these changes tested?

adding test

Are there any user-facing changes?

…hmetic Signed-off-by: Ruihang Xia <[email protected]>

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Signed-off-by: Ruihang Xia <[email protected]>

comphead

thanks @waynexia
I feed we can also add the initial problematic query into .slt file?

SELECT ('0.54321543215432154321543215432154321'::DECIMAL(35,35) + 10000)::VARCHAR

Signed-off-by: Ruihang Xia <[email protected]>

waynexia · 2025-01-15T04:35:42Z

thanks @waynexia I feed we can also add the initial problematic query into .slt file?
SELECT ('0.54321543215432154321543215432154321'::DECIMAL(35,35) + 10000)::VARCHAR

Done 👍, added one in select.slt

alamb

Thank you for the bug fix @waynexia

I am not sure about the code for min/max extremes however

alamb · 2025-01-15T15:29:50Z

datafusion/expr-common/src/interval_arithmetic.rs

@@ -76,6 +76,14 @@ macro_rules! get_extreme_value {
            DataType::Interval(IntervalUnit::MonthDayNano) => {
                ScalarValue::IntervalMonthDayNano(Some(IntervalMonthDayNano::$extreme))
            }
+            DataType::Decimal128(precision, scale) => {
+                ScalarValue::Decimal128(Some(i128::$extreme), *precision, *scale)


Is this correct for min/max of i128? It seems like the minimum value of Decimal128(precision, scale) would be the minimum value for the precision and scale separately rather than the min value of the overall i128 🤔

I dug around in arrow a bit and I think these are the min/max values for each decimal precision

https://docs.rs/arrow/latest/arrow/datatypes/constant.MIN_DECIMAL_FOR_EACH_PRECISION.html
https://docs.rs/arrow/latest/arrow/datatypes/constant.MAX_DECIMAL_FOR_EACH_PRECISION.html

Ahh, that makes sense. These huge arrays remind me it should be all of 9s...

sadly the decimal 256 version is not public 😢 let me find a workaround

maybe get_extreme_value should be part of arrow?

sadly the decimal 256 version is not public 😢 let me find a workaround

I recommend

copy/pasting the value into DataFusion for now

File a ticket in arrow-rs to make it public (or maybe even a PR!)

leave a comment next to the copy in DataFUsion referencing the upstream ticket

I filed a PR in arrow to expose these constants:

Expose min/max values for Decimal128/256 and improve docs arrow-rs#6992

wait for apache/datafusion#14126 to fix this

* change dep Signed-off-by: Ruihang Xia <[email protected]> * feat: adapt to arrow's interval array * chore: fix compile errors in datatypes crate * chore: fix api crate compiler errors * chore: fix compiler errors in common-grpc * chore: fix common-datasource errors * chore: fix deprecated code in common-datasource * fix promql and physical plan related Signed-off-by: Ruihang Xia <[email protected]> * wip: upgrading network deps Signed-off-by: Ruihang Xia <[email protected]> * block on updating `sqlparser` * upgrade sqlparser Signed-off-by: Ruihang Xia <[email protected]> * adapt new df's trait requirements Signed-off-by: Ruihang Xia <[email protected]> * chore: fix compiler errors in mito2 * chore: fix common-function crate errors * chore: fix catalog errors * change import path Signed-off-by: Ruihang Xia <[email protected]> * chore: fix some errors in query crate * chore: fix some errors in query crate * aggr expr and some other tiny fixes Signed-off-by: Ruihang Xia <[email protected]> * chore: fix expr related errors in query crate * chore: fix query serializer and admin command * chore: fix grpc services * feat: axum serve * chore: fix http server * remove handle_error handler * refactor timeout layer * serve axum * chore: fix flow aggr functions * chore: fix flow * feat: fix errors in meta-srv * boxed() * use TokioIo * feat!: Remove script crate and python feature (#5321) * feat: exclude script crate * chore: simplify feature * feat: remove the script crate * chore: remove python feature and some comments * chore: fix warning * chore: fix servers tests compiler errors * feat: fix tests-integration errors * chore: fix unused * test: fix catalog test * chore: fix compiler errors for crates using common-meta testing feature is enabled when check with --workspace * test: use display for logical plan test * test: implement rewrite for ScanHintRule * fix: http server build panic * test: fix mito test * fix: sql parser type alias error * test: fix TestClient not listen * test: some flow tests * test(flow): more fix * fix: test_otlp_logs * test: fix promql test that using deprecated method fun() * fix: sql type replace supports Int8 ~ Int64, UInt8 ~ UInt64 * test: fix infer schema test case * test: fix tests related to plan display * chore: fix last flow test * test: fix function format related assertion * test: use larger port range for tests * fix: test_otlp_traces * fix: test_otlp_metrics * fix range query and dist plan Signed-off-by: Ruihang Xia <[email protected]> * fix: flow handle distinct use deprecated field * fix: can't pass Join plan expressions to LogicalPlan::with_new_exprs * test: fix deserialize test * test: reduce split key case num * tests: lower case aggr func name * test: fix some sqlness tests * tests: more sqlness fix * tests: fixed sqlness test * commit non-bug changes Signed-off-by: Ruihang Xia <[email protected]> * fix: make our udf correct * fix: implement empty methods of ContextProvider for DfContextProviderAdapter * test: update sqlness test result * chore: remove unused * fix: provide alias name for AggregateExprBuilder in range plan * test: update range query result * fix: implement missing ContextProvider methods for DfContextProviderAdapter * test: update timestamps, cte result * fix: supports empty projection in mito * test: update comment for cte test * fix: support projection for numbers * test: update test cases after projection fix * fix: fix range select first_value/last_value * fix: handle CAST and time index conflict * fix: handle order by correctly in range first_value/last_value * test: update sqlness result * test: update view test result * test: update decimal test wait for apache/datafusion#14126 to fix this * feat: remove redundant physical optimization todo(ruihang): Check if we can remove this. * test: update sqlness test result * chore: range select default sort use nulls_first = false * test: update filter push down test result * test: comment deciaml test to avoid different panic message * test: update some distributed test result * test: update test for distributed count and filter push down * test: update subqueries test * fix: SessionState may overwrite our UDFs * chore: fix compiler errors after merging main * fix: fix elasticsearch and dashboard router panic * chore: fix common-functions tests * chore: update sqlness result * test: fix id keyword and update sqlness result * test: fix flow_null test * fix: enlarge thread size in debug mode to avoid overflow * chore: fix warnings in common-function * chore: fix warning in flow * chore: fix warnings in query crate * chore: remove unused warnings * chore: fix deprecated warnings for parquet * chore: fix deprecated warning in servers crate * style: fix clippy * test: enlarge mito cache tttl test ttl time * chore: fix typo * style: fmt toml * refactor: reimplement PartialOrd for RangeSelect * chore: remove script crate files introduced by merge * fix: return error if sql option is not kv * chore: do not use ..default::default() * chore: per review * chore: update error message in BuildAdminFunctionArgsSnafu Co-authored-by: jeremyhi <[email protected]> * refactor: typed precision * update sqlness view case Signed-off-by: Ruihang Xia <[email protected]> * chore: flow per review * chore: add example in comment * chore: warn if parquet stats of timestamp is not INT64 * style: add a newline before derive to make the comment more clear * test: update sqlness result * fix: flow from substrait * chore: change update_range_context log to debug level * chore: move axum-extra axum-macros to workspace --------- Signed-off-by: Ruihang Xia <[email protected]> Co-authored-by: Ruihang Xia <[email protected]> Co-authored-by: luofucong <[email protected]> Co-authored-by: discord9 <[email protected]> Co-authored-by: shuiyisong <[email protected]> Co-authored-by: jeremyhi <[email protected]>

Signed-off-by: Ruihang Xia <[email protected]>

waynexia · 2025-01-23T15:07:44Z

Hi @alamb, I've copied constant vectors from arrow, please take another look. A new dep (paste) is imported to concat variable name.

Signed-off-by: Ruihang Xia <[email protected]>

alamb

Thank you @waynexia ❤️ -- looks great to me

waynexia · 2025-01-25T13:24:30Z

Thank you @comphead and @alamb for reviewing!

fix: add support for Decimal128 and Decimal256 types in interval arit…

34088b5

…hmetic Signed-off-by: Ruihang Xia <[email protected]>

waynexia changed the title ~~fix: add support for Decimal128 and Decimal256 types in interval arit…~~ fix: add support for Decimal128 and Decimal256 types in interval arithmetic Jan 14, 2025

waynexia requested a review from Copilot January 14, 2025 13:12

Copilot AI reviewed Jan 14, 2025

View reviewed changes

add unit test

d0bef9a

Signed-off-by: Ruihang Xia <[email protected]>

comphead reviewed Jan 14, 2025

View reviewed changes

add sqllogictest

e3b193a

Signed-off-by: Ruihang Xia <[email protected]>

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 15, 2025

alamb reviewed Jan 15, 2025

View reviewed changes

evenyag added a commit to GreptimeTeam/greptimedb that referenced this pull request Jan 16, 2025

test: update decimal test

a53dc00

wait for apache/datafusion#14126 to fix this

alamb marked this pull request as draft January 17, 2025 15:22

alamb marked this pull request as ready for review January 17, 2025 15:23

alamb mentioned this pull request Jan 17, 2025

Expose min/max values for Decimal128/256 and improve docs apache/arrow-rs#6992

Merged

waynexia added 2 commits January 23, 2025 22:59

copy constant vectors from arrow

c51f5fb

Signed-off-by: Ruihang Xia <[email protected]>

toml file

a5f129b

Signed-off-by: Ruihang Xia <[email protected]>

waynexia added 3 commits January 23, 2025 23:24

update ut

e0768e6

Signed-off-by: Ruihang Xia <[email protected]>

update lock file

fe8d3b6

Signed-off-by: Ruihang Xia <[email protected]>

use auto link

0305028

Signed-off-by: Ruihang Xia <[email protected]>

alamb approved these changes Jan 24, 2025

View reviewed changes

waynexia merged commit 1dad545 into apache:main Jan 25, 2025
27 checks passed

waynexia deleted the fix-14124 branch January 25, 2025 13:24

alamb mentioned this pull request Feb 4, 2025

Feb 4, 2025: This week(s) in DataFusion #14491

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add support for Decimal128 and Decimal256 types in interval arithmetic #14126

fix: add support for Decimal128 and Decimal256 types in interval arithmetic #14126

waynexia commented Jan 14, 2025 •

edited

Loading

comphead left a comment

waynexia commented Jan 15, 2025

alamb left a comment

alamb Jan 15, 2025

alamb Jan 15, 2025

waynexia Jan 17, 2025

waynexia Jan 17, 2025

waynexia Jan 17, 2025

alamb Jan 17, 2025

alamb Jan 17, 2025

waynexia commented Jan 23, 2025 •

edited

Loading

alamb left a comment

waynexia commented Jan 25, 2025

fix: add support for Decimal128 and Decimal256 types in interval arithmetic #14126

fix: add support for Decimal128 and Decimal256 types in interval arithmetic #14126

Conversation

waynexia commented Jan 14, 2025 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

comphead left a comment

Choose a reason for hiding this comment

waynexia commented Jan 15, 2025

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 15, 2025

Choose a reason for hiding this comment

alamb Jan 15, 2025

Choose a reason for hiding this comment

waynexia Jan 17, 2025

Choose a reason for hiding this comment

waynexia Jan 17, 2025

Choose a reason for hiding this comment

waynexia Jan 17, 2025

Choose a reason for hiding this comment

alamb Jan 17, 2025

Choose a reason for hiding this comment

alamb Jan 17, 2025

Choose a reason for hiding this comment

waynexia commented Jan 23, 2025 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

waynexia commented Jan 25, 2025

waynexia commented Jan 14, 2025 •

edited

Loading

waynexia commented Jan 23, 2025 •

edited

Loading