[teiid-issues] [JBoss JIRA] (TEIID-2876) Order of DataTypeManager type names affects order of functions in FunctionLibrary

[Steven Hawkins (JIRA)]

    [ https://issues.jboss.org/browse/TEIID-2876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12948928#comment-12948928 ] 

Steven Hawkins commented on TEIID-2876:
---------------------------------------

Yes, this does generally highlight a soft spot in the resolving logic, but I don't think we should enforce a preference for string over object.  In this particular case it should be the latter as automatically widening to string is generally undesirable outside of string functions.  I would gather that many other dbs would simply throw a resolving error in this case - especially if they lack and object/variant type.

I'll address this and remove the ambiguity.
                
> Order of DataTypeManager type names affects order of functions in FunctionLibrary
> ---------------------------------------------------------------------------------
>
>                 Key: TEIID-2876
>                 URL: https://issues.jboss.org/browse/TEIID-2876
>             Project: Teiid
>          Issue Type: Bug
>          Components: Query Engine
>    Affects Versions: 7.7, 8.4, 8.7
>            Reporter: Paul Richardson
>            Assignee: Steven Hawkins
>            Priority: Minor
>
> This may be by design but the source code does not document this to be the case and appears brittle and open to future regressions.
> Executing the unit tests in TestFunctionResolving class, 'testResolveCoalesce4', the designer version of the teiid runtime client failed with the following error:
> {code}
> java.lang.AssertionError: expected:<class java.lang.String> but was:<class java.lang.Object>
> {code}
> The unit test passes in Teiid so I needed to determine the reason for the difference. Turns out the difference was the use of a LinkedHashSet rather than a HashSet.
> Tracing this back showed the following:
> # In FunctionLibrary.determineNecessaryConversions, the functionMethods are iterated through. In the case of teiid, the String version of 'coalesce' appears before the Object version and despite both versions of the function being scored the same, the String version is preferred. In the case of designer's runtime client, the Object version appeared before the String version hence the former was chosen and the unit test failed.
> # The functionMethods originate from the FunctionTree and are added into a TreeMap which is only ordered on the function name, ie. coalesce. The typed versions of this function are added to an ArrayList which is not sorted (halfway down addFunction()).
> # The coalesce functions originate from the SystemSource class and are added by iterating over  dataTypeManager.getAllDataTypeNames(). Since the latter are in a LinkedHashSet (in teiid) then ordering is preserved and the String datatype happens to precede the Object datatype. In the designer runtime client, this was a HashSet hence the difference in ordering and failing of the test.
> The consequence of this is that  determineNecessaryConversions in the FunctionLibrary gets the correct answer for the unit test but seems rather brittle in that a small change to the DataTypeManager's order of DataTypeName additions could have an indirect and non-obvious effect on which typed version of a function is returned by the resolver.
> Suggestions:
> * Ensure that the ArrayLists of functions in the FunctionTree's functionByName map are sorted based upon a narrow to broad ordering of types, ie. Object as the most broad will always be at the end. This will ensure that the scoring in FunctionLibrary.determineNecessaryConversions() will in the event of a tie always pick the narrowest of typed function.
> * Change the scoring in determineNecessaryConversions() to account for 'equal to' as well as 'less than' since String and Object functions get the same score.
> * Document that the ordering of DataTypeManager data type names should not be changed without consideration of this scenario.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira