Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update SQLAlchemy, fix tests #104

Conversation

richardscholtens
Copy link
Contributor

@richardscholtens richardscholtens commented Dec 5, 2023

  • Fix all test_aggregate_graphs
  • Fix all test_store_performance
  • Fix all test_type_to_term_combination
  • Update SQLAlchemy to version 2.0.23
  • Refactor sqlalchemy statements

@richardscholtens richardscholtens force-pushed the bugfix/issue-100-update-sqlalchemy-to-version-2.0.23 branch from e2777e9 to 5921603 Compare December 5, 2023 13:02
@mwatts15
Copy link
Collaborator

@richardscholtens: Sorry, I'm just getting back to this. Unfortunately Actions still turns up a test failure.

@amotl amotl mentioned this pull request Dec 18, 2023
@richardscholtens
Copy link
Contributor Author

I have been busy lately but I am still keen on finishing my contribution. However, I seem to be stuck at a certain test. The test now fail on for PostgreSQL for test SQLAPgSQLContextTestCase namely testTriples. Every assert passes except the last one:

        # all unbound without context, same result!
>       asserte(len(list(triples((Any, Any, Any)))), 7)

It states it cannot find the triples within the default context. Still trying to figure what is going wrong here.

@richardscholtens richardscholtens force-pushed the bugfix/issue-100-update-sqlalchemy-to-version-2.0.23 branch 2 times, most recently from c58c32e to 7e880c0 Compare January 19, 2024 10:08
from_obj=[table])

all_table_columns = [c for c in table.columns] + [expression.literal_column("NULL").label("objlanguage"), expression.literal_column("NULL").label("objdatatype")]
select_clause = expression.select(*all_table_columns).select_from(table).where(whereClause)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

select_clause = expression.select(*all_table_columns).select_from(table)
if whereClause is not None:
    select_clause = select_clause.where(whereClause)

I had to hunt for a while but this is what is hit by that part of the test triples cases for all free variables in the query. I checked the compiled SQL by casting the select_clause to a string and I saw that because the whereClause is just None using SQLA's where() method yields a compiled WHERE NULL which blocks everything in the result set.

Copy link
Contributor

@amotl amotl Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there. I've submitted the fix @jfahne outlined above (thank you very much!) targeted to your branch, @richardscholtens. Maybe you can bring it in and refresh your patch over here?

Copy link
Contributor Author

@richardscholtens richardscholtens Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amotl, I believe I have added your two commits. However, I do not see any fix in your commits for the SQL problem. @jfahne did not provide a fix as far I can tell. He only states where the problem lies. Please correct me if I'm wrong. Maybe I missed something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a if else statement now and that seems to work for the PostgreSQL database for now:

        elif tableType == ASSERTED_NON_TYPE_PARTITION:
            all_table_columns = [c for c in table.columns] + [expression.literal_column("NULL").label("objlanguage"), expression.literal_column("NULL").label("objdatatype")]
            if whereClause is not None:
                select_clause = expression.select(*all_table_columns).select_from(table).where(whereClause)
            else:
                select_clause = expression.select(*all_table_columns).select_from(table)
        **selects.append(select_clause)**

@amotl

This comment was marked as resolved.

@amotl

This comment was marked as duplicate.

* Not all tests fixed yet
* Fix all `test_aggregate_graphs`
* Fix all `test_store_performance`
* Fix all `test_type_to_term_combination`
* Update SQLAlchemy to version 2.0.23
* Refactor sqlalchemy statements
* Add docker-compose.yml
* Add .env.sample
@richardscholtens richardscholtens force-pushed the bugfix/issue-100-update-sqlalchemy-to-version-2.0.23 branch from 7e880c0 to df3e084 Compare February 6, 2024 09:21
@richardscholtens
Copy link
Contributor Author

richardscholtens commented Feb 9, 2024

I did another commit for tox linting error @mwatts15

@mwatts15 mwatts15 merged commit 99f4689 into RDFLib:develop Feb 10, 2024
13 checks passed
@mwatts15
Copy link
Collaborator

Well done, @richardscholtens! Good team work, @jfahne, @amotl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants