Improve compatibility with Apache Superset by showing CrateDB table metadata in the SQL Lab editor

[amotl]

Hi,

this patch is a refresh of 0212487, 8dfcf80, efe8bfb contributed by @Aymaru on behalf of #395 - thank you again!

According to the description of the original pull request, it aims to improve compatibility with Apache Superset by returning a list instead of a set from get_pk_constraint(). Otherwise, Apache Superset would not show CrateDB table metadata in the SQL Lab editor.

With kind regards,
Andreas.

P.S.: The patch has not been verified together with Apache Superset yet. Maybe you can have a look, @rafaelasantana, @hammerhead or @proddata?

Installing the code from the corresponding development branch conveniently would work like this:

pip install --upgrade "git+https://github.com/crate/crate-python@amo/fix-apache-superset-metadata#egg=crate[sqlalchemy]"

[seut]

Looks good to me, but would be nice to verify that this solves the issue first.
Is the pgsql dialect also returning a list here? (if so I'd assume it as verified :)

[amotl]

With this patch, we are observing a regression. Where the primary key information was able to be inspected beforehand, with the patch it does not.

You can see this on those screenshots.

Before

After

[amotl]

Maybe you can have a look at this again, @Aymaru? It looks like there is already a regression with this part of the patch, separated from #395.

[amotl]

Hm. @proddata pointed out that the documentation of get_pk_constraint clearly states:

Given a Connection, a string table_name, and an optional string schema, return primary key information as a dictionary with these keys:

• constrained_columns: a list of column names that make up the primary key.
• name: optional name of the primary key constraint.

So, I figure that returning constrained_columns as a list should cause no troubles. Instead, it should be compliant with the specification even better.

Why would it stop working with Apache Superset then?

[amotl]

Dear @Aymaru,

can I humbly ask you to have a look at this patch again? I am wondering if I correctly captured the gist of that part of your patch, which was originally submitted at #395.

[The patch] aims to improve compatibility with Apache Superset by returning a list instead of a set from get_pk_constraint(). Otherwise, Apache Superset would not show CrateDB table metadata in the SQL Lab editor.

Right now, we are actually observing the opposite behavior with this patch: It works without, but doesn't when applied. Maybe Apache Superset has advanced in this area in the meanwhile?

Maybe you can observe a flaw here, or maybe you have another explanation for the behavior of Apache Superset on this matter. Any help or support will be greatly appreciated.

With kind regards,
Andreas.

[amotl]

That test will now become flaky. The sample where it happened was on Python 3.10 with SQLAlchemy 1.3.24.

 Failure in test test_primary_keys (crate.client.sqlalchemy.tests.dialect_test.DialectTest)
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/unittest/case.py", line 591, in run
    self._callTestMethod(testMethod)
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/unittest/case.py", line 549, in _callTestMethod
    method()
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/unittest/mock.py", line 1369, in patched
    return func(*newargs, **newkeywargs)
  File "/home/runner/work/crate-python/crate-python/src/crate/client/sqlalchemy/tests/dialect_test.py", line 81, in test_primary_keys
    eq_(insp.get_pk_constraint("characters")['constrained_columns'], ["id", "id2", "id3"])
  File "/home/runner/work/crate-python/crate-python/.venv/lib/python3.10/site-packages/sqlalchemy/testing/assertions.py", line 238, in eq_
    assert a == b, msg or "%r != %r" % (a, b)
AssertionError: ['id2', 'id', 'id3'] != ['id', 'id2', 'id3']

-- https://github.com/crate/crate-python/runs/6969929509#step:4:392

[amotl]

I've probed this once more to make sure it is the correct outcome.

Before

Without this patch, table metadata is displayed.

After

When running with this patch, table metadata stops being displayed.

[amotl]

Dear @Aymaru,

based on my findings above, we will not bring in this part of the patch #395 you submitted the other day.

Right now, we are actually observing the opposite behavior with this patch: It works without, but doesn't when applied.
Maybe Apache Superset has advanced in this area in the meanwhile?

Feel free to raise your voice if you think this should be evaluated again.

With kind regards,
Andreas.

P.S.: I will have a look at the other parts of your PR on behalf of a different patch.