JetBrains/xodus-dnq

Suboptimal breaking change from PR #26

jk1 opened this issue · 9 comments

jk1 commented

I was quite surprised to see entityIterable with nulls:

interface XdQuery<out T : XdEntity> {
    val entityType: XdEntityType<T>
    val entityIterable: Iterable<Entity?>
}

To see where the problem is let's consider the simplest use case

XdProject.all().entityIterable  // Iterable<Entity?>

What does it mean? Why are there nulls among my projects? It just doesn't make sense.
Where Iterable<Entity?> does make sense is in map/distinctMap, and even there nullability should be inferred from a closure return type.

To make the matters worse naive developer is likely to use kotlin collection API to get rid of nulls:

XdUser.all().entityIterable.filterNotNull() // whoops, 200 000 users are now in memory
sedovalx commented

The problem is here

fun <S : XdEntity, T : XdEntity> XdQuery<S>.mapDistinct(dbFieldName: String, targetEntityType: XdEntityType<T>): XdQuery<T> {
    return queryEngine.selectDistinct(entityIterable, dbFieldName).asQuery(targetEntityType)
}

The QueryEngine here can return null elements in the resulting iterable. There is no way to predict if the result will contain null or not from the parameters of the function.

Another option would be to create nullable XdQuery<T : XdEntity, E : Entity?>, but it would be a bigger breaking change.

Probably, we should guard the XdQuery#entityIterable somehow.

lehvolk commented

Let's take a look how can we do it smarter.

lehvolk commented

Another thing here that worries me is that query.size() and query.asSequence().size may differs.

sedovalx commented

No nulls - no problems, right? We may change the mapDistinct to ignore null results at all, but I personally dislike this not obvious behavior. Though, we can make it more clear:

  • make XdQuery#entityIterable elements not nullable again
  • create a mapNotNull method that ignores null results
  • make mapDistinct to use mapNotNull
  • deprecate mapDistinct method in favor to the mapNotNull
  • remove XdQuery#asNullableSequence method

p.s.: mapDistinctNotNull/ mapNotNullDistinct?

lehvolk commented

Let's discuss nulls in EntityIterable with @penemue when he will be back. At the current moment seems that nulls may appear only on mapDistinct, flatMapDistinct operations.

penemue commented

Nulls can be among the results of SelectDistinct or SelectManyDistinct.

lehvolk commented

@penemue am I right that result of querying EntityIterable with nulls will be EntityIterable without nulls?

mazine commented

OK guys. After all discussions I think that we don't need Iterable<Entity?> at all. Yes, it means that it will be impossible to have nulls returned by mapDistinct. I don't feel that we actually need it.

mazine commented

I'm rolling back all changes related to Iterable<Entity?>. I will add ExcludeNullStaticTypedEntityIterable to mapDistinct / flatMapDistinct.