Need clarification for return value of did.database/run_sql_query

[ehennestad]

Last year, I made some changes to the run_sql_query method in did.database, commenting out a statement using struct2table. I think this was to improve performance, because struct2table has some overhead, and it seemed unnecessary to go via a table to extract the values to return.

https://github.com/VH-Lab/DID-matlab/blame/4581d4257776db2ffbc79629c1e29b91280f0fd2/code/%2Bdid/database.m#L740-L762

In the code below, I commented out line 748 & 751 and added 752:

DID-matlab/code/+did/database.m

Lines 740 to 762 in 4581d42

	data = do_run_sql_query(sqlitedb_obj, query_str);
	% Post-process the returned data
	if isempty(data), data = {}; return, end
	returnStruct = nargin > 2 && returnStruct; % default=false
	if ~returnStruct && isstruct(data)
	fn = fieldnames(data);
	numFields = numel(fn);
	%dataTable = struct2table(data,'AsArray',true);
	dataCells = {};
	for i = numFields : -1 : 1
	%results = dataTable.(fn{i});
	results = {data.(fn{i})};
	if isempty(results)
	results = {}; % ensure it's a cell-array
	elseif ~iscell(results) && (isscalar(results) || ischar(results))
	results = {results};
	end
	dataCells{i} = results;
	end
	%if numFields == 1, dataCells = dataCells{1}; end %de-cell
	data = dataCells;
	end

However, the current implementation will not return the result in the same data configuration as before:

Example:

>> data = struct('number', {1,2,3})

data = 

  1×3 struct array with fields:

    number

Original return value (after post processing the response):

data =

  1×1 cell array

    {3×1 double}

Current return value:

data =

  1×1 cell array

    {1×3 cell}

This is the current description of the expected return value:

            %    returnStruct - true to return a struct (or struct array) of values.
            %                   false (default) to return an array of values.
            %                     If multiple fields are returned by the query,
            %                     they are enclused within a containing cell-array.

According to this definition the original value is (almost) correct, and my change introduced a bug which needs to be fixed.

As part of the fix, I propose to refactor the code and extract the code for post processing the response into a separate function to make it easier to write unit tests to test different variations of inputs, i.e. scalar and non-scalar structures with one or more fields, as well as fields containing different data types.

At some point, line 760 was also commented out, but that seems to contradict the description of what the returned value should look like (i.e not a cell array if the structure only has one field).

[stevevanhooser]

I like the idea of refactoring the code into separate functions. The methods of the class are very long.

I have a memory of needing to reintroduce some of the old code because something was broken in situations where the structures were highly nested (a.b.c), but I can't seem to find it so maybe I didn't change it here.