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

TrinoFunction - new unit tests and some fixes for bugs using certain parameter types #24

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

smcl
Copy link

@smcl smcl commented Apr 3, 2025

I wanted to do a little more than just ask questions (here and here) so I looked around the codebase to see if I could contribute anything. I noticed the TrinoFunction class was missing unit tests and drafted some.

In doing so I've observed that there are a handful of bugs:

  1. the following integral types are quoted when passed into a function (i.e. catalog.myfunction("1", "2.0") rather than catalog.myfunction(1, 2.0)):
    • sbyte
    • byte
    • short
    • ushort
    • uint
    • ulong
    • System.IntPtr / nint
    • System.UIntPtr / uint
  2. similarly decimal types are quoted when they shouldn't be
  3. bool parameters are quoted as "true/false" rather than as 1/0 'bit' types (I am less certain of this one)
  4. date & time parameters use a possibly ambiguous representation rather than ISO 8601 (Again I am not 100% on this one)

This PR contains the following changes:

  1. a new test class TrinoFunctionTests, many of which (four out of nine) failed with the existing implementation of TrinoFunction
  2. an updated version of TrinoFunction that causes the tests to pass. I have adjusted it slightly because the existing implementation was slightly non-idiomatic - now using foreach (var parameter in Parameters) rather than for (int i = 0; i < Parameters.Count; i++) for example.

Before you begin think about merging this could someone please just double check that my assumptions below are correct:

  1. that bool typed params should be 1/0 rather than "true"/"false"
  2. that the format of the Date, DateTime and DateTimeOffset params are correct (that they should be ISO 8601 format)
  3. that we'd rather have a slightly funky test setup where I subclass TrinoFunction in order to access the protected BuildFunctionStatement() method rather than:
    • making that function public (very bad practice)
    • introducing a IRecordExecutorFactory interface, injecting that on instantiation and testing also TrinoFunction.Execute (feels a bit unnecessarily "AbstractEnterpriseSingletonProxyFactory")
    • abstract the function statement builder into a class (slight overengineering)
  4. that my changes to TrinoFunction are generally welcome - as I said, I changed things around quite a bit!

Copy link

cla-bot bot commented Apr 3, 2025

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Sean McLemon | LOGEX.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@smcl
Copy link
Author

smcl commented Apr 3, 2025

I've added the email address these changes were made under to my GitHub account.

@mosabua mosabua requested a review from georgewfisher April 4, 2025 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant