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.

Sign up for GitHub

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

Jump to bottom

R: Fix core dump when failing to evaluate ALTREP query #8600

Merged
merged 4 commits into from Aug 18, 2023
Merged

R: Fix core dump when failing to evaluate ALTREP query #8600

merged 4 commits into from Aug 18, 2023

Conversation

krlmlr
Copy link
Collaborator

@krlmlr krlmlr commented Aug 17, 2023

This affects ~100 of 4500 of dplyr's reverse dependencies.

@krlmlr krlmlr requested a review from hannes August 17, 2023 08:16
@krlmlr
Copy link
Collaborator Author

krlmlr commented Aug 17, 2023

We can only use cpp11::stop() in functions that are wrapped by cpp11.

@krlmlr krlmlr mentioned this pull request Sep 13, 2023
Closed
1 task
Tmonster
Tmonster approved these changes Aug 17, 2023
View reviewed changes
Copy link
Contributor

@Tmonster Tmonster left a comment

Choose a reason for hiding this comment

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

lgtm. Looks like the test_relational.R file is all whitespace changes

@krlmlr
Copy link
Collaborator Author

krlmlr commented Aug 17, 2023

Almost. It's the last two commits, removing ::: and code style (whitespace and a few other tweaks).

@hannes hannes merged commit 65fbf5f into duckdb:master Aug 18, 2023
7 checks passed
@hannes
Copy link
Member

hannes commented Aug 18, 2023

Thanks, do we call cpp11::stop in other unwrapped functions?

@krlmlr
Copy link
Collaborator Author

krlmlr commented Aug 18, 2023

This is going to need some investigation. I've put it on my list.

krlmlr pushed a commit to krlmlr/duckdb that referenced this pull request Aug 21, 2023
@hannes @krlmlr
Merge pull request duckdb#8600 from krlmlr/b-altrep-unwind
17689fb 
R: Fix core dump when failing to evaluate ALTREP query
@krlmlr krlmlr mentioned this pull request Aug 21, 2023
Merged
krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023
@hannes @krlmlr
Indenting tests
a8cce34 
Remainder of the following two PRs:

Merge pull request duckdb/duckdb#8600 from krlmlr/b-altrep-unwind

R: Fix core dump when failing to evaluate ALTREP query

Merge pull request duckdb/duckdb#8647 from krlmlr/b-altrep-cpp11-wrap

Wrap ALTREP methods with BEGIN_CPP11 and END_CPP11
krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023
@hannes @krlmlr
Indenting tests
d0bf2e9 
Remainder of the following two PRs:

Merge pull request duckdb/duckdb#8600 from krlmlr/b-altrep-unwind

R: Fix core dump when failing to evaluate ALTREP query

Merge pull request duckdb/duckdb#8647 from krlmlr/b-altrep-cpp11-wrap

Wrap ALTREP methods with BEGIN_CPP11 and END_CPP11
krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023
@hannes @krlmlr
Indent tests
0d79f04 
Remainder of the following two PRs:

Merge pull request duckdb/duckdb#8600 from krlmlr/b-altrep-unwind

R: Fix core dump when failing to evaluate ALTREP query

Merge pull request duckdb/duckdb#8647 from krlmlr/b-altrep-cpp11-wrap

Wrap ALTREP methods with BEGIN_CPP11 and END_CPP11
krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023
@hannes @krlmlr
When projecting, use GetName instead of ToString()
edcaf64 
- Merge pull request duckdb/duckdb#8717 from Tmonster/projection_quoted_columns

- Indent tests, was part of of the following PRs which otherwise cancel themselves out:

- Merge pull request duckdb/duckdb#8600 from krlmlr/b-altrep-unwind: R: Fix core dump when failing to evaluate ALTREP query

- Merge pull request duckdb/duckdb#8647 from krlmlr/b-altrep-cpp11-wrap: Wrap ALTREP methods with BEGIN_CPP11 and END_CPP11

