Dynamic columns: Cast to double if type exists in schema

[yaelleC]

Description

Step one of: #454
To fix an escalation we received with the wrong type being used in cast.

This PR introduces a check for double in the types list of a dynamic column, if it is present we use it to cast, otherwise we default to the current behaviour: using the first type on the list.

This PR is not the perfect fix, but as explained on the issue, casting to double if it exists makes more sense than arbitrarily taking the first type in array.

Next steps will involve updating the stored schema based on the query filters to refine the type used in cast.

[github-actions]

Backend code coverage report for PR #471
No changes

[github-actions]

Frontend code coverage report for PR #471

Plugin	Main	PR	Difference
src	79.06%	79.02%	-.04%

[andresmgot]

I believe this does not represent reality. The schema will contain only one element per Name so, there won't be a repeated column["level"]["active"].

The fix should be in datasource.ts:

const recordSchemaArray = (name: string, types: AdxSchemaDefinition[], result: AdxColumnSchema[]) => {
  // If a field can have different types (e.g. long and double)
  // we select the first, assuming they are interchangeable
  const defaultCslType = types[0];
  if (types.every((t) => typeof t === 'string' && toPropertyType(t) === QueryEditorPropertyType.Number)) {
    // If all the types are numbers, the double takes precedence since it has more precission.
    const cslType = types.find((t) => typeof t === 'string' && (t === 'double' || t === 'real')) || defaultCslType;
    result.push({ Name: name, CslType: cslType as string, isDynamic: true });
  } else {
    console.warn(`schema ${name} may contain different types, assuming ${defaultCslType}`);
    if (typeof defaultCslType === 'object') {
      recordSchema(name, types[0], result);
    } else {
      result.push({ Name: name, CslType: defaultCslType, isDynamic: true });
    }
  }
};

here is where we record the schema in case the type has multiple values. We just need to remove the condition that checks if all the types are Number and simply set the type to double or real if it's part of the list.

[yaelleC]

I see, so when we get to escapeAndCastIfDynamic the selection of a single type has already been made, my bad!

I'll make the changes to datasource.ts 🙇‍♀️

[yaelleC]

I understand that if cslType is an array it will have more than one value, but I figured it would be good to catch the single value array case to avoid triggering a warning unnecessarily.

Plus it might come in handy when we make the changes to update the stored schema based on the query filters.

[andresmgot]

I believe that buildschema does not return an array if there is only one type but it doesn't hurt to have this check here.

[andresmgot]

also, just a minor styling thing but now I would order this a bit different. Something like:

    if (length > 1 && !allNumbers) { console.warn... }
    // previus else, no need to cast defaultCslType as string
    if (typeof defaultCslType === 'object') {
      recordSchema(name, types[0], result);
    } else {
      result.push({ Name: name, CslType: defaultCslType, isDynamic: true });
    }

and then you do the rest o

[andresmgot]

LGTM!

[andresmgot]

I believe that buildschema does not return an array if there is only one type but it doesn't hurt to have this check here.

[andresmgot]

also, just a minor styling thing but now I would order this a bit different. Something like:

    if (length > 1 && !allNumbers) { console.warn... }
    // previus else, no need to cast defaultCslType as string
    if (typeof defaultCslType === 'object') {
      recordSchema(name, types[0], result);
    } else {
      result.push({ Name: name, CslType: defaultCslType, isDynamic: true });
    }

and then you do the rest o