The issue outlines four filter expressions that need optimization.
The first one (label = <value>) is already supported.
I have added support for the last one (id/label/version/property != <value>).

It seems we need to discuss how to handle the following cases:

• <id/property/label> = <value 1> and <id/property/label> = <value 2>
• <id/property/label> = <value 1> or <id/property/label> = <value 2>

Specifically:

• Should we support only combining two expressions, or was the example just a demonstration, with the solution needing to be more flexible?
• To write tests, we must first merge Improved parsing attributes in tests javarosa#808, as wiring expressions with and/or will fail without this.

Another question, not directly related to any of the filter types:
we are calling [replacePartialElements](https://github.com/getodk/collect/blob/master/entities/src/main/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategy.kt#L52C36-L52C58). In which scenario would we encounter issues if we do not call it? I removed this call, and all tests passed, which was strange to me. I believe I understood its purpose, but now that all tests are passing without it, I feel a bit disoriented.

@seadowg, feel free to not only answer these questions but also take over if you don't have other pressing tasks. You might have intended to work on this yourself, and I took it on without asking. If that's the case, I hope at least what I’ve done will be helpful.

The first one (label = ) is already supported.

It's supported, but not optimized (compared to id = <value>). The current implementation will use the DB to find the tree references, but it does that by calling getEntities and then filtering them in memory which requires the whole entity list (from the DB) in memory (at least temporarily). We want to let SQLite handle the filtering here (like we do with ID) so we don't need to take that hit.

Should we support only combining two expressions, or was the example just a demonstration, with the solution needing to be more flexible?

For this issue, I'd only intended that we add support for those exact expressions. If it feels easier to you at this point to make the solution more general though, that'd be a bonus.

To write tests, we must first merge getodk/javarosa#808, as wiring expressions with and/or will fail without this.

Interesting! I'll have a look at that as soon as I can.

Another question, not directly related to any of the filter types:
we are calling [replacePartialElements](https://github.com/getodk/collect/blob/master/entities/src/main/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategy.kt#L52C36-L52C58). In which scenario would we encounter issues if we do not call it? I removed this call, and all tests passed, which was strange to me. I believe I understood its purpose, but now that all tests are passing without it, I feel a bit disoriented.

Everything will "work" without replacePartialElements: the FilterStrategy chain will return the correct references and then later those will get materialized into elements via the secondary instance's resolveReference method. Without replacePartialElements however, the call to resolveReference will end up forcing a full parse of the secondary instance as the reference will point at a partial element. replacePartialElements essentially fills in the gaps in the partially parsed instance (from the representation) as we make filters so we only load the things into memory that we need. There are specific tests that check that we don't "fallback" like this (example here).

@seadowg, feel free to not only answer these questions but also take over if you don't have other pressing tasks. You might have intended to work on this yourself, and I took it on without asking. If that's the case, I hope at least what I’ve done will be helpful.

No I think it's actually a good idea to swap over on this one as I'm worried I'm a silo on how the entities stuff all works (especially the optimizations). It'll be good to spread the knowledge out a bit!

@seadowg
I think I need to rethink the approach we started with, so I wanted to discuss it with you.

If you take a look at the first couple of commits, you'll see that I added support for label = <value> and id != <value> by continuing with our initial approach. Specifically, I updated the queryEq method and implemented a new queryNotEq

While this worked well initially, I found that adding new methods like getBySomething for every case ended up generating a lot of code. The real challenge came when I started thinking about how to handle combinations of = and != with AND/OR. I realized that our current approach would make it extremely difficult, if not impossible, to support versatile expressions involving multiple AND/OR conditions.

I believe the best solution to address this, while also reducing the amount of code, would be to generate SQLite selection and selection arguments directly in LocalEntitiesFilterStrategy.
This approach offers several advantages:

• It significantly reduces boilerplate code.
• It automatically optimizes other expressions like version = <value> and version != <value>, eliminating the need to add support for each case individually.
• It simplifies handling combinations of expressions using AND/OR.

Please take a quick look to make sure you agree with it before I continue.

While this worked well initially, I found that adding new methods like getBySomething for every case ended up generating a lot of code. The real challenge came when I started thinking about how to handle combinations of = and != with AND/OR. I realized that our current approach would make it extremely difficult, if not impossible, to support versatile expressions involving multiple AND/OR conditions.

I do think you're right that we're going to need something more versatile to support all the kinds of expressions we want to optimize, and I think you're going in the right direction. It is probably the right time to consider generalizing so we don't end up with loads of methods on EntitiesRepository we replace later (as you say). However, I'd like to avoid passing SQL through as much as possible so any actual string query construction is kept to one place (DatabaseEntitiesRepository) and so that the API is still pretty rigid so we could go further. I think we're probably aiming to build our own fairly limited query language here to expand on what you've already got (which is going to be interesting). I'm just going to think out loud about how that could work here:

entitiesRepository.query(Eq("age", "34"))
entitiesRepository.query(NotEq("age", "52"))
entitiesRepository.query(And(Eq("age", "34"), NotEq("name", "Callum"))

I guess that would require a type hierarchy like:

sealed interface Query {
    class Eq(val column: String, val value: String): Query
    class NotEq(val column: String, val value: String): Query
    class And(val query1: Query, val query2: Query): Query
    class Or(val query1: Query, val query2: Query): Query
}

This would mean we'd support any length/depth of and or or XPath expressions though (and that's not something we need right now), so we could simplify it initially:

sealed interface Query {
    class And(val comparison1: Comparison, val comparison2: Comparison): Query
    class Or(val comparison1: Comparison, val comparison2: Comparison): Query

    sealed interface Comparison : Query {
        class Eq(val column: String, val value: String): Comparison
        class NotEq(val column: String, val value: String): Comparison
    }
}

What do you think?

What do you think?

It looks nice but I'm not sure if generating expressions with multiple AND/OR would be easy. I will try to add support for it with the current approach and then introduce such an interface at the end of the process.

It looks nice but I'm not sure if generating expressions with multiple AND/OR would be easy.

Do you mean generating Query or creating the SQL from it wouldn't be easy?

I will try to add support for it with the current approach and then introduce such an interface at the end of the process.

That's fine, but I really do think we should make sure we end up with something structured rather than passing string SQL queries to the repository (but it doesn't have to be this type based tree representation approach - that's just one possibility). I'd be tempted to start with just replacing with the current getByX methods with something general and then build on top of that to add the new expressions. Remember we don't need to deal with mixed or nested and/or expressions at the moment as well.