Consideration for converting IndexMethod.hash in sqlite transactions

[Skelmis]

Currently I run various text columns with indexes on them, ideally IndexMethod.hash as this provides good equality matching lookups. I run into an issue with this however during tests which run on sqlite, as Piccolo will exit with an error if this index method is used. This is really not ideal and basically means I cannot use this index method at all as it is not feasible to run postgres just for test cases.

Possible I'm thinking of maybe adding another way to say to Piccolo, hey please map this to a usable sqlite index instead of erroring, either by app level variable of a column argument like sqlite_index_method. I.e. In my use case it is fine for IndexMethod.hash to be mapped to IndexMethod.btree if the DB is sqlite, as long

I also may be overlooking something, perhaps there is a way to do this already?

Thoughts?

[sinisaos]

@Skelmis Yes, Piccolo will raise a ValueError because SQLite does not supports hash indexes. That came from this section

piccolo/piccolo/query/methods/create_index.py

Lines 66 to 68 in 11ada0b

	method_name = self.method.value
	if method_name != "btree":
	raise ValueError("SQLite only support btree indexes.")

If we replace ValueError by assigning a method name to the given index method (in SQLite it is btree) like this

method_name = self.method.value
if method_name != "btree":
    method_name = self.method.value
    colored_warning(
        message=(
            "=> A btree index was not used when creating the table. "
            "Piccolo converts the index method to a btree because "
            "it is only method supported by SQLite."
        ),
        level=Level.high,
    )

the engine will replace the index name of the created table with the default one. Should we do this or not?

[Skelmis]

I would quite like it, but it should probably raise a warning so that people do actually know that it's happening as well as being documented behaviour

[sinisaos]

@dantownsend what do you think about this change?

[dantownsend]

Hmm, yeah - tricky. I wish SQLite was closer to Postgres in terms of functionality.

The easiest solution is to use Postgres for unit tests too. That's what I typically do.

We could have some kind of IndexMethod.default or something, which has different values for Postgres and SQLite. I just worry how much additional complexity we are adding.

If we just change the index method during tests, then our code needs to be aware of when it's running in a test or not.

[sinisaos]

The easiest solution is to use Postgres for unit tests too. That's what I typically do.

@dantownsend I absolutely agree that it is best to have Postgres for both the main and test databases (same columns types, same indexes types, etc.) because Postgres has a lot of features and is more advanced than SQLite. But in this case I think the question is what if it is not feasible to run Postgres for test cases. In that case this change is useful (for rare cases) because it does not affect other features of Postgres and SQLite.

We could have some kind of IndexMethod.default or something, which has different values for Postgres and SQLite. I just worry how much additional complexity we are adding.

I think this change is sufficient (I may be wrong, but I don't see any potential problems) because it just replaces ValueError by adding the default SQLite index type which is btree. This change does not affect other features. I just tried this change and the SQLite tests ran without any problems (the warning appears, but the tests run successfully) and before the tests ended with errors due to ValueError.

[dantownsend]

Yes, I see your point. It's not clear to me if it's better to raise an exception, or just fall back to a supported index type. I wonder what happens if SQLite supports hash indexes in the future.

What about giving the user an option to explicitly define a fallback?

class MyTable(Table):
    my_column = Varchar(index_method=[Index.hash, Index.btree])

The first one gets applied if possible, and it falls back to ones lower in the list depending on the database.

[sinisaos]

I wonder what happens if SQLite supports hash indexes in the future.

You are right. I hadn't thought about it.

What about giving the user an option to explicitly define a fallback?

class MyTable(Table):
my_column = Varchar(index_method=[Index.hash, Index.btree])
The first one gets applied if possible, and it falls back to ones lower in the list depending on the database.

That's a good option, but it seems complicated to me for a case that will probably be rare. I think in that case we need to change the migrations as well because index_method will not be an Enum value but a list of Enum values. I don't know, that seems like too much work. I know it's not a popular and good solution (especially if there is a lot of tables), but for that rare case the user can always copy their tables into conftest.py and remove the index_method that is not btree and the tests in SQLite will pass.
Maybe we should close this issue as not planned and leave everything as it was before (raise ValueError).

[Skelmis]

Is the migration engine not aware of the database, I thought it was. But yea I've had heaps of issues running postgres for tests with asyncpg causing a bunch of issues based on event loop shenanigans where as sqlite just works.

Would perhaps adding an extra column argument of like sqlite_index_method be more appealing perhaps? Then the only change that is required is at migration time the migration engine should prefer the usage of that column if set over the overarching index method column

[sinisaos]

I've never had any problems testing in Postgres (both sync and async) and I think the main advantage of this approach is that we have the same main and test database.
We can also not check index_method at all for SQLite because it is always btree for now (if any new method is added to SQLite there will probably be a way to choose and check index_method similar to Postgres). Anyway, whatever is decided I agree with that, but I wouldn't like the solution which adds complexity to Piccolo.

[Skelmis]

Original Post

Heres what brings this on for me:

piccolo_conf_test.py

import os

from dotenv import load_dotenv
from piccolo.conf.apps import AppRegistry
from piccolo.engine import SQLiteEngine, PostgresEngine

load_dotenv()

DB = PostgresEngine(
    config={
        "database": "tests",
        "user": os.environ["POSTGRES_USER"],
        "password": os.environ["POSTGRES_PASSWORD"],
        "host": os.environ["POSTGRES_HOST"],
        "port": int(os.environ["POSTGRES_PORT"]),
    },
)

# DB = SQLiteEngine(path="test_suite.sqlite")
APP_REGISTRY = AppRegistry(
    apps=[
        "home.piccolo_app",
        "piccolo_admin.piccolo_app",
        "piccolo_api.mfa.authenticator.piccolo_app",
    ]
)

I then have this in my conftest file which is coupled together from various areas

@pytest.fixture(scope="function", autouse=True)
async def configure_testing():
    # Setup DB per test
    with set_env_var(var_name="PICCOLO_CONF", temp_value="piccolo_conf_test"):
        from app import app

        app.debug = True

        refresh_db()
        tables = Finder().get_table_classes()
        # Ensure DB is cleared from any prior hanging tests
        await drop_db_tables(*tables)

        # Set up DB
        await create_db_tables(*tables)
        await scaffold_db()
        yield
        await drop_db_tables(*tables)

But my issue is that I keep getting errors like this

So in classic 'now I am sharing it I've fixed it', I've got my test cases working with postgres by removing the yield from the original post.

I would still be keen for a way to have sqlite not just error with unsupported hash types though.

We can also not check index_method at all for SQLite because it is always btree for now (if any new method is added to SQLite there will probably be a way to choose and check index_method similar to Postgres). Anyway, whatever is decided I agree with that, but I wouldn't like the solution which adds complexity to Piccolo.

I think this could perhaps be good? We could still check it, and if its not btree just inform the user that we have changed it to btree and maybe link them to this issue

[sinisaos]

Here is an example of postgres tests for sync and async. You can also use the table finder if you prefer. I've never had any problems.