CLN: Deprecate and standardize exists_table #2905

datapythonista · 2021-08-16T00:19:38Z

Not very useful, and backends reimplement it for no reason.

jreback · 2021-08-17T13:06:22Z

can you rebase

jreback · 2021-08-30T13:09:05Z

can you rebase

jreback · 2021-09-10T20:00:14Z

@datapythonista can you rebase here

github-actions · 2021-09-22T00:16:58Z

Unit Test Results

      19 files       19 suites 1h 47m 15s ⏱️
10 777 tests   8 735 ✔️   2 042 💤 0 ❌
54 579 runs 43 801 ✔️ 10 778 💤 0 ❌

Results for commit ffe03a2.

♻️ This comment has been updated with latest results.

…ng table

cpcloud · 2021-09-22T12:58:48Z

ibis/backends/impala/tests/test_client.py

@@ -266,11 +266,15 @@ def test_close_drops_temp_tables(con, test_data_dir):

    table = con.parquet_file(hdfs_path)

-    name = table.op().name
-    assert con.exists_table(name) is True
+    qualified_name = table.op().name


After reading through this test, and the code path it's trying to exercise I no longer think this test is useful nor is there any real way to make this test reliable.

The main question here is: what should happen to any object that is referencing a temporary object from an open database connection, when that connection is closed?

I don't think this behavior is well defined, nor should we try to define it. However, since we're depending on __del__ to clean up ImpalaTemporaryTables (roughly speaking) when they are garbage collected, we should definitely not be dropping tables when a connection is closed.

I will submit a PR to address that.

In this PR I think you can remove this test.

Ok, so this is actually a different problem (though the lifetime problem still exists).

The issue is that con.exists_table(name) has different semantics than what you're inlining here.

exists_table(like=name) will return true if the list of tables matching name is non-empty.

What you've written here is to assert that whatever tables are listed contains name as an exact match.

The important thing to note is that this is only true when con is connected to the same database that name is contained in. Otherwise, the test will fail.

The solution is to write the assertion as assert con.list_tables(like=name).

jreback

can you assert the FutureWarning in a test (I think this is how we handled other deprecated items)

also add a release note

cpcloud · 2021-09-22T14:10:24Z

ibis/backends/impala/tests/test_client.py

@@ -266,11 +266,15 @@ def test_close_drops_temp_tables(con, test_data_dir):

    table = con.parquet_file(hdfs_path)

-    name = table.op().name
-    assert con.exists_table(name) is True
+    qualified_name = table.op().name


Ok, so this is actually a different problem (though the lifetime problem still exists).

The issue is that con.exists_table(name) has different semantics than what you're inlining here.

exists_table(like=name) will return true if the list of tables matching name is non-empty.

What you've written here is to assert that whatever tables are listed contains name as an exact match.

The important thing to note is that this is only true when con is connected to the same database that name is contained in. Otherwise, the test will fail.

The solution is to write the assertion as assert con.list_tables(like=name).

datapythonista · 2021-09-22T17:34:09Z

I saw the like issue. I think it's a bug, but I didn't overthink it, as exists_table is being deprecated anyway. I don't think con.exists_able('cat') should be true, if a table catalog exists, but no table cat. This is indirectly being fixed with this clean up.

But I don't think the problem with the test is that, I did try what you say, and also printed the tables in the database to see if it was a prefix problem, or something related, but the table just doesn't exist.

datapythonista · 2021-09-22T17:39:54Z

This is green now. If it's still find to delete the tricky test, this should be ready.

cpcloud · 2021-09-22T18:38:29Z

I saw the like issue. I think it's a bug, but I didn't overthink it, as exists_table is being deprecated anyway. I don't think con.exists_able('cat') should be true, if a table catalog exists, but no table cat. This is indirectly being fixed with this clean up.

Can you clarify this a bit? Ibis doesn't currently have this behavior. Here's me executing a few exists_table calls in the test:

