SNOW-2241913: non-retryable error handling#3770
Conversation
sfc-gh-yuwang
requested review from
sfc-gh-jdu,
sfc-gh-jrose and
sfc-gh-mayliu
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
sfc-gh-yuwang
requested review from
sfc-gh-helmeleegy and
sfc-gh-mvashishtha
| - Hybrid execution mode is now enabled by default. Certain operations on smaller data will now automatically execute in native pandas in-memory. Use `from modin.config import AutoSwitchBackend; AutoSwitchBackend.disable()` to turn this off and force all execution to occur in Snowflake. | ||
| - Added a session parameter `pandas_hybrid_execution_enabled` to enable/disable hybrid execution as an alternative to using `AutoSwitchBackend`. | ||
| - Removed an unnecessary `SHOW OBJECTS` query issued from `read_snowflake` under certain conditions. | ||
| - Add non-retryable failure in `DataFrameReader.dbapi`(PuPr), such as syntax error in external data source SQL. |
There was a problem hiding this comment.
nit:
- this should be moved to snowpark python api updates
- the description is a little confusing, you could describe it like improve dbapi not to retry on non-retryable error
| pass | ||
|
|
||
|
|
||
| class SnowparkDataSourceNonRetryableException(SnowparkGeneralException): |
There was a problem hiding this comment.
this makes the exception class public api. is this class for internal usage only to differentiate from other error? if so we can make it private by _ SnowparkDataSourceNonRetryableException.
another way to is set a private memvar on SnowparkDataframeReaderException to differentiate retry from non-retry
the name sound general and more for internal usage, and if we want something for public customer facing exception, I personally prefer exception classes with concrete meanings which I think would take more time to design
There was a problem hiding this comment.
what about ingestion (copy into), if we have SnowparkSQLException, should we retry?
There was a problem hiding this comment.
I think non-retryable exception is for error that we know that it will not work even if we retry.
I think SnowparkSQLException is a very general failure that not only exception like SQL syntax error could trigger it.
For example, a udtf could fail with SnowparkSQLException because of python error that is retryable.
| raise ValueError("fetch size cannot be smaller than 0") | ||
| except Exception as exc: | ||
| if self.driver.non_retryable_error_checker(exc): | ||
| raise _SnowparkDataSourceNonRetryableException(exc) |
There was a problem hiding this comment.
as per our discussion, we do not need the private class and can just raise the existing reader exception here
| create_connection_oracledb, | ||
| table=ORACLEDB_TABLE_NAME, | ||
| predicates=["invalid syntax"], | ||
| ).collect() |
There was a problem hiding this comment.
Curious about why some tests use .collect() while others don't, my impression is that dbapi() is not lazily evaluated?
3 participants
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-2241913
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.