Skip to content

Fix the panic when lpad/rpad parameter is negative#3829

Merged
andygrove merged 1 commit into
apache:masterfrom
ZuoTiJia:pad-panic
Oct 17, 2022
Merged

Fix the panic when lpad/rpad parameter is negative#3829
andygrove merged 1 commit into
apache:masterfrom
ZuoTiJia:pad-panic

Conversation

@ZuoTiJia

@ZuoTiJia ZuoTiJia commented Oct 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

When I pass a negative parameter to the lpad function, it panics. Because the function will cast i64 to usize, which causes Vec to be unable to allocate enough memory, resulting in 'panic: capacity overflow'.

DataFusion CLI v13.0.0
❯ select lpad('abc', -1, 'abc');
thread 'main' panicked at 'capacity overflow', library/alloc/src/raw_vec.rs:518:5

I refer to the implementation of postgres, in postges, length is limited to i32, too large length will throw an error.

In psql 14.5

SELECT lpad('abc', 2147483647, 'abc');

ERROR: requested length too large

SELECT lpad('abc', -1);

lpad

------

(1 row)

At first I changed the function signature.

But after I changed the function signature, calling this function requires troublesome cast.

What changes are included in this PR?

So I can only change the inside of the function

if length < 0 length = 0;

if length > i32::MAX, an Error will be thrown

@github-actions github-actions Bot added core Core DataFusion crate physical-expr Changes to the physical-expr crates labels Oct 14, 2022
@ZuoTiJia ZuoTiJia changed the title Fix the panic when lpad/rpad parameter is -1 Fix the panic when lpad/rpad parameter is negative Oct 14, 2022
@ZuoTiJia

ZuoTiJia commented Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

@alamb could you review?

andygrove
andygrove reviewed Oct 14, 2022
View reviewed changes
Comment thread datafusion/physical-expr/src/unicode_expressions.rs Outdated
let length = length as usize;
if length > i32::MAX as i64 {
return Err(DataFusionError::Internal(
"lpad requested length too large".to_string(),

@andygrove andygrove Oct 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"lpad requested length too large".to_string(),
"rpad requested length too large".to_string(),

@alamb alamb Oct 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"lpad requested length too large".to_string(),
format!("rpad requested length {} too large", length),

andygrove
andygrove reviewed Oct 14, 2022
View reviewed changes
Comment thread datafusion/physical-expr/src/unicode_expressions.rs Outdated
let length = length as usize;
if length > i32::MAX as i64 {
return Err(DataFusionError::Internal(
"lpad requested length too large".to_string(),

@andygrove andygrove Oct 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"lpad requested length too large".to_string(),
"rpad requested length too large".to_string(),

@alamb alamb Oct 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"lpad requested length too large".to_string(),
format!("rpad requested length {} too large", length),

alamb
alamb reviewed Oct 15, 2022
View reviewed changes

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is looking great -- thank you @ZuoTiJia

Comment thread datafusion/physical-expr/src/unicode_expressions.rs Outdated
let length = length as usize;
if length > i32::MAX as i64 {
return Err(DataFusionError::Internal(
"lpad requested length too large".to_string(),

@alamb alamb Oct 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"lpad requested length too large".to_string(),
format!("lpad requested length {} too large", length),

Comment thread datafusion/physical-expr/src/unicode_expressions.rs Outdated
let length = length as usize;
if length > i32::MAX as i64 {
return Err(DataFusionError::Internal(
"lpad requested length too large".to_string(),

@alamb alamb Oct 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"lpad requested length too large".to_string(),
format!("rpad requested length {} too large", length),

Comment thread datafusion/physical-expr/src/unicode_expressions.rs Outdated
let length = length as usize;
if length > i32::MAX as i64 {
return Err(DataFusionError::Internal(
"lpad requested length too large".to_string(),

@alamb alamb Oct 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"lpad requested length too large".to_string(),
format!("rpad requested length {} too large", length),

@ZuoTiJia
Fix the panic when lpad/rpad parameter is negative
c1938e3 
* fix the panic when lpad/rpad parameter is negative

* add lpad/rpad test
@ZuoTiJia
ZuoTiJia requested review from alamb and andygrove and removed request for alamb and andygrove October 15, 2022 13:26
alamb
alamb approved these changes Oct 16, 2022
View reviewed changes

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great -- thanks @ZuoTiJia

@ZuoTiJia
ZuoTiJia requested a review from andygrove October 17, 2022 00:25
andygrove
andygrove approved these changes Oct 17, 2022
View reviewed changes

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @ZuoTiJia

@andygrove
andygrove merged commit aec240f into apache:master Oct 17, 2022
@ursabot

ursabot commented Oct 17, 2022

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 431a412 and contender = aec240f. aec240f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes
@andygrove andygrove andygrove approved these changes

Assignees

No one assigned

Labels

core Core DataFusion crate physical-expr Changes to the physical-expr crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ZuoTiJia @ursabot @alamb @andygrove