(Pdb) list
265
266         table = con.parquet_file(hdfs_path)
267
268         name = table.op().name
269         breakpoint()
270  ->     assert con.exists_table(name) is True
271         con.close()
272
273         assert not con.exists_table(name)
274
275
(Pdb) con.list_tables()
['alltypes', 'functional_alltypes', 'tpch_customer', 'tpch_lineitem', 'tpch_nation', 'tpch_orders', 'tpch_part', 'tpch_partsupp', 'tpch_region', 'tpch_region_avro', 'tpch_supplier']
(Pdb) con.exists_table('all')
False
(Pdb) con.exists_table('func')
False
(Pdb) con.exists_table('tpch_')
False

But I don't think the problem with the test is that, I did try what you say, and also printed the tables in the database to see if it was a prefix problem, or something related, but the table just doesn't exist.

The table exists, it's just in a different database.

What's happening is the prefix is being split off by a regular expression and that prefix is used as the database in a

SHOW TABLES IN __ibis_tmp LIKE '__ibis_tmp_<SOME UUID>'

query.

The LIKE doesn't contain any wildcards, so it'll perform an exact match.

We should not delete the test until we understand why it fails.

ibis/backends/base/file/__init__.py

cpcloud · 2021-09-22T19:00:42Z

ibis/backends/base/__init__.py

+            '`name in client.list_tables()` instead.',
+            FutureWarning,
+        )
+        return name in self.client.list_tables(database=database)


Should this be implemented the way it was before as len(self.client.list_tables(like=name, database=database)) > 0?

name won't match tables in other databases, so I don't think you can use in here. Since this isn't doing something trivial, should we reconsider deprecating it?

Standardizing it seems like a good idea FWIW, just not sure about removal.

I don't fully understand what's Impala doing. I don't think other backends are affected.

We've got a syntax for checking other databases, name in con.list_tables(database='foo'). And I think it's better than this magic failing the test. Just the explicit syntax doesn't seem to work in Impala (or there was an error in my code when I tried that, but I don't think so). I'm using the previous syntax of checking the list of tables with the like param for now, so we can get tests passing.

I don't fully understand what's Impala doing. I don't think other backends are affected.

Did you read my comment?

And I think it's better than this magic failing the test.

Are you referring to the regular expression splitting?

Just the explicit syntax doesn't seem to work in Impala

Are you sure about that? Here's an example that seems to work just fine:

(Pdb) list 265 266 table = con.parquet_file(hdfs_path) 267 268 name = table.op().name 269 breakpoint() 270 -> assert con.exists_table(name) is True 271 con.close() 272 273 assert not con.exists_table(name) 274 275 (Pdb) con.list_tables('__ibis_tmp_81c73096e0924eafba359d2a1de85f85', database='__ibis_tmp') ['__ibis_tmp_81c73096e0924eafba359d2a1de85f85'] (Pdb) name '__ibis_tmp.`__ibis_tmp_81c73096e0924eafba359d2a1de85f85`'

Mhh, I see, you're right. I didn't fully understand the problem, now it's clear. Then, what's different in Impala is that other backends don't support fully qualified table names, as Impala handles in list_tables. Not sure why the test wasn't working when I splittted the table myself in the test, I guess I missed something.

In any case, I think this PR is fine now. Or is there anything else you would change?

cpcloud · 2021-09-22T21:21:03Z

I think this is good to go! @datapythonista Thanks for putting up with my reviews!

jreback

lgtm. can you rebase & also i think we have a link somewhere for the deprecated apis? pls add to that

datapythonista · 2021-09-22T22:40:01Z

Good point, added to #2863 and rebased

cpcloud · 2021-09-22T23:02:16Z

thanks @datapythonista!

CLN: Deprecate and standardize exists_table

72d1850

datapythonista added refactor Issues or PRs related to refactoring the codebase backends Issues related to all backends labels Aug 16, 2021

datapythonista added 2 commits August 16, 2021 17:49

Fixing tests

8fb19c0

Fixing test

e43397a

datapythonista added 4 commits August 17, 2021 09:36

Merge remote-tracking branch 'upstream/master' into exists_table

fa7c98c

Merge master

c264e4f

Fix for database parameter

2706d9a

Merge master

bab3e65

datapythonista mentioned this pull request Sep 6, 2021

RLS: 2.0 #2526

Closed

datapythonista added 4 commits September 9, 2021 09:35

Merge master

f490960

Removing hack after list_table clean up

291420f

Merge remote-tracking branch 'upstream/master' into exists_table

