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

expr: allow functions to return errors #2233

Merged
merged 1 commit into from Mar 7, 2020

Conversation

@benesch
Copy link
Member

benesch commented Mar 5, 2020

The errors are thrown away immediately and replaced with NULL, as
before, but we have to start somewhere.

Touches #489.

The errors are thrown away immediately and replaced with NULL, as
before, but we have to start somewhere.

Touches #489.
let b = b.unwrap_int32();
if b == 0 {
Datum::Null
bail!("division by zero");

This comment has been minimized.

Copy link
@frankmcsherry

frankmcsherry Mar 5, 2020

Member

Should we provide clearer information about this being mod rather than div? I get that the first is usually implemented in terms of the latter, but might help?

This comment has been minimized.

Copy link
@frankmcsherry

frankmcsherry Mar 5, 2020

Member

And if so, the other mod methods are similar.

This comment has been minimized.

Copy link
@benesch

benesch Mar 6, 2020

Author Member

Just following Postgres here

https://github.com/postgres/postgres/blob/13661ddd7eaec7e2809ff5c29fc14653b6161036/src/backend/utils/adt/int.c#L1110-L1123

though it looks like Cockroach decided not to follow Postgres, as it returns "zero modulus". 🤷‍♂

fn cast_int64_to_int32<'a>(a: Datum<'a>) -> Result<Datum<'a>, failure::Error> {
match i32::try_from(a.unwrap_int64()) {
Ok(n) => Ok(Datum::from(n)),
Err(_) => Err(format_err!("integer out of range")),

This comment has been minimized.

Copy link
@frankmcsherry

frankmcsherry Mar 5, 2020

Member

Should this be bail!() to match other examples in the code? No opinion myself, especially if there is a reason, but consistency is great if possible! :D

This comment has been minimized.

Copy link
@benesch

benesch Mar 6, 2020

Author Member

I personally use bail! only for early returns, and format_err! for non-early returns—but I can see how that might be more confusing for other folks! I'll change if I go back through CI on this PR. Otherwise will punt to the follow-on PR to ease the rebase.

Copy link
Member

frankmcsherry left a comment

This looks good! It does change the behavior to produce nulls rather than panic, which my understanding is we will evolve to produce the results themselves, and which I'm sure you know but I just wanted to flag as a behavioral change.

Copy link
Member Author

benesch left a comment

It does change the behavior to produce nulls rather than panic, which my understanding is we will evolve to produce the results themselves, and which I'm sure you know but I just wanted to flag as a behavioral change.

I'm confused! We take the errors returned by function evaluation and suppress them by returning Datum::Null instead of panicking—but that should result in behavior identical to the current behavior, not a behavioral change!

fn cast_int64_to_int32<'a>(a: Datum<'a>) -> Result<Datum<'a>, failure::Error> {
match i32::try_from(a.unwrap_int64()) {
Ok(n) => Ok(Datum::from(n)),
Err(_) => Err(format_err!("integer out of range")),

This comment has been minimized.

Copy link
@benesch

benesch Mar 6, 2020

Author Member

I personally use bail! only for early returns, and format_err! for non-early returns—but I can see how that might be more confusing for other folks! I'll change if I go back through CI on this PR. Otherwise will punt to the follow-on PR to ease the rebase.

let b = b.unwrap_int32();
if b == 0 {
Datum::Null
bail!("division by zero");

This comment has been minimized.

Copy link
@benesch

benesch Mar 6, 2020

Author Member

Just following Postgres here

https://github.com/postgres/postgres/blob/13661ddd7eaec7e2809ff5c29fc14653b6161036/src/backend/utils/adt/int.c#L1110-L1123

though it looks like Cockroach decided not to follow Postgres, as it returns "zero modulus". 🤷‍♂

@jamii

This comment has been minimized.

Copy link
Member

jamii commented Mar 6, 2020

Is this intended to cover all the errors? Most of the jsonb_* and cast_jsonb_* functions have error cases (although some also legitimately return null at times).

@jamii

This comment has been minimized.

Copy link
Member

jamii commented Mar 6, 2020

Also some of the cast_string_* functions.

@benesch

This comment has been minimized.

Copy link
Member Author

benesch commented Mar 7, 2020

Good point! I only added errors where there was an explicit TODO in place. I'll go through and file issues or fix the rest.

@benesch benesch merged commit e6322c7 into MaterializeInc:master Mar 7, 2020
14 checks passed
14 checks passed
buildkite/tests Build #5743 passed (16 minutes, 16 seconds)
Details
buildkite/tests/bath-lint-and-rustfmt Passed (20 seconds)
Details
buildkite/tests/bulb-bulb-full-sql-logic-tests Passed (0 seconds)
Details
buildkite/tests/bulb-short-sql-logic-tests Passed (1 minute, 9 seconds)
Details
buildkite/tests/cargo-test Passed (44 seconds)
Details
buildkite/tests/docker-build Passed (12 minutes, 24 seconds)
Details
buildkite/tests/face-with-monocle-miri-test Passed (1 minute, 4 seconds)
Details
buildkite/tests/metabase-demo Passed (42 seconds)
Details
buildkite/tests/paperclip-clippy-and-doctests Passed (1 minute, 49 seconds)
Details
buildkite/tests/pipeline Passed (15 seconds)
Details
buildkite/tests/racing-car-testdrive Passed (3 minutes, 13 seconds)
Details
buildkite/tests/shower-streaming-demo Passed (27 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
netlify/materializeinc/deploy-preview Deploy preview canceled.
Details
@benesch benesch deleted the benesch:func-errors branch Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.