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

[datagrid] Allow iterator or iterable as rows #10969

Closed
daniatic opened this issue Nov 9, 2023 · 11 comments
Closed

[datagrid] Allow iterator or iterable as rows #10969

daniatic opened this issue Nov 9, 2023 · 11 comments
Labels
component: data grid This is the name of the generic UI component, not the React module!

Comments

@daniatic
Copy link

daniatic commented Nov 9, 2023

Summary 💡

I'd like to provide an iterator/iterable as rows instead of an array of objects.

Examples 🌈

Following data is served from the server:

{
    columns: [{index: 0, name: "Name1", dataType: "boolean"}, {index: 1, name: "Name2", dataType: "date"}],
    data: [
        [true, "2007-08-31T16:47+00:00"],
        [false, "2007-08-31T16:47+00:00"],
        [false, "2007-08-31T16:47+00:00"],
        [false, "2007-08-31T16:47+00:00"],
        [false, "2007-08-31T16:47+00:00"],
        ....
    ]
}

Just send whats needed! Why send redundant data over the line?

With an iterator/iterable I could provide a mapping function that maps each row on interation.

Motivation 🔦

Data is not always served in the required scheme. Therefore the data has to be iterated expensively in order to map it into the required schema. Only to be iterated again for display.

Search keywords: datagrid rows

@daniatic daniatic added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 9, 2023
@daniatic daniatic changed the title Allow iterator or iterable as rows data source Allow iterator or iterable as rows for data grid Nov 9, 2023
@romgrk
Copy link
Contributor

romgrk commented Nov 9, 2023

Can you provide an example?

@romgrk romgrk added waiting for 👍 Waiting for upvotes and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 9, 2023
@romgrk romgrk changed the title Allow iterator or iterable as rows for data grid [datagrid] Allow iterator or iterable as rows Nov 9, 2023
@romgrk romgrk added the component: data grid This is the name of the generic UI component, not the React module! label Nov 9, 2023
@daniatic
Copy link
Author

daniatic commented Nov 9, 2023

Can you provide an example?

Sure, updated the issue.

@daniatic
Copy link
Author

daniatic commented Nov 9, 2023

Another solution would just be to allow this type of scheme as data source.

@romgrk
Copy link
Contributor

romgrk commented Nov 9, 2023

I'm not favorable to this feature. Converting from iterator to array is trivially done outside of the grid. Having array rows isn't just about iteration for the grid, it's about having a way to do random access and addressing. Iterators aren't built for that purpose.

Ping @mui/xgrid if anyone else has an opinion here.

@daniatic If the conversion is expensive, it means you probably already have big costs for your data transfer, and you might benefit from looking into server-side data, pagination and/or lazy-loading.

@romgrk
Copy link
Contributor

romgrk commented Nov 9, 2023

It also seems that using rows with the format any[] already works, see here: https://codesandbox.io/s/nice-brahmagupta-p8qwws?file=/src/demo.tsx

Note that in JS, array[0] and array['0'] are equivalent at the spec level. But we could add the typing number for GridColDef.field.

@romgrk romgrk added the status: waiting for author Issue with insufficient information label Nov 9, 2023
@daniatic
Copy link
Author

Of course, we use server side pagination to mitigate the data transferred. But assuming that solves the problem is too short-sighted. In cases of many client requests there would still be a ton of unneccessary redundant data sent over the wire.

Maybe redundant data is no problem (still careless in my opinion) for simple applications, but for data intensive services it is a no go. You could argue to burden that mapping onto client but this seems careless in my opinion, too.

It also seems that using rows with the format any[] already works, see here: https://codesandbox.io/s/nice-brahmagupta-p8qwws?file=/src/demo.tsx

Note that in JS, array[0] and array['0'] are equivalent at the spec level. But we could add the typing number for GridColDef.field.

Looks promising. Does this come with feature parity, compared to the "normal" way?

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Nov 10, 2023
@romgrk
Copy link
Contributor

romgrk commented Nov 10, 2023

I don't see any reason for it not to work, if you find a bug please report it.

Does that solve your issue here? I don't see how iterators could make things more efficient here. Could you provide an example of how iterators could improve things?

@romgrk romgrk added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 10, 2023
@daniatic
Copy link
Author

I haven't tried the any[] format, but I if it works it would solve our problem, for now.

Besides that I think that iterators could help in scenarios where the server-side data does not come in the needed format and you don't want to waste resources on iterating it an additional time.

Nearly every data grid I worked with uses the built-in langauage features/iterators to solve these scenarios:
C# IEnumerable
Rust Iterator
Kotlin Sequence
js/python generator functions
...

In my eyes that's a very basic feature of a data grid.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Nov 13, 2023
@romgrk
Copy link
Contributor

romgrk commented Nov 13, 2023

Considering that the use-case you have is already supported by the datagrid, I don't see a different use-case that needs to be solved here. We can't use iterators directly in the grid because as I said we need random-access in O(1) which arrays provide but iterators don't.

If what you're after is more lazyness, then you can always provide all your columns with a .valueGetter instead of a .field. But dependeing on the use-case, this could cause all the operations on the grid to be slower because there is more indirection.

I'd say if you want us to do something here, provide us with a use-case where we can see clear tangible performance benefits, we'll consider it.

@romgrk romgrk added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 13, 2023
@daniatic
Copy link
Author

All right! Thanks for feedback. If our use cases changes/demands something similar in the future, I'll let you know.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Nov 14, 2023
@romgrk
Copy link
Contributor

romgrk commented Nov 15, 2023

I'll close for now, open again if you have more to add.

@romgrk romgrk closed this as completed Nov 15, 2023
@romgrk romgrk removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 15, 2023
@romgrk romgrk removed the waiting for 👍 Waiting for upvotes label Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

2 participants