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

[CALCITE-6168] RexExecutor can throw during compilation #3588

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

mihaibudiu
Copy link
Contributor

No description provided.

@mihaibudiu
Copy link
Contributor Author

Maybe I should refine the catch block to catch fewer exceptions.

Copy link

sonarcloud bot commented Dec 19, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
90.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -1418,6 +1418,14 @@ protected static Calendar getCalendarNotTooNear(int timeUnit) {
}
}

/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-6168">[CALCITE-6168]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this test belongs here, it's not a test case for a specific operator. I think there is a test class called RexExecutorTest that might be better.

@mihaibudiu
Copy link
Contributor Author

Thank you @tanclary for the review, I have pushed one more commit which addresses your suggestion.

@mihaibudiu
Copy link
Contributor Author

Can someone please review this PR which fixes a bug?

Copy link
Contributor

@tanclary tanclary left a comment

Choose a reason for hiding this comment

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

LGTM

ise = (IllegalStateException) x;
break;
}
if (x.getCause() == x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just to prevent SO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have seen this pattern used elsewhere in this function, so I have followed it. I am not sure whether a Throwable can have itself as a cause. It looks like the Java APIs make that possible, since you can create a Throwable and assign a cause to it later using initCause.

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@mihaibudiu mihaibudiu merged commit ba9c840 into apache:main Jan 26, 2024
15 of 16 checks passed
@mihaibudiu mihaibudiu deleted the issue6168 branch January 26, 2024 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants