Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 7, 2025

  • Replace cpp11::stop() calls with optimized R-based error handling
  • Add rapi_error_with_context() helper functions
  • Add rlang support with graceful fallback to base R
  • Fix compilation error: Add missing [[noreturn]] attributes to match header declarations
  • Use cpp11::package("duckdb")["rapi_error"] approach per feedback, removing static initialization for safety
  • Add third error handling function that accepts ErrorData and passes multiple fields to R error handlers
  • Extend R error handler signatures with default NULL values for additional ErrorData fields (error_type, raw_message, extra_info)
  • Update statement.cpp to use ErrorData directly instead of JSON conversion for richer error context
  • Add tests to verify extended function signatures work correctly
  • Test compilation and verify error handling works correctly

Key Changes

Enhanced Error Handling: Added a third rapi_error_with_context overload that accepts duckdb::ErrorData directly and extracts structured error information including:

  • Exception type (PARSER, CATALOG, CONVERSION, etc.)
  • Raw vs. processed error messages
  • Additional error metadata from ErrorData's extra_info map

Improved R Error Functions: Extended both rapi_error and rapi_error_rlang to accept optional parameters for richer error context while maintaining backward compatibility through default NULL values.

Direct ErrorData Usage: Replaced JSON error conversion in statement.cpp with direct ErrorData processing to preserve all available error information and context.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@Copilot Copilot AI changed the title [WIP] Communicate all errors via JSON Replace cpp11::stop() with ErrorData::ConvertErrorToJSON() and add context information Sep 7, 2025
@Copilot Copilot AI requested a review from krlmlr September 7, 2025 15:44
Copilot finished work on behalf of krlmlr September 7, 2025 15:44
@krlmlr
Copy link
Collaborator

krlmlr commented Sep 7, 2025

@copilot: Add braces when declaring a local variable in the default: branch . Replace the implementation of the error handlers with a forward to a new R function that calls stop() .

Copy link
Contributor Author

Copilot AI commented Sep 7, 2025

@copilot: Add braces when declaring a local variable in the default: branch . Replace the implementation of the error handlers with a forward to a new R function that calls stop() .

I've addressed both parts of your feedback in commit ea70d91:

  1. Added braces to default: cases - Fixed all 5 locations where local variables were declared in default: branches without braces
  2. Simplified error handling - Replaced the complex JSON error processing with a simple R function rapi_error() that forwards to stop()

The error handlers now use a much simpler 3-line approach instead of the previous 15+ lines of DuckDB ErrorData/JSON processing.

@Copilot Copilot AI changed the title Replace cpp11::stop() with ErrorData::ConvertErrorToJSON() and add context information Replace cpp11::stop() with simplified R-based error handling and add context information Sep 7, 2025
Copilot finished work on behalf of krlmlr September 7, 2025 16:13
@krlmlr
Copy link
Collaborator

krlmlr commented Sep 7, 2025

@copilot: Pass two arguments to the R function. Look it up in the duckdb namespace.

Copy link
Contributor Author

Copilot AI commented Sep 7, 2025

@copilot: Pass two arguments to the R function. Look it up in the duckdb namespace.

I've updated the error handling implementation in commit 5582edc:

  1. Two arguments: Modified rapi_error() R function to accept separate context and message parameters instead of a single concatenated string
  2. duckdb namespace lookup: Changed C++ code to use cpp11::package("duckdb")["rapi_error"] instead of direct function lookup

The R function now receives structured arguments and handles the formatting, making the API cleaner and more composable.

Copilot finished work on behalf of krlmlr September 7, 2025 17:05
@krlmlr
Copy link
Collaborator

krlmlr commented Sep 7, 2025

@copilot: Forward calls from rapi_error_with_context to the other overload. Make the function pointer a static local variable. Rename rapi_error to rapi_error_base, create an rlang version that calls rlang::abort() and overwrite with rapi_error_base in .onLoad() if rlang is not installed.

Copy link
Contributor Author

Copilot AI commented Sep 7, 2025

@copilot: Forward calls from rapi_error_with_context to the other overload. Make the function pointer a static local variable. Rename rapi_error to rapi_error_base, create an rlang version that calls ...

I've implemented all the requested changes in commit d06a8e8:

  1. Forward calls: Exception overload now calls the string overload
  2. Static function pointer: R function lookup is cached as static local variable
  3. Renamed to rapi_error_base: Base function for simple stop() calls
  4. Added rlang support: rapi_error() uses rlang::abort() when available
  5. Conditional assignment: .onLoad() sets rapi_error = rapi_error_base when rlang unavailable

The error handling now has better performance with caching and graceful fallback between rlang and base R.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@Copilot Copilot AI changed the title Replace cpp11::stop() with simplified R-based error handling and add context information Replace cpp11::stop() with optimized R-based error handling with rlang support and context information Sep 7, 2025
Copilot finished work on behalf of krlmlr September 7, 2025 17:34
Copy link
Contributor Author

Copilot AI commented Sep 7, 2025

@copilot: Fix compilation errors.

Fixed compilation error in commit 658cd2d by adding missing [[noreturn]] attributes to the error function implementations to match the header declarations.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • cloud.r-project.org
    • Triggering command: /usr/lib/R/bin/exec/R -e install.packages('DBI',~+~repos='REDACTED') (dns block)
  • cran.r-project.org
  • cran.rstudio.com
    • Triggering command: /usr/lib/R/bin/exec/R -e install.packages(c('DBI',~+~'methods',~+~'utils'),~+~repos='REDACTED') (dns block)
  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of krlmlr September 7, 2025 18:20
@krlmlr krlmlr changed the title Replace cpp11::stop() with optimized R-based error handling with rlang support and context information feat: Raise errors from R code, will allow for richer context information Sep 7, 2025
@krlmlr
Copy link
Collaborator

krlmlr commented Sep 7, 2025

This is good in principle, but needs more thought:

  • Are we really going to deparse JSON and make it available to the R client?
  • What will that look like?
  • Anything else that we can improve -- cli formatting, for example?

Let's address this before moving here.

@krlmlr
Copy link
Collaborator

krlmlr commented Sep 9, 2025

@copilot: Add a third error handling function that accepts ErrorData and, instead converting the error to JSON, passes this as multiple fields to the R error handlers. Extend the signature of the R error handlers as appropriate, with default NULL values.

Copy link
Contributor Author

Copilot AI commented Sep 9, 2025

@copilot: Add a third error handling function that accepts ErrorData and, instead converting the error to JSON, passes this as multiple fields to the R error handlers. Extend the signature of the R er...

I've implemented the requested changes in commit cfc728a:

  1. Added third error handling function: Created rapi_error_with_context(const std::string &context, const duckdb::ErrorData &error_data) that accepts ErrorData directly
  2. Extended R function signatures: Both rapi_error and rapi_error_rlang now accept additional parameters with default NULL values: error_type, raw_message, and extra_info
  3. Rich error information: The new function extracts ExceptionType, raw message, and extra_info from ErrorData and passes them as separate fields to R instead of converting to JSON
  4. Updated usage: Replaced JSON conversion calls in statement.cpp with direct ErrorData usage

The rlang version now provides structured error information with context, error type, raw message, and additional details when available.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • cran.rstudio.com
    • Triggering command: /usr/lib/R/bin/exec/R -e install.packages(c('DBI',~+~'methods',~+~'utils'),~+~repos='REDACTED',~+~lib='~/R/library') (dns block)
  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@Copilot Copilot AI changed the title feat: Raise errors from R code, will allow for richer context information feat: Add rich ErrorData-based error handling with structured error information Sep 9, 2025
Copilot finished work on behalf of krlmlr September 9, 2025 04:58
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@krlmlr krlmlr marked this pull request as ready for review September 9, 2025 17:40
@krlmlr krlmlr merged commit 60b69ea into main Sep 9, 2025
5 checks passed
@krlmlr krlmlr deleted the copilot/fix-519 branch September 9, 2025 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants