refactor(table-api): unify exception type for all backends to `TableNotFound` when a table does not exist #9695

ncclementi · 2024-07-25T22:17:14Z

TODO:

BREAKING CHANGE: Missing tables now all raise a single Ibis exception. Library code that catches backend-specific exceptions to handle missing tables will need to be adjusted to use TableNotFound instead.

ibis/backends/mysql/__init__.py

cpcloud

Couple changes needed, thanks for doing this!

ibis/backends/mysql/__init__.py

ibis/backends/polars/__init__.py

ibis/backends/postgres/tests/test_client.py

ibis/backends/trino/tests/test_client.py

ibis/common/exceptions.py

ncclementi · 2024-07-31T21:27:10Z

ibis/backends/druid/__init__.py

+                .sql(self.dialect)
+            )
+        except ProgrammingError as e:
+            if re.search(r"\bINVALID_INPUT\b", str(e)):


I wasn't able to get anything better than this matching, if anyone has any ideas, suggestions welcome

ncclementi · 2024-07-31T21:28:07Z

ibis/backends/flink/__init__.py

+        except Py4JJavaError as e:
+            # This seems to msg specific but not sure what a good work around is
+            if re.search(
+                rf"Table `{re.escape(table_name)}` was not found",


I don't like this, but I'm not sure if there is a better way to get the error matching here. Suggestions welcomed.

@cpcloud do we think this is alright for flink? It's a bit specific but I didn't find an alternative, If so I'll remove the comment here

It seems like there's some poking around the java object that could be done.

I tried multiple thinks, I wasn't able to land in anything better than this, I tried inspecting e.java_exception to get the getSQLState but I wasn't able. Maybe I'm missing something.

I went ahead and changed this to catch the error, and then check for table existence instead of matching.

INVALID_INPUT is way too general of a string to match on in any case.

Sorry, I was commenting on the druid one...this is the flink one.

ibis/backends/pyspark/__init__.py

ncclementi · 2024-07-31T21:32:59Z

I think this is good to get a first path of review. I left some comments on the cases where the exception caught is not great/pretty and we are doing some for of string matching. If anyone has better ideas for those, suggestions are more than welcome.

Notes/help needed:

Exasol and Impala don't run on mac arm64 which makes it harder to test/fix those.
How do we trigger bigquery and snowflake to run?
Not quite sure how to fix the error for the PySpark 3.3.3 job.
druid window function fail: before there was a mark notimpl that raised PyDruidProgrammingError but after the addition of TableNotFound that revealed that some were because the table wasn't found and other is raised here:

pydruid.db.exceptions.ProgrammingError: druidException (INVALID_INPUT): The query 
contains window functions; To run these window functions, specify [enableWindowing]
in query context. (line [1], column [256])

one solution is to add also PyDruidProgrammingError to the marker, like

pytestmark = [
    pytest.mark.notimpl(
        ["druid"], raises=(com.OperationNotDefinedError, com.TableNotFound, PyDruidProgrammingError)
    )
]

is this the right approach?

Co-authored-by: Phillip Cloud <417981+cpcloud@users.noreply.github.com>

ibis/backends/tests/test_client.py

cpcloud · 2024-08-01T17:08:22Z

The PySpark error class import needs an ImportError guard to handle the module path change from 3.3 to 3.5.

There are some examples around the codebase for this that were recently added.

cpcloud · 2024-08-01T18:35:25Z

Added a Snowflake implementation. It's ... something.

ncclementi · 2024-08-01T18:35:26Z

I added this to the 10.0 milestone as it will have breaking changes for those using Ibis as a library.

cpcloud · 2024-08-01T18:45:11Z

Ok, BigQuery implementation is up.

cpcloud

Just a couple more missing from es

ibis/backends/pyspark/__init__.py

ibis/backends/snowflake/__init__.py

cpcloud · 2024-08-01T19:48:53Z

ibis/backends/pyspark/__init__.py

+                if e.getErrorClass() == "TABLE_OR_VIEW_NOT_FOUND":
+                    raise com.TableNotFound(table_name) from e


Suggested change

if e.getErrorClass() == "TABLE_OR_VIEW_NOT_FOUND":

raise com.TableNotFound(table_name) from e

if e.getErrorClass() == "TABLE_OR_VIEW_NOT_FOUND":

raise com.TableNotFound(table_name) from e

raise

You still need to raise for any other AnalysisException that isn't handled by the error class check.

Also this check doesn't seem to be working for PySpark 3.3.3, I'm looking into it.

Fixed in the latest commit.

ibis/backends/druid/__init__.py

ibis/backends/flink/__init__.py

ncclementi · 2024-08-01T21:09:20Z

ibis/backends/druid/__init__.py

@@ -126,6 +126,21 @@ def _get_schema_using_query(self, query: str) -> sch.Schema:
            schema[name] = dtype
        return sch.Schema(schema)

+    def _table_exists(self, name: str):


nice, thanks for adding this. It's way more clear.

cpcloud · 2024-08-01T21:25:12Z

I think this is good to go.

What an absolute hellscape this problem is.

Thanks for powering through @ncclementi!

ncclementi · 2024-08-01T21:58:30Z

Thanks @cpcloud, great job you too! Thanks for the pairing and troubleshooting. When we are ready for 10.0 we can rebase and deal with conflicts if we have to.

ncclementi · 2024-09-13T16:48:11Z

Hey y'all I tried to rebase this locally and did not go well 😞 so I'd need some help getting this one up to mark to be able to consolidate changes.

cpcloud

Just a couple small things, but otherwise this LGTM.

ibis/backends/mysql/__init__.py

…otFound` when a table does not exist (ibis-project#9695) Co-authored-by: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Co-authored-by: Gil Forsyth <gil@forsyth.dev>

ncclementi commented Jul 26, 2024

View reviewed changes

ibis/backends/mysql/__init__.py Outdated Show resolved Hide resolved

cpcloud requested changes Jul 26, 2024

View reviewed changes

gforsyth mentioned this pull request Jul 31, 2024

feat: make Exceptions public #6757

Open

1 task

ncclementi commented Jul 31, 2024

View reviewed changes

ibis/backends/pyspark/__init__.py Outdated Show resolved Hide resolved

ncclementi marked this pull request as ready for review July 31, 2024 21:29

ncclementi and others added 19 commits August 1, 2024 11:59

refactor: unify exception when TableNotFound

a7b2efe

chore: update test_create_and_drop_table postgres

efb1381

chore: catch original mysql no table exc and reraise

e9604c8

chore: check table name is in the message

c8b1bde

Co-authored-by: Phillip Cloud <417981+cpcloud@users.noreply.github.com>

chore: update message to match regex

8a4f3c8

Co-authored-by: Phillip Cloud <417981+cpcloud@users.noreply.github.com>

chore: remove redundant attribute

fe24900

chore: fix exception catching in mysql

c5366e4

chore: fix clickhouse exception catch

5b0d6a6

chore: fix exception catch in datafusion

33f2fbb

chore: fix exception catch in pandas and dask

2f865da

chore: fix exception catching in mysql

33c7507

chore: fix exception catching in polars

ea4571b

chore: fix regex in trino test

f6f3103

chore: add comment about pyspark exception catch

14bb13c

chore: fix druid exception catch

b79ca92

chore: exception catch for flink

93f6810

chore: fix druid exception and test case

71368f7

chore: fix druid exception in test_window

00be355

chore: add error to noimpl param to druid test window

9dde9d4

ncclementi force-pushed the table-not-found branch from 3d01efc to 9dde9d4 Compare August 1, 2024 15:59

cpcloud reviewed Aug 1, 2024

View reviewed changes

ibis/backends/tests/test_client.py Outdated Show resolved Hide resolved

ncclementi added the breaking change Changes that introduce an API break at any level label Aug 1, 2024

chore(bigquery): raise tablenotfound and consolidate

afccae4

cpcloud requested changes Aug 1, 2024

View reviewed changes

ibis/backends/pyspark/__init__.py Outdated Show resolved Hide resolved

ibis/backends/snowflake/__init__.py Outdated Show resolved Hide resolved

cpcloud and others added 2 commits August 1, 2024 14:48

Apply suggestions from code review

33bf6a1

chore: fix typo

e501cc2

cpcloud requested changes Aug 1, 2024

View reviewed changes

chore(pyspark): handle the difference between 3.3 and 3.5

0541e17

cpcloud reviewed Aug 1, 2024

View reviewed changes

ibis/backends/druid/__init__.py Show resolved Hide resolved

cpcloud reviewed Aug 1, 2024

View reviewed changes

ibis/backends/flink/__init__.py Show resolved Hide resolved

cpcloud added 2 commits August 1, 2024 16:52

Apply suggestions from code review

ecc40e7

chore(druid): check for table existence instead of pattern matching

14ccba6

ncclementi commented Aug 1, 2024

View reviewed changes

chore: comment about flink; ignore case and table name

a087c14

jcrist mentioned this pull request Sep 9, 2024

perf(backends): speed up most memtable existence checks #10067

Merged

ncclementi and others added 2 commits September 13, 2024 12:31

chore: fix conflicts

1e2992f

chore: swap in common exception for tablenotfound

061860a

ncclementi mentioned this pull request Sep 13, 2024

refactor: standardize _in_memory_table_exists #10123

Open

cpcloud approved these changes Sep 14, 2024

View reviewed changes

ibis/backends/mysql/__init__.py Outdated Show resolved Hide resolved

ibis/backends/mysql/__init__.py Outdated Show resolved Hide resolved

chore: address review comments

c22979f

cpcloud approved these changes Sep 16, 2024

View reviewed changes

cpcloud changed the title ~~refactor: unify exception when TableNotFound~~ refactor(table-api): unify exception type for all backends to TableNotFound when a table does not exist Sep 16, 2024

cpcloud merged commit 0c49e3b into ibis-project:main Sep 16, 2024
76 checks passed

cpcloud mentioned this pull request Sep 16, 2024

test(bigquery): check the correct exception for missing tables #10137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(table-api): unify exception type for all backends to `TableNotFound` when a table does not exist #9695

refactor(table-api): unify exception type for all backends to `TableNotFound` when a table does not exist #9695

ncclementi commented Jul 25, 2024 •

edited by cpcloud

Loading

cpcloud left a comment

ncclementi Jul 31, 2024

ncclementi Jul 31, 2024

ncclementi Aug 1, 2024

cpcloud Aug 1, 2024

ncclementi Aug 1, 2024

cpcloud Aug 1, 2024

cpcloud Aug 1, 2024

ncclementi commented Jul 31, 2024 •

edited

Loading

cpcloud commented Aug 1, 2024 •

edited

Loading

cpcloud commented Aug 1, 2024

ncclementi commented Aug 1, 2024

cpcloud commented Aug 1, 2024

cpcloud left a comment

cpcloud Aug 1, 2024

cpcloud Aug 1, 2024

cpcloud Aug 1, 2024

ncclementi Aug 1, 2024

cpcloud commented Aug 1, 2024

ncclementi commented Aug 1, 2024

ncclementi commented Sep 13, 2024

cpcloud left a comment

		if e.getErrorClass() == "TABLE_OR_VIEW_NOT_FOUND":
		raise com.TableNotFound(table_name) from e

refactor(table-api): unify exception type for all backends to TableNotFound when a table does not exist #9695

refactor(table-api): unify exception type for all backends to TableNotFound when a table does not exist #9695

Conversation

ncclementi commented Jul 25, 2024 • edited by cpcloud Loading

cpcloud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncclementi commented Jul 31, 2024 • edited Loading

cpcloud commented Aug 1, 2024 • edited Loading

cpcloud commented Aug 1, 2024

ncclementi commented Aug 1, 2024

cpcloud commented Aug 1, 2024

cpcloud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcloud commented Aug 1, 2024

ncclementi commented Aug 1, 2024

ncclementi commented Sep 13, 2024

cpcloud left a comment

Choose a reason for hiding this comment

refactor(table-api): unify exception type for all backends to `TableNotFound` when a table does not exist #9695

refactor(table-api): unify exception type for all backends to `TableNotFound` when a table does not exist #9695

ncclementi commented Jul 25, 2024 •

edited by cpcloud

Loading

ncclementi commented Jul 31, 2024 •

edited

Loading

cpcloud commented Aug 1, 2024 •

edited

Loading