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: change total items from int to long #5902

Open
wants to merge 11 commits into
base: next-2.0
Choose a base branch
from

Conversation

tesar-tech
Copy link
Collaborator

@tesar-tech tesar-tech commented Dec 16, 2024

Description

Closes #5170

Supporting larger datasets than int.MaxValue isn't just about changing int to long...

First, the issue with Virtualize needs to be addressed.

Breaking changes

Changing TotalItems to long also requires to change all the datatypes around it. For example for Pager. Also TotalPages are now long. Which leads to some breaking changes in user code. The biggest one I think is the inability to do:

filteredData.Skip( ( e.Page - 1 ) * e.PageSize ).Take( e.PageSize )

because the result in Skip is now long and skip doesn't support long

filteredData.Skip( (int)( e.Page - 1 ) * e.PageSize ).Take( e.PageSize ).ToList();

↑ Solution is shortsighted, because it will overflow "soon". That's why I include all such changes in one commit (we can revert them easily)..

Also all the custom pagers will have to be upgraded to use long instead of int. And probably many more places too.

Current PR state

Overall it works and we can support quite huge datasets. Check the DataGridLargePage.razor .

Possible solution:

From all this I think we should take another approach:

  • Internally handle everything in longs, as is the current state of the pr.

  • Expose the "public getter only properties" also with ints, in their original names (CurrentPage, LastVisiblePage, etc):

  • Rename the "working" public getters with *Long suffix.

  • Probably TotalItems should still be long.

     public int FirstVisiblePage => (int) FirstVisiblePageLong;
    
     public long FirstVisiblePageLong
        {
            get
            {
                CalculateFirstAndLastVisiblePage();
                return firstVisiblePage;
            }
        }
    
  • Vast majority of users don't need the long support. I would say - don't force it to them => less breaking changes for current users.

  • Who really wants the long support has to change to the *Long properties - no big deal.

Todos

  • Decide what to do with virtualize
  • Decide how to support longs.
  • Check other components to support for larger datasets (autcomplete, ..?)
  • Update the test to test such large datasets

@tesar-tech tesar-tech requested a review from stsrki December 16, 2024 17:53
Copy link
Collaborator

@stsrki stsrki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is quite straightforward so there is not too complex things to review. We just need to document it for the user.

And also, write migration notes and release notes.

@tesar-tech tesar-tech marked this pull request as ready for review December 17, 2024 17:37
@stsrki
Copy link
Collaborator

stsrki commented Dec 19, 2024

@tesar-tech can you please merge next-2.0 into this branch and fix conflicts?

…from-int-to-long

# Conflicts:
#	Demos/Blazorise.Demo/Pages/Tests/DataGrid/DataGridPage.razor.cs
#	Demos/Blazorise.Demo/Pages/Tests/DataGrid/PagerPage.razor
#	Documentation/Blazorise.Docs/Pages/Docs/Extensions/DataGrid/Examples/DataGridPagerExample.razor
#	Source/Extensions/Blazorise.DataGrid/Contexts/PaginationContext.cs
#	Source/Extensions/Blazorise.DataGrid/DataGrid.razor.cs
#	Source/Extensions/Blazorise.DataGrid/DataGridState.cs
#	Source/Extensions/Blazorise.DataGrid/EventArguments/DataGridPageChangedEventArgs.cs
#	Source/Extensions/Blazorise.DataGrid/_DataGridPagination.razor
@tesar-tech
Copy link
Collaborator Author

Now with the ExpressionCompiler there is the long to int conversion issue again. I hid it for now, but it's suboptimal solution.

It boils down to Queryable.Skip method that cannot accept long.. Previously the need for conversion stopped users and made them at least think about it by implementing the conversion. But now, with ApplyDataGridPaging<TItem>( this IQueryable<TItem> data, DataGridReadDataEventArgs<TItem> dataGridReadDataEventArgs ) where the long/int attribute is "hidden" in dataGridReadDataEventArgs, nothing is stopping users from shooting their leg.

Warning in case of larger datasets? Some Skip implementation under the hood? Removing the "problematic" overload? Currently there are just comments.

@stsrki
Copy link
Collaborator

stsrki commented Dec 24, 2024