- Merge pull request duckdb/duckdb#8207 from krlmlr/b-copy-string: R: Avoid crash when finalizing

- Merge pull request duckdb/duckdb#8272 from krlmlr/b-warn-copy: Remove dangerous overload of cpp11::warning()
krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023
@hannes @krlmlr
When projecting, use GetName instead of ToString()
8cf6e11 
- Merge pull request duckdb/duckdb#8717 from Tmonster/projection_quoted_columns

- Indent tests, was part of of the following PRs which otherwise cancel themselves out:

- Merge pull request duckdb/duckdb#8600 from krlmlr/b-altrep-unwind: R: Fix core dump when failing to evaluate ALTREP query

- Merge pull request duckdb/duckdb#8647 from krlmlr/b-altrep-cpp11-wrap: Wrap ALTREP methods with BEGIN_CPP11 and END_CPP11

- Merge pull request duckdb/duckdb#8207 from krlmlr/b-copy-string: R: Avoid crash when finalizing

- Merge pull request duckdb/duckdb#8272 from krlmlr/b-warn-copy: Remove dangerous overload of cpp11::warning()
krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023
@hannes @krlmlr
When projecting, use GetName instead of ToString()
d3dc8d6 
- Merge pull request duckdb/duckdb#8717 from Tmonster/projection_quoted_columns

- Indent tests, was part of of the following PRs which otherwise cancel themselves out:

- Merge pull request duckdb/duckdb#8600 from krlmlr/b-altrep-unwind: R: Fix core dump when failing to evaluate ALTREP query

- Merge pull request duckdb/duckdb#8647 from krlmlr/b-altrep-cpp11-wrap: Wrap ALTREP methods with BEGIN_CPP11 and END_CPP11

- Merge pull request duckdb/duckdb#8207 from krlmlr/b-copy-string: R: Avoid crash when finalizing

- Merge pull request duckdb/duckdb#8272 from krlmlr/b-warn-copy: Remove dangerous overload of cpp11::warning()
krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023
@hannes @krlmlr
When projecting, use GetName() instead of ToString()
9869d9d 
- Merge pull request duckdb/duckdb#8717 from Tmonster/projection_quoted_columns

- Indent tests, was part of of the following PRs which otherwise cancel themselves out:

- Merge pull request duckdb/duckdb#8600 from krlmlr/b-altrep-unwind: R: Fix core dump when failing to evaluate ALTREP query

- Merge pull request duckdb/duckdb#8647 from krlmlr/b-altrep-cpp11-wrap: Wrap ALTREP methods with BEGIN_CPP11 and END_CPP11

- Merge pull request duckdb/duckdb#8207 from krlmlr/b-copy-string: R: Avoid crash when finalizing

- Merge pull request duckdb/duckdb#8272 from krlmlr/b-warn-copy: Remove dangerous overload of cpp11::warning()
krlmlr pushed a commit to duckdb/duckdb-r that referenced this pull request Sep 5, 2023
@hannes @krlmlr
When projecting, use GetName() instead of ToString()
b5f2425 
- Merge pull request duckdb/duckdb#8717 from Tmonster/projection_quoted_columns

- Indent tests, was part of of the following PRs which otherwise cancel themselves out:

- Merge pull request duckdb/duckdb#8600 from krlmlr/b-altrep-unwind: R: Fix core dump when failing to evaluate ALTREP query

- Merge pull request duckdb/duckdb#8647 from krlmlr/b-altrep-cpp11-wrap: Wrap ALTREP methods with BEGIN_CPP11 and END_CPP11

- Merge pull request duckdb/duckdb#8207 from krlmlr/b-copy-string: R: Avoid crash when finalizing

- Merge pull request duckdb/duckdb#8272 from krlmlr/b-warn-copy: Remove dangerous overload of cpp11::warning()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tmonster Tmonster Tmonster approved these changes

@hannes hannes Awaiting requested review from hannes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@krlmlr @hannes @Tmonster