Safeguard round function decimal places overflow #6232

tz70s · 2023-05-04T16:23:00Z

Which issue does this PR close?

Closes #6133.

Rationale for this change

Add runtime check rather than crash the system.

What changes are included in this PR?

Safeguard the panic from round function decimal places overflow.

In #6133 stated there could be a type check in binding phase, only add the runtime check in this PR though I do agree having error throwing out in the logical planning might be better.

Are these changes tested?

Yes

Are there any user-facing changes?

No

tz70s · 2023-05-04T16:24:37Z

datafusion/physical-expr/src/math_expressions.rs

                    }
                )) as ArrayRef)
            }
-            ColumnarValue::Array(decimal_places) => Ok(Arc::new(make_function_inputs2!(


I basically gave up using the existing macro (make_function_inputs2), not sure is it worth changing it to be possibly return Result within the $FUNC.

Nice work! This overflow issue can be solved during binding in the future(hopefully...), I think it's not necessary to change the existing macro 🤔

tz70s · 2023-05-04T17:28:12Z

Maybe we can simply change the Round to accept i32 here: https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/function.rs#L651-L654

tz70s · 2023-05-04T17:29:53Z

Maybe we can simply change the Round to accept i32 here: https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/function.rs#L651-L654

Postgres output:

postgres=# select round(5.4323, 2147483648);
ERROR:  function round(numeric, bigint) does not exist
LINE 1: select round(5.4323, 2147483648);
               ^
HINT:  No function matches the given name and argument types. You might need to add explicit type casts.

2010YOUY01 · 2023-05-19T20:06:18Z

datafusion/physical-expr/src/math_expressions.rs

                    }
                )) as ArrayRef)
            }
-            ColumnarValue::Array(decimal_places) => Ok(Arc::new(make_function_inputs2!(


Nice work! This overflow issue can be solved during binding in the future(hopefully...), I think it's not necessary to change the existing macro 🤔

2010YOUY01 · 2023-05-19T20:09:09Z

datafusion/physical-expr/src/math_expressions.rs

    }
 }

+fn round_downcast_decimal_places(decimal_place: i64) -> Result<i32> {


I noticed most small helpers inside execution are macros to guarantee inline for performance, should we make this function macro?

Thanks, I made it as a macro instead!

You could inline a function too. Not necessary to be macros actually as macros are more difficult to read.

Thanks, revert it back with inline attribute instead.

* Update object_store 0.6 and arrow * Remove pin * Fix pyarrow * Tomlfmt * Update flight_sql_service * Fix parquet_sql_multiple_files

* feat: multidimensional arrays * feat: array_append, array_prepend, array_concat, array_fill * feat: array_dims, array_length * feat: array_position, array_positions, array_remove, array_replace, array_to_string * feat: trim_array, cardinality * feat: docs * refactoring: code cleanup * fix: test_scalar_expr * fix: clippy * fix: array_concat capacity * fix: cargo fmt

* MemoryExec insert into refactor * Merge leftovers * Set target partition * Comment and formatting improvements * Comments on state. * ListingTable INSERT INTO support * Removing unnecessary code * Some comments are leftover. * Compression import error * Minor resolutions on cargo docs * Corrections after merge * Make FileWriterExt available * Single file support * Resolve linter errors * Minor changes, simplifications * Fix failing tests because of api change * Simplifications * Replace block nesting with drop * remove unnecessary code * Convert to new approach * simplify display * Update debug display * use handle err macro * Make handle err close all writer in case of error. * Final review, stylistic changes * Improve comments * Move insert into test to the explain.slt * convert macro to function * return error for abort in append mode. * Simplify condition of has header * Update comments * Remove file writer factory * use AbortableWrite struct instead of trait --------- Co-authored-by: metesynnada <100111937+metesynnada@users.noreply.github.com> Co-authored-by: Mehmet Ozan Kabak <ozankabak@gmail.com>

* Update hashbrown * Update hashbrown * Update hashbrown --------- Co-authored-by: Daniël Heres <daniel.heres@coralogix.com>

…pagation into `RecordBatchReceiverStream` (apache#6507) * Propagate panics Another try for fixing apache#3104. RepartitionExec might need a similar fix. * avoid allocation by pinning on the stack instead * Consolidate panic propagation into RecordBatchReceiverStream * Update docs / cleanup/ * Apply suggestions from code review Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com> * rename to be consistent and not deal with English pecularities * Add a test and comments * write test for drop cancel * Add test fpr not driving to completion * Do not drive all streams to error * terminate early on panic * tweak comments * tweak comments * use futures::stream --------- Co-authored-by: Nicolae Vartolomei <nv@nvartolomei.com> Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

* Move `physical_plan::file_format` to `datasource::plan` * fix doclinks

…he#6579) Updates the requirements on [substrait](https://github.com/substrait-io/substrait-rs) to permit the latest version. - [Release notes](https://github.com/substrait-io/substrait-rs/releases) - [Changelog](https://github.com/substrait-io/substrait-rs/blob/main/CHANGELOG.md) - [Commits](substrait-io/substrait-rs@v0.10.0...v0.11.0) --- updated-dependencies: - dependency-name: substrait dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* make page_filter public * make parquet public * fix CI

* Update `use crate::error` to `use datafusion_common` in physical_plan * More updates

* feat: distinct bitwise and boolean aggregate functions * feat: add distinct sqllogictests for other bitwise and boolean aggregate functions * feat: improve docs for csv_query_bit_xor_distinct

* Make the struct function return the correct data type. * add testcase

…pache#6592) * Add additional docstrings to Window function implementations * Update docs * updates * fix doc link * Change resorting --> re-sorting

* Cleanup tpch benchmark * Cleanup tpch benchmark --------- Co-authored-by: Daniël Heres <daniel.heres@coralogix.com>

* clippy * remove default() * style: remove redundant prefix in function.rs

… being used more than once (apache#6135) * Fix incorrect join key fields (indices) when same table is being used more than once * Addressed comments Update datafusion/substrait/src/logical_plan/producer.rs Co-authored-by: Ruihang Xia <waynestxia@gmail.com> Update datafusion/substrait/src/logical_plan/producer.rs Co-authored-by: Ruihang Xia <waynestxia@gmail.com> * Fixed bugs after rebase --------- Co-authored-by: Ruihang Xia <waynestxia@gmail.com>

* Add datafusion-cli tests to the CI Job * Fix clippy error of datafusion-cli * update tests of datafusion-cli * fix tests error of datafusion-cli * Splits out "cargo test datafusion-cli (amd64)" into separate CI job, fix "no space left on device" error

* Refactor joins test to sqllogic * Port more tests * More tests * Remove unused method * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test * Add more tests * Add more test * Add more test * Add more tests * Add more tests * Add more tests * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test * Add more test

This fixes inconsistency between rustdocs for Display of files and file groups by applying the same function formatting up to 5 elements. Also adds corresponding unit tests.

* feat: make_array support empty arguments * fix fmt error * fix error * array_append support empty array * array_prepend support empty make_array * array_concat support empty make_array * fix clippy * update * fix * rename `array_make` --> `make_array` --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

…ent at aggregate.slt

…pache#6646) Updates the requirements on [sqllogictest](https://github.com/risinglightdb/sqllogictest-rs) to permit the latest version. - [Release notes](https://github.com/risinglightdb/sqllogictest-rs/releases) - [Changelog](https://github.com/risinglightdb/sqllogictest-rs/blob/main/CHANGELOG.md) - [Commits](risinglightdb/sqllogictest-rs@v0.13.2...v0.14.0) --- updated-dependencies: - dependency-name: sqllogictest dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…he#6564) * Improve main api doc page * fix doc examples * fmt

) * Minor: Move include_rank into `BuiltInWindowFunctionExpr` * fix docs

apache#6601) * Prioritize UDF over scalar built-in function in case of function name collision * Remove prioritize_udf config flag (assume true by default) * Add test scalar_udf_override_built_in_scalar_function

viirya · 2023-06-12T18:18:39Z

datafusion/physical-expr/src/math_expressions.rs

+        })
+    };
+}
+


The input/output types are always i64/i32. This isn't necessary to be a macro, I guess.

You can have a inline function instead.

alamb · 2023-06-15T14:08:59Z

@viirya (or @liukun4515 ) do you have time to review / approve this PR? I haven't been following closely and I think you are the experts in Decimal implementation in DataFusion

alamb · 2023-07-03T19:32:42Z

I wonder if #6833 is similar functionality

alamb · 2023-07-18T20:59:44Z

I am not sure what is going on with this PR -- it now looks like a huge change and it has been stale for quite a while. Marking it as draft while we figure out what to do with it

viirya · 2023-07-19T00:43:25Z

I think the change is so out-of-dated from latest branch so the diffs are unclear what are actual change from this PR. It'd be better to sync up with latest branch.

github-actions · 2024-05-08T01:40:37Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

tz70s commented May 4, 2023

View reviewed changes

alamb requested a review from viirya May 4, 2023 20:34

2010YOUY01 reviewed May 19, 2023

View reviewed changes

tustvold and others added 25 commits June 6, 2023 14:41

