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

Unable to orderby decimal column #10249

Closed
chris31389 opened this issue Nov 9, 2017 · 9 comments
Closed

Unable to orderby decimal column #10249

chris31389 opened this issue Nov 9, 2017 · 9 comments

Comments

@chris31389
Copy link

chris31389 commented Nov 9, 2017

I have a class with a decimal property. I wanted to order all instances of the class by the decimal property within the SQLite database.

I created three instances with these decimal values { 7m, 84.3m, 13.4m }. After running an OrderBy function, I expected it to order it to { 7m, 13.4m, 84.3m } but instead it ordered it to { 13.4m, 7m, 84.3m }

Steps to reproduce

Please run the test below

public class TestEntity : Entity
{
    public TestEntity(decimal @decimal) => Decimal = @decimal;

    protected TestEntity()
    {
    }

    public decimal Decimal { get; set; }
}

[TestFixture]
public class DecimalSortingTests : DbTests
{
    [Test]
    public async Task GivenClassWithDecimalColumn_WhenIOrderBy_ItOrdersCorrectly()
    {
        decimal[] decimals = { 7m, 84.3m, 13.4m };
        IEnumerable<TestEntity> testClasses = decimals.Select(@decimal => new TestEntity(@decimal));
        using (DbContext dbContext = GetDbContext())
        {
            dbContext.Set<TestEntity>().AddRange(testClasses);
            await dbContext.SaveChangesAsync();
        }

        List<TestEntity> entities;
        using (DbContext dbContext = GetDbContext())
        {
            entities = await dbContext.Set<TestEntity>()
                .OrderBy(x => x.Decimal)
                .ToListAsync();
        }

        List<TestEntity> list = entities.ToList();
        Assert.That(list.Count, Is.EqualTo(decimals.Length));
        Assert.That(list.ToArray()[0].Decimal, Is.EqualTo(7m));
        Assert.That(list.ToArray()[1].Decimal, Is.EqualTo(13.4m));
        Assert.That(list.ToArray()[2].Decimal, Is.EqualTo(84.3m));
    }

    private class TestDbContext : DbContext
    {
        public TestDbContext(DbContextOptions options) : base(options)
        {
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<TestEntity>();
            base.OnModelCreating(modelBuilder);
        }
    }

    private DbContext GetDbContext()
    {
        DbContextOptions<TestDbContext> options = new DbContextOptionsBuilder<TestDbContext>()
            .UseSqlite(SqliteConnection)
            .EnableSensitiveDataLogging()
            .Options;

        TestDbContext dbContext = new TestDbContext(options);
        dbContext.Database.EnsureCreated();
        return dbContext;
    }
}
  Expected: 7m
  But was:  13.4m

Further technical details

EF Core version: 2.0.0
Database Provider: Microsoft.EntityFrameworkCore.Sqlite
Operating system: Windows 10 64x
IDE: Visual Studio 2017

@ajcvickers
Copy link
Contributor

/cc @bricelam @divega

@smitpatel
Copy link
Contributor

SQLite stores decimal as Text, The ordering on database would sort strings rather than decimal values.

https://github.com/aspnet/EntityFrameworkCore/blob/2197ef2c0dcc06fa162b0525d66757ad52e01257/src/EFCore.Sqlite.Core/Storage/Internal/SqliteTypeMapper.cs#L58

@bricelam
Copy link
Contributor

bricelam commented Nov 9, 2017

Correct. SQLite doesn't natively support decimal values, so we persist them as TEXT. You can get more expected results by casting to REAL (maps to double) before ordering, but there may still be unexpected results due to the lossy conversion.

@chris31389
Copy link
Author

Should line 58 be replaced with the line below?

{ typeof(decimal), new DecimalTypeMapping(_realTypeName) },

@bricelam
Copy link
Contributor

bricelam commented Nov 9, 2017

No, converting from decimal to REAL is lossy; hence, we picked TEXT. Not losing data felt more important.

@bricelam
Copy link
Contributor

bricelam commented Nov 9, 2017

Would using a double work for you? Do you need the precision of decimal?

@bricelam
Copy link
Contributor

bricelam commented Nov 9, 2017

My proposal was to use .OrderBy(x => (double)x.Decimal) Then you'll get mostly correct ordering while keeping decimal-precise values in the database.

@chris31389
Copy link
Author

Would that result in casting in the underlying query? I'm working with a money value and heard that decimal was better to use for money values than a double?

@ajcvickers
Copy link
Contributor

@chris31389 SQLite can't store decimal values natively. This means that, in the general case, using SQLite to store things like monetary values will always have some limitations. As @bricelam said, usually storing the exact values (non-lossy) is more important than preserving store-side sort order. It's probably safe to do the conversion to doubles in the query, as shown above, because even if this is lossy, it is unlikely to impact sort order. If it will, then the only option is to bring the data back to the client and sort it there.

I have filed #10265 for us to track future work in this area. For now, closing this as by-design.

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

4 participants