Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Model support for TreeItemSelector control source data #2023
base: develop
Are you sure you want to change the base?
Add Model support for TreeItemSelector control source data #2023
Changes from 12 commits
cb6fb64
e160b21
d0742d6
b42a49c
4fe23b7
06a821e
416abe6
09bc291
fb7a1a8
17306b9
faf1f8a
590bbc6
63d04a6
7024eb2
d54c7ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implies a DB query (Model::getIterator() which implies SELECT). Recursively.
Single query recursive data fetch can be hard to implement and will require RECURSIVE CTE.
A good enough compromise might be to fetch +1 tree level collectively at once instead of 1 leaf at each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvorisek Recursive CTE is an overkill for such a simple task. Or shall I import the whole tree definition table into an array, and do the recursive operation inside the array? That would speed it up as well, and we talk about reasonably sized trees typically (otherwise we would need another treeview component which dynamically loads sub-trees upon click, there are only few Vue components doing this beautifully). Materializing the whole table into an array, I did before to speed up the script, but you did not like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It it obvious both extremes - current dummy impl. and whole table materialization is not a solution. The later case can produce OOM error easily, imagine the perf. on table with millions of records. It would be good to impl. recursive iterator in atk4/data Model and use it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your worries are not realistic, as models for this TreeItemSelector are never tables of million entries. And if they are TreeItem is the wrong control as written before: you would need a Tree Control which dynamically loads sub-trees on clicks. As said, Recursive CTE is an overkill for this simple control, unless you see yourself in a position to implement this soon. Even with recursive CTE, the control would not load properly in time with your million-entries example. Otherwise I suggest we go for the faster export variant. Working well in real life with a few hundreds entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Michael, let's focus on the solution and please reread my posts above. I did not written CTE is needed, I have written the current demo is awfully slow, which is a fact and I hope we both understand the PR cannot be merged as is.
I can also tell that table with millions of records is a realistic usecase, not for display, but imagine any tree features per some item like product. No product will have a lot of features, but the whole table can be huge. And as long as you cannot resolve the "minimal spanning tree" in one query, you cannot fetch the data using a single query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this cond?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we have to set a new parentId condition on an unconditioned model, otherwise will have contradictory conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why to clone here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we have to set a new parentId condition on an unconditioned model, otherwise will have contradictory conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct myself - this clone is indeed not needed, as we clone earlier into
nodeModel
already. Correct in latest commit