Update to object_store 0.6 and arrow 41 (apache#6374)

91e75d7

* Update object_store 0.6 and arrow * Remove pin * Fix pyarrow * Tomlfmt * Update flight_sql_service * Fix parquet_sql_multiple_files

[Minor] Update hashbrown to 0.14 (apache#6562)

2eab6d7

* Update hashbrown * Update hashbrown * Update hashbrown --------- Co-authored-by: Daniël Heres <daniel.heres@coralogix.com>

refactor: use bitwise and boolean compute functions (apache#6568)

9cb6161

Move physical_plan::file_format to datasource::plan (apache#6516)

786f222

* Move `physical_plan::file_format` to `datasource::plan` * fix doclinks

refactor: remove type_coercion in PhysicalExpr. (apache#6575)

12b88ea

Minor: remove tokio_stream dependency (apache#6565)

ce76f1b

minor: remove useless mut and borrow() (apache#6580)

afa64e0

Add tests for object_store builders of datafusion-cli (apache#6576)

25b6556

Avoid per-batch field lookups in SchemaMapping (apache#6563)

8f7f76d

Move JoinType to datafusion_common (apache#6572)

cbfb7c8

refactor: bitwise kernel right and left shifts (apache#6585)

1d3860d

feat: type coercion support date - date (apache#6578)

9c13a77

make page filter public (apache#6523)

4973c7c

* make page_filter public * make parquet public * fix CI

Minor: Remove some use crate:: uses in physical_plan (apache#6573)

d689daf

* Update `use crate::error` to `use datafusion_common` in physical_plan * More updates

feat: DISTINCT bitwise and boolean aggregate functions (apache#6581)

dbd92e6

* feat: distinct bitwise and boolean aggregate functions * feat: add distinct sqllogictests for other bitwise and boolean aggregate functions * feat: improve docs for csv_query_bit_xor_distinct

Make the struct function return the correct data type. (apache#6594)

34fa0b9

* Make the struct function return the correct data type. * add testcase

fix: avoid panic in list_files_for_scan (apache#6605)

1a1efcf

feat: analyze/optimize plan in CREATE TABLE AS SELECT (apache#6610)

95c4681

Minor: Add additional docstrings to Window function implementations (a…

5032060

…pache#6592) * Add additional docstrings to Window function implementations * Update docs * updates * fix doc link * Change resorting --> re-sorting

Remove from_slice and use upstream (apache#6587)

3d36dfe

[Minor] Cleanup tpch benchmark (apache#6609)

43423be

* Cleanup tpch benchmark * Cleanup tpch benchmark --------- Co-authored-by: Daniël Heres <daniel.heres@coralogix.com>

Folyd and others added 16 commits June 11, 2023 07:05

Impl Literal trait for NonZero* types (apache#6627)

c4a036d

style: make clippy happy and remove redundant prefix (apache#6624)

57bc5b0

* clippy * remove default() * style: remove redundant prefix in function.rs

Minor: Add debug logging for schema mismatch errors (apache#6626)

aebe71e

Move functionality into BuildInScalarFunction (apache#6612)

d024f37

fix: fix more panics in ListingTable (apache#6636)

93b1a57

fix: median with even number of decimal128 not working (apache#6634)

945191f

Unify formatting of both groups and files up to 5 elements (apache#6637)

708044c

This fixes inconsistency between rustdocs for Display of files and file groups by applying the same function formatting up to 5 elements. Also adds corresponding unit tests.

Minor: cleanup the unnecessary CREATE TABLE aggregate_test_100 statem…

373e291

…ent at aggregate.slt

Improve main api doc page, move avro_to_arrow to datasource (apac…

b8f90fe

…he#6564) * Improve main api doc page * fix doc examples * fmt

Minor: Move include_rank into BuiltInWindowFunctionExpr (apache#6620

d584d55

) * Minor: Move include_rank into `BuiltInWindowFunctionExpr` * fix docs

Prioritize UDF over scalar built-in function in case of function name… (

c96949e

apache#6601) * Prioritize UDF over scalar built-in function in case of function name collision * Remove prioritize_udf config flag (assume true by default) * Add test scalar_udf_override_built_in_scalar_function

tz70s force-pushed the guard-round-decimal-overflow branch from 0fdffa8 to 70014e6 Compare June 12, 2023 17:16

github-actions bot added the physical-expr Changes to the physical-expr crates label Jun 12, 2023

viirya reviewed Jun 12, 2023

View reviewed changes

Safeguard round function decimal places overflow

fe128c4

tz70s force-pushed the guard-round-decimal-overflow branch from 70014e6 to fe128c4 Compare June 12, 2023 18:28

tustvold force-pushed the main branch from 9dd3c2b to 70d633a Compare June 27, 2023 09:44

alamb marked this pull request as draft July 18, 2023 20:59

github-actions bot added the Stale PR has not had any activity for some time label May 8, 2024

github-actions bot closed this May 15, 2024

Safeguard round function decimal places overflow #6232

Safeguard round function decimal places overflow #6232

Uh oh!

Conversation

tz70s commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

tz70s May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tz70s commented May 4, 2023

Uh oh!

tz70s commented May 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jun 15, 2023

Uh oh!

alamb commented Jul 3, 2023

Uh oh!

alamb commented Jul 18, 2023

Uh oh!

viirya commented Jul 19, 2023

Uh oh!

github-actions bot commented May 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

18 participants

tz70s commented May 4, 2023 •

edited

Loading

tz70s May 4, 2023 •

edited

Loading