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

DataTable: add reflection based LazyDataModel, similar to JpaLazyDataModel #8395

Closed
tandraschko opened this issue Feb 4, 2022 · 26 comments
Closed
Assignees
Milestone

Comments

@tandraschko
Copy link
Member

tandraschko commented Feb 4, 2022

Currently we have 2 possible solutions (and maybe more?) to use a DataTable:

  1. value binding to a List<MyPojo>
  2. value binding to a LazyDataModel

in 1) sorting and filtering is done via our SortFeature+FilterFeature, whereas in 2) its done in the LazyDataModel itself

There are some benefits with a LazyDataModel, compared to the old/non-lazy way:

  • the model handels filter/sort like in LazyDataModel (1 place to filter/sort, doesnt matter if lazy or not)
  • it allows to apply filters via lambda
  • it "forces" to uses a lamba to get the list of data, which will be filtered afterwards. This could to lead to better practice, where the user doesnt store the list of data in a ViewScoped bean
  • its a performance boost as done via reflection instead of restoring states, rowindex, loop components, create/apply EL

What we need to discuss in future is:
Should we force people to use a LazyDataModel when filtering and sorting is activated?
Using Jpa or ReflectionLazyDataModel definitily leads to a better performance (anyonse interessted in creating a small benchamark?) and big lists will be avoided in ViewState.

@tandraschko
Copy link
Member Author

tandraschko commented Feb 4, 2022

WDYT about @Rapster? should we provide something or even replace our current Sort/FilterFeature with something like this? DataTable must wrap a value-binding with a List<MyPojo> with this new model of course.

In theory the code MUST be faster and cleaner with this. We just need a good place to cache the PropertyDescirptors.

@tandraschko
Copy link
Member Author

cc @christophs78 @melloware

@melloware
Copy link
Member

I like where you are going with it!

@Rapster
Copy link
Member

Rapster commented Feb 4, 2022

WDYT about @Rapster? should we provide something or even replace our current Sort/FilterFeature with something like this? DataTable must wrap a value-binding with a List with this new model of course

I think it definitely worth to try and see how far you can go with this 😉 Please make a PR so we can follow your progress

@christophs78
Copy link
Contributor

Sound´s interresting.
This would make filteredValue obsolet?

Thinking about implications of this:

  • no more issues issues because filteredValue is not updated correct (good)
  • people currently using filteredValue will need to rework (neutral, probably minor usage)
  • we´ll aply filter/sort on each pagination-event (negativ performance-impact for datasources with a larger amount of datasets?)

@jepsar
Copy link
Member

jepsar commented Feb 5, 2022

I use a property for a global filter (search query string) in my lazy model, as well as a two maps of properties of columns of which I need a sum or a count. Support in the lazy model for a certain operation on certain columns would be a nice addition (maybe needs a different ticket).

@tandraschko
Copy link
Member Author

@christophs78 no, that would not change filteredValue behavior at all but we have to rework it in general
currently filteredValue is only used when !lazy - and its currently also optional, its just a store, where the user can receive the filtered list, which will also be used to restore it; maybe we should use it write-only anyway
and it should also work for lazy

@tandraschko
Copy link
Member Author

so in best case, we filter once per request, for each request the datatable must be rendered
and this will be faster with this model-based-approach
the current filter and sort-feature always loops the DataTable, do some state changes like setRowIndex, evaluate EL expression and so on

@christophs78
Copy link
Contributor

@tandraschko - i agree to you.
we just should avoid a 10.0.0 like mess. but we are in a better position this time.
i also agree to @Rapster this should be done via PR.

@Rapster
Copy link
Member

Rapster commented Feb 5, 2022

PropertyUtilsBean and DefaultResolver from beanutils could be a good source of inspiration for handling propertydescriptors.

@fcorneli
Copy link

fcorneli commented Feb 9, 2022

Why not go all the way with the reflection? :)
https://github.com/e-Contract/jsf-crud

@tandraschko
Copy link
Member Author

tandraschko commented Feb 9, 2022

update the code now
the PD cache should propably go into PrimeApplication
i wont invest time for now to try to replace the SortFeature/FilterFeature but i think its a good idea for the future

@djmj

This comment was marked as off-topic.

@tandraschko
Copy link
Member Author

Actually i would like to contribute this type of LazyDataModel to PF and also write a example + doc
i just wonder, what is a perfect example for it and if we should support it?

i mean, the benefits are clear:

  • the model handels filter/sort like in LazyDataModel
  • it allows to apply filters via lambda
  • it "forces" to uses a lamba to get the list of data, which will be filtered afterwards. This could to lead to better practice, where the user doesnt store the list of data in a ViewScoped bean
  • its a performance boost as done via reflection instead of restoring states, rowindex, loop components, create/apply EL

but we provide another way of using the DataTable
maybe its to much if we dont plan to remove FilterFeature/SortFeature in future

@melloware
Copy link
Member

I like it you could call it ReflectionLazyDataModel and add it ?

Can you add it without disturbing the Sort/FilterFeature.

@christophs78
Copy link
Contributor

Needs to be done one a feature-branch + PR.
Curious to see whether it works out.

@tandraschko
Copy link
Member Author

tandraschko commented Sep 28, 2023

Commited the ReflectionDataModel now as we use it in some projects already, also added some tests
will provide a small docu enhancement tomorrow

@tandraschko tandraschko changed the title DataTable: move sort + filter to a DataModel / Introduce LazyDataModel based on object list and reflection`? DataTable: add reflection based LazyDataModel, similar to JpaLazyDataModel Sep 28, 2023
@tandraschko tandraschko self-assigned this Sep 28, 2023
@tandraschko tandraschko added this to the 14.0.0 milestone Sep 28, 2023
tandraschko added a commit that referenced this issue Sep 28, 2023
tandraschko added a commit that referenced this issue Sep 28, 2023
@Rapster
Copy link
Member

Rapster commented Sep 28, 2023

I'd like to review this but I need to know if you're done with it? If I remember well, the whole idea is to replace SortFeature & FilterFeature with the ReflectionDataModel, so I suppose what you already committed is an initial step to this whole idea or i'm missing something? Will people want to use this new DataModel? The way I understood it, this new model was meant to be used internally in PF for practical reasons

@tandraschko
Copy link
Member Author

tandraschko commented Sep 29, 2023

yep, i'm roughly done with it
Currently it's just the brother of JpaLazyDataModel. Our usecases are webservices, which doesnt support filtering or sorting. So we need to call the webservice, filter/sort via reflection. Also some features like skip filtering of some items or enhance filtering/sorting.

We dont need to replace anything, it's just an idea for the future and has some benefits.

@tandraschko
Copy link
Member Author

tandraschko commented Sep 30, 2023

JFYI, simple case normal list vs ReflectionLazyDataModel sorting 10000 entries:

  1. simple list: ~320ms response time
  2. reflectionLazyDataModel: ~230ms response time

not used a profiler and using an extreme case but it seems saving not only a few CPU

@melloware
Copy link
Member

This sounds good to me!

@tandraschko
Copy link
Member Author

added docs; closing it for now

tandraschko added a commit that referenced this issue Oct 4, 2023
@tandraschko
Copy link
Member Author

even reworked the getting started a bit and added new data binding section

tandraschko added a commit that referenced this issue Oct 4, 2023
@tandraschko
Copy link
Member Author

tandraschko commented Oct 4, 2023

@melloware @Rapster please review

i think in general we miss such hints or best practices for many components

@melloware
Copy link
Member

LGTM. And you are probably right about this could be done in a lot more places.

@Rapster
Copy link
Member

Rapster commented Oct 4, 2023

I'm currently reviewing/refactoring/polishing the new lazy models, I'll submit a draft PR soon and would like to see it for 14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants