tomasfabian / ksqlDB.RestApi.Client-DotNet

ksqlDb.RestApi.Client is a C# LINQ-enabled client API for issuing and consuming ksqlDB push and pull queries and executing statements.
MIT License
97 stars 26 forks source link

DECIMAL is specified without precision and scale, causing an syntax error. #66

Closed mrt181 closed 7 months ago

mrt181 commented 8 months ago

Describe the bug When a field/property has type decimal the statementgenerator maps the type to DECIMAL instead of DECIMAL(precision,scale) where precision and scale must be some integer number.

To Reproduce see description

Expected behavior Precision and scale are set on a DECIMAL type. Precision and scale need to be configurable.

Environment (please complete the following information):

mrt181 commented 8 months ago

I would like to provide a PR

mrt181 commented 8 months ago

Cool, its already fixed in main using the DecimalAttribute. Can you provide a new release candidate

mrt181 commented 8 months ago

would it make sense to use a documented default value in case the DecimalAttribute is missing?

      if (type == typeof(decimal))
      {
        var decimalMember = memberInfo.TryGetAttribute<DecimalAttribute>();

        if (decimalMember != null)
        {
          return $"({decimalMember.Precision},{decimalMember.Scale})";
        }
        else
        {
          return $"(14,14)";
        }
      }
tomasfabian commented 8 months ago

Hi @mrt181,
how did you come up with this default value $"(14,14)"? Are these just some arbitrary numbers? Otherwise, I agree that it makes sense to provide a documented default precision and scale for the DECIMAL ksqldb type. For example, upcasting a BIGINT to a decimal results in a decimal value with a precision of 19 and a scale of 0. I'm looking forward to your next PR. :) It could be documented here for your reference, in case you haven't come across it yet.

tomasfabian commented 8 months ago

Initially, I was hesitant to decide on a default precision and scale for the DECIMAL type. I'm considering whether it's better to throw a .NET exception when the DecimalAttribute is missing instead of setting a default. What do you think?

mrt181 commented 8 months ago

decimal in dotnet has 28 significat digits. I defaulted to 14 for precision and 14 for scale.

The released versiond fail with BadRequest because they do not set precision and scale. I would opt for default precision and scale.

I have two views.

Currently I would like to use the code as is from main that uses the DecimalAttribute and could implement the second approach as a feature later. This would cover StructAttribute, KeyAttribute and DecimalAttribute. The idea would be to set the properties that should be treated as if they have those attribites in the EntityCreationMetadata. The translator could check for the attribute on the member and the registered properties attribute tuple. This would allow for a schema first approach without the need to set teh attributes on types that are coming from code-gen.

tomasfabian commented 8 months ago

I'm growing increasingly curious. Would you recommend developing a code-generation tool and incorporating a database schema-first approach alongside the current code-first approach? From what I gather, this seems more like an epic endeavor rather than just a simple story.

mrt181 commented 8 months ago

Ksqldb works with avro, protobuf and json-schema when leveraging schema-registry. There are already tools available that do the code-gen part from those schemas. I would like to leverage those tools and when using this library to tell it which properties/types should be treated as Key, Struct etc. without the need to touch the generated code. This would allow to not commit generated code to a repo and keep the generation as part of the build process.

I know the schema of the data in a kafka-topic and I want to use ksqldb to access the data. I have access to the schema and can generate the code for that. That code doesn't/shouldn't have a dependency on ksqldb/ksqldb.restapi.

tomasfabian commented 8 months ago

Thank you for the clarification, I think that now I understand your use case. It seems you're interested in implementing a more fluent API that would allow you to configure entities (from-item types):

public class TransactionConfiguration : IFromItemTypeConfiguration<Transaction>
{
    public void Configure(FromItemTypeBuilder<Transaction> builder)
    {
        builder
            .Property(c => b.Amount)
            .Decimal(14, 14);
    }
}

The above code is of course inspired by EF Core.

The ideal scenario would involve effortlessly extending pre-existing (generated) classes/records with additional decorations.

tomasfabian commented 8 months ago

I created a branch with a model builder prototype. I'd appreciate your feedback when you have a moment.

tomasfabian commented 7 months ago

@mrt181, could you please confirm if you intend to submit a PR as previously mentioned, or should I proceed with handling the task? I'm progressing with the prototype so if you don't mind I would like to overtake this task. Thank you in advance!

tomasfabian commented 7 months ago

The requested functionality has been just released:

dotnet add package ksqlDb.RestApi.Client --version 5.0.0