f32fb24

Adding exists_table implementation to impala client

37f367e

datapythonista mentioned this pull request Sep 10, 2021

ci: replace bespoke test munging with junit reporter #2957

Merged

datapythonista added 12 commits September 10, 2021 22:37

Merge master

046e038

Remove custom impala method

7a8e692

Improving failing test output

28fb3b4

Replacing in tests exists_table by in list_tables

b62eaaf

Allowing full qualified table names

b021f8c

Fixing tests

7340c4c

Fixing test

c82967d

Fix typo

7ae5c81

Reverting exists_table full qualified handling

909a012

Unqualifying table names to check if they exist

e8563ce

Fix typo

2c3f753

Adding verbosity to pytest

3dcea92

datapythonista added 2 commits September 12, 2021 23:56

Merge remote-tracking branch 'upstream/master' into exists_table

7210b8b

Merge master

af8bac9

datapythonista added 2 commits September 21, 2021 19:23

Adding debug info

e5f2e34

Testing if adding previous parameters to list_table returns the missi…

47368df

…ng table

cpcloud reviewed Sep 22, 2021

View reviewed changes

jreback suggested changes Sep 22, 2021

View reviewed changes

jreback added this to the Next release milestone Sep 22, 2021

cpcloud requested changes Sep 22, 2021

View reviewed changes

datapythonista added 2 commits September 22, 2021 12:00

Removing broken test, adding release note and test the future warning

787effc

Fixing file tests

40fd716

Restore failing test

07fcdf3

cpcloud reviewed Sep 22, 2021

View reviewed changes

ibis/backends/base/file/__init__.py Show resolved Hide resolved

cpcloud reviewed Sep 22, 2021

View reviewed changes

Considering a table exists using list_tables with like param

f11fcdd

cpcloud approved these changes Sep 22, 2021

View reviewed changes

jreback approved these changes Sep 22, 2021

View reviewed changes

Merge master

f542b6d

datapythonista mentioned this pull request Sep 22, 2021

refactor: remove deprecations #2863

Closed

4 tasks

cpcloud merged commit ffe03a2 into ibis-project:master Sep 22, 2021

cpcloud removed this from the Next release milestone Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: Deprecate and standardize exists_table #2905

CLN: Deprecate and standardize exists_table #2905

datapythonista commented Aug 16, 2021

jreback commented Aug 17, 2021

jreback commented Aug 30, 2021

jreback commented Sep 10, 2021

github-actions bot commented Sep 22, 2021 •

edited

Loading

cpcloud Sep 22, 2021 •

edited

Loading

cpcloud Sep 22, 2021

jreback left a comment

cpcloud Sep 22, 2021

datapythonista commented Sep 22, 2021

datapythonista commented Sep 22, 2021

cpcloud commented Sep 22, 2021 •

edited

Loading

cpcloud Sep 22, 2021

cpcloud Sep 22, 2021

cpcloud Sep 22, 2021

datapythonista Sep 22, 2021

cpcloud Sep 22, 2021

cpcloud Sep 22, 2021 •

edited

Loading

datapythonista Sep 22, 2021

cpcloud commented Sep 22, 2021

jreback left a comment

datapythonista commented Sep 22, 2021

cpcloud commented Sep 22, 2021

CLN: Deprecate and standardize exists_table #2905

CLN: Deprecate and standardize exists_table #2905

Conversation

datapythonista commented Aug 16, 2021

jreback commented Aug 17, 2021

jreback commented Aug 30, 2021

jreback commented Sep 10, 2021

github-actions bot commented Sep 22, 2021 • edited Loading

Unit Test Results

cpcloud Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista commented Sep 22, 2021

datapythonista commented Sep 22, 2021

cpcloud commented Sep 22, 2021 • edited Loading

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 Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcloud commented Sep 22, 2021

jreback left a comment

Choose a reason for hiding this comment

datapythonista commented Sep 22, 2021

cpcloud commented Sep 22, 2021

github-actions bot commented Sep 22, 2021 •

edited

Loading

cpcloud Sep 22, 2021 •

edited

Loading

cpcloud commented Sep 22, 2021 •

edited

Loading

cpcloud Sep 22, 2021 •

edited

Loading