RDFLib / rdflib-sqlalchemy

RDFLib store using SQLAlchemy dbapi as back-end
Other
148 stars 34 forks source link

Violent deprecation warnings vs sqlalchemy 1.4.46 #100

Closed bollwyvl closed 8 months ago

bollwyvl commented 1 year ago

Use of rdflib_sqlalchemy.store.union_select with sqlalchemy 1.4.46 shows:

The legacy calling style of select() is deprecated and will be removed in SQLAlchemy 2.0.  Please use the new calling style described at select(). (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)

It's possible to hide these errors with SQLALCHEMY_SILENCE_UBER_WARNING=1, but it can be difficult to reason about this error, and how a downstream would be able to effectively do it.

mwatts15 commented 1 year ago

Hi, thanks for reporting this. I'll try to address the issue, but I'm short on time for rdflib-sqlalchemy and think the project could use a new maintainer. If you're interested, let me know.

On Fri, Jan 20, 2023, 13:43 Nicholas Bollweg @.***> wrote:

Use of rdflib_sqlalchemy.store.union_select with sqlalchemy 1.4.46 shows:

The legacy calling style of select() is deprecated and will be removed in SQLAlchemy 2.0. Please use the new calling style described at select(). (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)

It's possible to hide these errors with SQLALCHEMY_SILENCE_UBER_WARNING=1, but it can be difficult to reason about this error, and how a downstream would be able to effectively do it.

— Reply to this email directly, view it on GitHub https://github.com/RDFLib/rdflib-sqlalchemy/issues/100, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALLFSAVBOROUUL3FJJ3KF3WTLTHTANCNFSM6AAAAAAUB4FPIU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

richardscholtens commented 11 months ago

I am updating the library to use SQLAlchemy version 2.0.23. Doing so results in the failing of multiple tests. I believe this is also has to do with the deprecation of the select() and similar constructs. I already fixed the first test and will continue to fix the rest as well.

richardscholtens commented 11 months ago

@mwatts15 , so I fixed all tests but one. I'm still unable to get the test_contexts_result under test_sqlalchemy to work. The test is as follows:

class SQLATestCase(unittest.TestCase):
    identifier = URIRef("rdflib_test")
    dburi = Literal("sqlite://")

    def setUp(self):
        self.store = plugin.get(
            "SQLAlchemy", Store)(identifier=self.identifier, configuration=self.dburi)
        self.graph = ConjunctiveGraph(self.store, identifier=self.identifier)
        self.graph.open(self.dburi, create=True)

    def test_contexts_result(self):
        ctx_id = URIRef('http://example.org/context')
        g = self.graph.get_context(ctx_id)
        g.add((michel, likes, pizza))
        actual = list(self.store.contexts())
        self.assertEqual(actual[0], ctx_id)

So it seems that information about michel liking pizza is transferred in the graph. However, when retrieving the store.contexts() it fails to retrieve the data. Any ideas what might be the cause of this?

I added my commit in the links below.

Bugfix branch issue 100 Bugfix commit

richardscholtens commented 11 months ago

So it does seems to work when the test gets a adjusted a little bit.

def test_contexts_result(self):
    ctx_id = URIRef('http://example.org/context')
    g = self.graph.get_context(ctx_id)
    g.add((michel, likes, pizza))
    actual = list(self.store.contexts(triple=(michel, likes, pizza)))
    self.assertEqual(actual[0], ctx_id)

Here I fill in the parameter triple so it know exactly which triple to look for and only retrieves the contexts it is in. This means it does store the triple somewhere in the contexts. Also see this context function reference

I've also started a pull request.

mwatts15 commented 11 months ago

Good catch, Richard. It should definitely work without the triple argument passed in. My first guess at why it might not is a change in how SQLAlchemy generates the query or stores the triples. More verbose logging should help determine what the cause is.

Message ID: @.***>

richardscholtens commented 11 months ago

I believe it has to do with the following code:

# sql.py line 68
        elif select_type == CONTEXT_SELECT:
            select_clause = expression.select(*[table.c.context]).where(whereClause)

If I change it to the code below the test test_contexts_results works, but then test_contexts_with_triple fails.

# sql.py line 68
        elif select_type == CONTEXT_SELECT:
            select_clause = expression.select(table.c.context)
            if whereClause:
                select_clause = expression.select(table.c.context).where(whereClause)

New error:

    def __bool__(self):
>       raise TypeError("Boolean value of this clause is not defined")
E       TypeError: Boolean value of this clause is not defined
richardscholtens commented 11 months ago

Ah never mind. Already fixed it with;

if whereClause is not None:

I believe the pull request is ready for reviewing, @mwatts15

mwatts15 commented 11 months ago

Sounds good. I will try to merge in the next few days.

On Tue, Dec 5, 2023, 07:01 Richard Scholtens @.***> wrote:

Ah never mind. Already fixed it with;

if whereClause is not None:

— Reply to this email directly, view it on GitHub https://github.com/RDFLib/rdflib-sqlalchemy/issues/100#issuecomment-1840750209, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALLFSE6GUAVR7O4RNL7V23YH4LKLAVCNFSM6AAAAAAUB4FPIWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBQG42TAMRQHE . You are receiving this because you were mentioned.Message ID: @.***>

richardscholtens commented 11 months ago

Thanks! Let me know if there are any issues.

richardscholtens commented 11 months ago

I noticed I still had an old commit message in the PR. Updated this PR comment.

richardscholtens commented 9 months ago

@mwatts15, I pushed a new fix for one of the bugs. All Postgres test seem to work now. Did not test on the other databases. Could you rerun the pipeline for me?

PR-104

richardscholtens commented 8 months ago

@mwatts15 , I believe this issue can be closed now.