Now with the ExpressionCompiler there is the long to int conversion issue again. I hid it for now, but it's suboptimal solution.

It boils down to Queryable.Skip method that cannot accept long.. Previously the need for conversion stopped users and made them at least think about it by implementing the conversion. But now, with ApplyDataGridPaging<TItem>( this IQueryable<TItem> data, DataGridReadDataEventArgs<TItem> dataGridReadDataEventArgs ) where the long/int attribute is "hidden" in dataGridReadDataEventArgs, nothing is stopping users from shooting their leg.

Warning in case of larger datasets? Some Skip implementation under the hood? Removing the "problematic" overload? Currently there are just comments.

@David-Moreira

@David-Moreira
Copy link
Contributor

Now with the ExpressionCompiler there is the long to int conversion issue again. I hid it for now, but it's suboptimal solution.
It boils down to Queryable.Skip method that cannot accept long.. Previously the need for conversion stopped users and made them at least think about it by implementing the conversion. But now, with ApplyDataGridPaging<TItem>( this IQueryable<TItem> data, DataGridReadDataEventArgs<TItem> dataGridReadDataEventArgs ) where the long/int attribute is "hidden" in dataGridReadDataEventArgs, nothing is stopping users from shooting their leg.
Warning in case of larger datasets? Some Skip implementation under the hood? Removing the "problematic" overload? Currently there are just comments.

@David-Moreira

I'd just personally make everything long. Realistically the price you pay is negligible and makes the api handle a wider number of cases.
Having to support both int and long in some way just seems like a big mess 😅

We could always take some inspiration from other libraries also.

@tesar-tech
Copy link
Collaborator Author

I'd just personally make everything long. Realistically the price you pay is negligible and makes the api handle a wider number of cases. Having to support both int and long in some way just seems like a big mess 😅

We could always take some inspiration from other libraries also.

That is already decided, but it has some issues with changes introduced in your recent PR.

@David-Moreira
Copy link
Contributor

David-Moreira commented Dec 27, 2024

where the long/int attribute is "hidden" in dataGridReadDataEventArgs, nothing is stopping users from shooting their leg.

Can you clarify? Isn't the attribute there an int and can't we also change it to long? Maybe I'm not getting it.

@stsrki
Copy link
Collaborator

stsrki commented Dec 27, 2024

where the long/int attribute is "hidden" in dataGridReadDataEventArgs, nothing is stopping users from shooting their leg.

Can you clarify? Isn't the attribute there an int and can't we also change it to long? Maybe I'm not getting it.

I believe the problem is that in Linq, some of the methods cannot receive the long type. But only int. For example Skip method. So our new expression builder has the same limitation.

@stsrki
Copy link
Collaborator

stsrki commented Dec 27, 2024

I have asked ChatGPT is it possible to implement custom extension methods and translate it into a valid SQL. Note that I haven't tried this in the code.

Step 1

public static class QueryableExtensions
{
    public static IQueryable<T> SkipLong<T>(this IQueryable<T> source, long count)
    {
        if (source == null)
            throw new ArgumentNullException(nameof(source));

        if (count > int.MaxValue)
            throw new ArgumentOutOfRangeException(nameof(count), "Count exceeds int.MaxValue, which is not supported by LINQ.");

        return source.Skip((int)count);
    }
}

Step 2

public static class SqlServerDbFunctionsExtensions
{
    public static IQueryable<T> SkipLong<T>(this IQueryable<T> source, long count)
    {
        throw new NotSupportedException("This method is for use with EF Core LINQ queries only and has no in-memory implementation.");
    }
}

Step 3

public class CustomSkipLongTranslator : IMethodCallTranslator
{
    private static readonly MethodInfo SkipLongMethod =
        typeof(SqlServerDbFunctionsExtensions).GetMethod(nameof(SqlServerDbFunctionsExtensions.SkipLong), new[] { typeof(IQueryable<>), typeof(long) });

    public Expression Translate(MethodCallExpression methodCallExpression)
    {
        if (methodCallExpression.Method == SkipLongMethod)
        {
            var source = methodCallExpression.Arguments[0];
            var count = methodCallExpression.Arguments[1];
            return Expression.Call(
                typeof(Queryable).GetMethods()
                    .First(m => m.Name == "Skip" && m.GetParameters().Length == 2)
                    .MakeGenericMethod(methodCallExpression.Method.GetGenericArguments()),
                source,
                Expression.Convert(count, typeof(int)));
        }

        return null;
    }
}

Step 4

public class CustomSkipLongTranslator : IMethodCallTranslator
{
    private static readonly MethodInfo SkipLongMethod =
        typeof(SqlServerDbFunctionsExtensions).GetMethod(nameof(SqlServerDbFunctionsExtensions.SkipLong), new[] { typeof(IQueryable<>), typeof(long) });

    public Expression Translate(MethodCallExpression methodCallExpression)
    {
        if (methodCallExpression.Method == SkipLongMethod)
        {
            var source = methodCallExpression.Arguments[0];
            var count = methodCallExpression.Arguments[1];
            return Expression.Call(
                typeof(Queryable).GetMethods()
                    .First(m => m.Name == "Skip" && m.GetParameters().Length == 2)
                    .MakeGenericMethod(methodCallExpression.Method.GetGenericArguments()),
                source,
                Expression.Convert(count, typeof(int)));
        }

        return null;
    }
}

If this works there might be a way around it.


Now, I don't think we should do this in our code. We can provide our users with a guide on how to do this in their own projects. Write a blog post or documentation page. We could even create a separate project that does it for them, like Blazorise.DataGrid.EFCore, or something like that.

@tesar-tech
Copy link
Collaborator Author

Now, I don't think we should do this in our code. We can provide our users with a guide on how to do this in their own projects. Write a blog post or documentation page. We could even create a separate project that does it for them, like Blazorise.DataGrid.EFCore, or something like that.

Yeah, I was here—trying to figure out a way to support long in Skip.

This whole long addition feels off to me. It’s not well-supported in Blazor (Virtualize), LINQ, or even in SQL. With this PR, we’re giving the impression that it’s fully solved on our end, but in reality, it makes things worse.

  • For the majority of users (99%, I’d guess) who will never need to use long, we’re forcing them to adapt their surrounding logic to accommodate long. This change offers them no benefit—in fact, it often introduces unsafe int conversions.
  • For a subset of users who eventually reach numbers higher than int.MaxValue, it creates silent issues. The DataGrid gives the appearance of supporting large numbers everywhere out of the box, but it doesn’t.
  • For another small fraction of users who genuinely benefit from this change right now, it’s a precarious solution. They’ll need to carefully navigate a minefield to avoid the silent issues.

Adding this information to the documentation is an option, but it feels like a way to sidestep the real problem rather than addressing it properly.

...

  • Avoid troubling users with below int.MaxValue item. The majority of users should not be burdened with this change.
  • Require an explicit opt-in for long support. This ensures users are aware of the potential pitfalls and can’t accidentally run into silent issues.

One potential solution could be introducing a separate component, like DataGridLong (or something similar). This component wouldn’t support "int stuff" by default, avoiding scenarios where users inadvertently cause issues. It would require some restructuring, as most of the logic would be reused, but the "int-specific" features would remain in only the standard DataGrid where they don't cause any troubles...

@stsrki
Copy link
Collaborator

stsrki commented Dec 31, 2024

The way I see it. Long-to-int internal conversion should not be a problem. Nobody will load over 2 billion of data in memory to show it in the data grid. So doing the .Skip((int)longValue) is OK.

Only once they use our ReadData API they are going to need to implement their own ways of reading outside data. And in this moment the long type that we are trying to support will be of great help.

@tesar-tech
Copy link
Collaborator Author

The way I see it. Long-to-int internal conversion should not be a problem. Nobody will load over 2 billion of data in memory to show it in the data grid. So doing the .Skip((int)longValue) is OK.

But the Skip is done on IQueryable. And the ExpressionCompiller is meant to be used in ReadData as shown inside the demo. The whole datasest isn't loaded to memory.

The original dataset inside the demo is a List, but that's for demo purposes. It can easily be IQueryable from the very start.
So it will work when the dataset is smaller than int.MaxValue and silently cause bugs when the dataset will be larger...

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

Successfully merging this pull request may close these issues.

3 participants