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

Limit filter option for querying Collection #334

Closed
ryankauk opened this issue Feb 17, 2020 · 2 comments · Fixed by #1502
Closed

Limit filter option for querying Collection #334

ryankauk opened this issue Feb 17, 2020 · 2 comments · Fixed by #1502
Assignees
Labels
enhancement New feature or request

Comments

@ryankauk
Copy link

ryankauk commented Feb 17, 2020

Is your feature request related to a problem? Please describe.
I'm worried about relations that could become large with the collection.init() function using postgres. Looked through the code and it seems like a relatively simple change. I don't mind helping out with this if you'd like as this is something I would like to have and really like this project.

qb.select('*').populate(populate).where(where).orderBy(orderBy);

could be change to:

qb.select('*').populate(populate).limit(limit).offset(offset).where(where).orderBy(orderBy);

Describe the solution you'd like
drivers/DatabaseDriver.ts

interface Filter<T>{
    where?: FilterQuery<T>;
    orderBy?: QueryOrderMap; 
    fields?: string[];
    limit?: number;
    offset?: number,
}

loadFromPivotTable<T extends AnyEntity<T>>(prop: EntityProperty, owners: Primary<T>[], filter: Filter<T>, ctx?: Transaction): Promise<Dictionary<T[]>>;

This should change the DatabaseDriver functions to all except Filter, this could make the function arguments more dynamic and extendible for custom configuration. Which would cause a breaking change unfortunately, unless you had variants for the functions inputs until the next version.

Let me know your thoughts!

@ryankauk ryankauk added the enhancement New feature or request label Feb 17, 2020
@B4nan
Copy link
Member

B4nan commented Feb 17, 2020

I am not against this, but the problem is that semi-initialized collection can't be used for anything more than reading, so we will need to introduce some read only collection wrapper to be used instead. The problem is already there, as you are able to use filtering on collections.

Are you talking just about the Collection.init() method, or you would like to incorporate this into the EM API too? Because you can populate collections too via EntityLoader, but that would be more complicated.

@stephenh
Copy link

Agreed with @B4nan that this will very likely cause bugs if 1 part of your application initializes the collection with the "only load 50" option, but then some other part of your application processes some business logic (like "sum all my account transactions") with the "great it's already loaded collection, I'll skip calling init" but it's missing ~80% of the rows.

If you have too many rows to pull into memory as a Collection, then it shouldn't be a collection, b/c the collection API insinuates to the rest of your program that "this is small enough to pull into memory", and if any caller forgets to pass this filter option, you'll cause a out-of-memory type error.

Alternatively, Mikro could introduce a concept of VeryLargeCollection<Child>, with some sort of API that would make it very apparent to callers that they will never all a Child[] in-memory array of every single child this entity points to, but instead they could work in batches of 100, or maybe do bulk operations like parent.largeChildrenCollection.deleteAll() or something.

@B4nan B4nan self-assigned this Feb 18, 2021
B4nan added a commit that referenced this issue Feb 28, 2021
Collections now have a `matching` method that allows to slice parts of data from a collection.
By default, it will return the list of entities based on the query. We can use the `store`
boolean parameter to save this list into the collection items - this will mark the
collection as `readonly`, methods like `add` or `remove` will throw.

```ts
const a = await em.findOneOrFail(Author, 1);

// only loading the list of items
const books = await a.books.matching({ limit: 3, offset: 10, orderBy: { title: 'asc' } });
console.log(books); // [Book, Book, Book]
console.log(a.books.isInitialized()); // false

// storing the items in collection
const tags = await books[0].tags.matching({
  limit: 3,
  offset: 5,
  orderBy: { name: 'asc' },
  store: true,
});
console.log(books); // [BookTag, BookTag, BookTag]
console.log(a.books.isInitialized()); // true
console.log(a.books.getItems()); // [BookTag, BookTag, BookTag]
```

Closes #334
B4nan added a commit that referenced this issue Feb 28, 2021
Collections now have a `matching` method that allows to slice parts of data from a collection.
By default, it will return the list of entities based on the query. We can use the `store`
boolean parameter to save this list into the collection items - this will mark the
collection as `readonly`, methods like `add` or `remove` will throw.

```ts
const a = await em.findOneOrFail(Author, 1);

// only loading the list of items
const books = await a.books.matching({ limit: 3, offset: 10, orderBy: { title: 'asc' } });
console.log(books); // [Book, Book, Book]
console.log(a.books.isInitialized()); // false

// storing the items in collection
const tags = await books[0].tags.matching({
  limit: 3,
  offset: 5,
  orderBy: { name: 'asc' },
  store: true,
});
console.log(tags); // [BookTag, BookTag, BookTag]
console.log(books[0].tags.isInitialized()); // true
console.log(books[0].tags.getItems()); // [BookTag, BookTag, BookTag]
```

Closes #334
B4nan added a commit that referenced this issue Feb 28, 2021
…1502)

Collections now have a `matching` method that allows to slice parts of data from a collection.
By default, it will return the list of entities based on the query. We can use the `store`
boolean parameter to save this list into the collection items - this will mark the
collection as `readonly`, methods like `add` or `remove` will throw.

```ts
const a = await em.findOneOrFail(Author, 1);

// only loading the list of items
const books = await a.books.matching({ limit: 3, offset: 10, orderBy: { title: 'asc' } });
console.log(books); // [Book, Book, Book]
console.log(a.books.isInitialized()); // false

// storing the items in collection
const tags = await books[0].tags.matching({
  limit: 3,
  offset: 5,
  orderBy: { name: 'asc' },
  store: true,
});
console.log(tags); // [BookTag, BookTag, BookTag]
console.log(books[0].tags.isInitialized()); // true
console.log(books[0].tags.getItems()); // [BookTag, BookTag, BookTag]
```

Closes #334
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants