mongodb / mongo-jdbc-driver

JDBC Driver for MongoDB Atlas SQL interface
Apache License 2.0
35 stars 33 forks source link

SQL-1055: Update the list of supported functions #126

Closed mattChiaravalloti closed 1 year ago

mattChiaravalloti commented 1 year ago

This PR updates the list of supported functions in the JDBC driver. The function list now includes functions that were added to MongoSQL for Tableau support. Only functions that are directly defined in the dialect -- not those that are defined via Tableau formulae -- are included, as per the ticket comment.

This PR also updates references to "ADL" to "ADF", particularly the environment variable names. This makes this repo consistent with the odbc repo. Evergreen variables have been modified accordingly.

mattChiaravalloti commented 1 year ago

Looks like there may be some issues with the new variable names. Will investigate.

Also, there are unit tests I need to update!

mattChiaravalloti commented 1 year ago

Ah, I know the issue -- I forgot you need to press "save" all the way at the bottom of the evergreen page! will update and re-run a patch to verify the changes 👍

bucaojit commented 1 year ago

There is another adl reference in gradle.properties for the artifactId.

bucaojit commented 1 year ago

@nbagnard I find it helpful to have all the function permutations, unless it is documented elsewhere. Although if other driver's don't normally handle things that way it does make sense to reduce it to one entry or fewer for example, MOD and LOG instead of 16 entries could show how they can be used with INTEGER and DOUBLE to reduce it to 4 entries?

mattChiaravalloti commented 1 year ago

There is another adl reference in gradle.properties for the artifactId.

That one I wasn't sure if I could/should change since that will start to change the name of the released artifacts, right? Is it safe/okay to make that change?

mattChiaravalloti commented 1 year ago

@nbagnard I find it helpful to have all the function permutations, unless it is documented elsewhere. Although if other driver's don't normally handle things that way it does make sense to reduce it to one entry or fewer for example, MOD and LOG instead of 16 entries could show how they can be used with INTEGER and DOUBLE to reduce it to 4 entries?

For both of you, which types would we choose for the unary functions? Should we do DECIMAL since it is the "highest" numeric type in our hierarchy? Or should we do DOUBLE since that is a more common type?

bucaojit commented 1 year ago

There is another adl reference in gradle.properties for the artifactId.

That one I wasn't sure if I could/should change since that will start to change the name of the released artifacts, right? Is it safe/okay to make that change?

So far I have only seen mongodb-jdbc for the artifact ID such as here. Let's keep it as is for now until we have a better idea of where it is used.

nbagnard commented 1 year ago

Thank you both for your comments. It looks like even though the list is pretty long for MOD for example, it is representing the functions we support realistically and trying to find out which type we should use for 'number' gets us into the tricky discussion of DECIMAL vs DOUBLE. Let's keep it this way then :)

nbagnard commented 1 year ago

So far I have only seen mongodb-jdbc for the artifact ID such as here. Let's keep it as is for now until we have a better idea of where it is used.

It's probably not used. Gradle uses the project name as the artifact id. The default project name is the project’s directory name, or it can be defined in settings.gradle (this is where you'll see rootProject.name = 'mongodb-jdbc'). And we don't override the artifactid in publish.gradle.

mattChiaravalloti commented 1 year ago

Thank you both for your comments. It looks like even though the list is pretty long for MOD for example, it is representing the functions we support realistically and trying to find out which type we should use for 'number' gets us into the tricky discussion of DECIMAL vs DOUBLE. Let's keep it this way then :)

Left it as-is!

mattChiaravalloti commented 1 year ago

evergreen merge