revig / revigniter

Framework for people who build websites using LiveCode
https://revigniter.com/
Apache License 2.0
28 stars 9 forks source link

_rigFetchAssoc - query results not Associative Array #7

Closed bhall2001 closed 7 years ago

bhall2001 commented 7 years ago

revigniter 1.9.5

Query results can only be referenced by column number and not column name for _rigFetchAssoc. It appears at one point both the numeric and text index of a columns were returned by _rigFetchAssoc. There is commented out code providing associative indexes. However, uncommenting this code will have adverse effects.

Being able to return an associative array directly from _rigFetchAssoc (and rigDbQuery/rigDBresults) as either an associative or numeric array is desirable in my opinion.

I am working on an enhancement and should have a pull request ready soon. My solution offers the ability to have _rigFetchAssoc return the numeric index by default as it does now OR have results be returned as an associative array using the pre-existing commented out code.

The ability to directly create an associative array from the query results of rigDbQuery makes code more readable, and reduces processing time of query results when an associative array is the desired result.

Bob

revig commented 7 years ago

@bhall2001 may be I don’t really understand what you mean, please bare with me if this is the case. Anyway, here is my 2 cents:
As _rigFetchAssoc is actually a private handler, it is not meant to be called directly. So, please keep in mind that changing this handler will affect the whole database query system.
According to the first version of your contribution I did test the code in question like:

put rigDBresult(rigDbQuery("SELECT * FROM recipe")) into tResult

put 0 into tIncr
repeat for each key tKey in tResult
    add 1 to tIncr
    put rigDbRow(tIncr) into tRow
    put tRow["Name"] & comma after tResultData
end repeat

This worked for me as expected and it shows that you already can reference columns by name.
There is even an alternative way (in case you overlooked it) to reference columns by names using less code like:

put rigDbQuery("SELECT * FROM recipe") into tQuery

repeat for each key tKey in tQuery["resultarray"]
    put tQuery["resultarray"][tKey] into tRow
    put tRow[tQuery["fieldnumbers”][“Name”]] & comma after tResultData
end repeat
bhall2001 commented 7 years ago

I did find that I had an issue that was a mistake on my part which is why the issue has changed.

I do understand how field names can be referenced with the existing structure. It is why I don't use the function and have written my own hook. I find the result format is more difficult than it needs to be and the pattern is not one that I see often in Livecode or other environments I work in. I have more experience with query results being returned referenced by field name directly and not by number with a reference list of field names.

The function _rigFetchAssoc implies by name and description that it was intended to return an associative array, which it does not. There is commented out code that would indeed return an array indexed by field names.

My intent is to have the function return either an array in the current and default format or an "associative" indexed array.

The change is 100% backwards compatible and makes the format of the query results in a format that I (and I hope others) would prefer to have as an option. Changes to the 3 functions touched are minimal (less than 20 lines of code total).

The solution adds an OPTIONAL parameter to rigDbQuery (and rigDBresult). Set this parameter to true and resultarray contains field name indexes. Omit the parameter or set to false and revigniter returns the exact data and format that exists today.

put rigDbQuery("SELECT * FROM recipe",,true) into tQuery
put tQuery["resultarray"] into tResultA

tResultA is a Livecode array with field names as the 2nd index.

put rigDbQuery("SELECT * FROM recipe") into tQuery
put tQuery["resultarray"] into tResultA

tResultA has the current/default array structure with numeric indexes.

A recipe for searching data by primary key... (of course this is a hard coded example)

put rigDbQuery("SELECT * FROM recipe WHERE recipe.id=1234",,true) into tQuery
return tQuery["resultarray"][1]

returns a true Livecode associative array indexed by field name. The pattern is familiar to other Livecode associative array functions outside of revIgniter. Having an array using field names allows the query results to be handle quite easily in Controllers and Views.

I also find that using field names directly works well when serializing data to LSON, JSON, YAML or XML when using revIgniter as an API endpoint. No additional processing is needed on the query results to directly convert the data to the desired format.

An equivalent translation of the example provided would be:

put rigDbQuery("SELECT * FROM recipe",,true) into tQuery
put tQuery["resultarray"] into tResultA

repeat for each key tKey in tResultA
    put tResultA[tKey][“Name”] & comma after tResultData
end repeat

I do not think this demonstrates the reason why I would request Name indexes be added to revIgniter though. The code in my opinion above is more readable and follows patterns more familiar with Livecode associative arrays.

I've also made changes to rigDBresult to return an array indexed with field names.

get rigDbQuery("SELECT * FROM recipe")
put rigDBresult(true) into tResultA

tResultA contains an array where the 2nd index is by field name.

Another recipe I use quite often with field name indexes is

put rigDbQuery("SELECT Name, Ingredients, Directions FROM recipe WHERE recipe.Ingredients CONTAINS 'chocolate'",,true) into tQuery
return tQuery["resultarray"]

returns an array using field names on the 2nd index of all recipes with chocolate to the Controller.

Changing the core of revIgniter is not something to be taken lightly. But I believe that having, as an option, the ability for queries to be directly usable as an array indexed by field names would offer many benefits to revIgniter.

pull request coming from https://github.com/bhall2001/revigniter/tree/feature/_rigFetchAssoc if you want to give it a try. Not done yet. I need to address one technical item before the request is submitted. However, the code is working if you want to take it for a test run.

Bob

PS. all above code samples contain code that will/may not work.

PPS. I did move _rigFetchAssoc to DBresult.lc from the database drivers file. There is no dependencies of this function to any particular database but the code is replicated to each driver. My conclusion is this is redundant code so I've consolidated it to the DBresult.lc file.

revig commented 7 years ago

Moving _rigFetchAssoc to DBresult.lc is definitely something needed to be done and something I missed as yet. May be the handler name needs to be changed after doing modifications. Regarding the returned value being a numeric array, I remember vaguely that this had something to do with keeping table field names in order, in other words, to store the sequence of field names in the result array.

bhall2001 commented 7 years ago

Field name order with the numeric index is critical. This is a required for a number of reasons and the functionality can not be changed.

Accessing query results by field name one would not expect the field names to be in order. The data will be referenced by name. The order is arbitrary.

What I'm proposing is how Livecode demonstrates handling query results as arrays. Livecode's tutorial for converting a DB cursor to and array uses the command ConvertSQLCursorToArray. This command returns an array indexed by field name. This is a familiar recipe to most (if not all) programmers who work with query results in Livecode.

Bob

revig commented 7 years ago

As you mentioned the LiveCode tutorial “Converting a Database Cursor to a Data Grid Array”, this is the equivalent code using revIgniter:

put rigDbQuery("SELECT * FROM Table1") into tQuery
put tQuery["resultarray"] into tDataGridArray
queryResultArrayToDgArray tDataGridArray

# POPULATE DATAGRID
set the dgData of grp "DataGrid 1" to tDataGridArray

command queryResultArrayToDgArray @pArray
    local tKey, tRow, tName, tDataGridArray

    repeat for each key tKey in pArray
        put rigDbRow(tKey) into tRow
        repeat for each key tName in tRow
            put tRow[tName] into tDataGridArray[tKey][tName]
        end repeat
    end repeat

    put tDataGridArray into pArray
end queryResultArrayToDgArray

You get an array indexed by field names (as you propose) which can be used to populate a datagrid like in the LiveCode tutorial, the difference is you need to write much less code. (And if I have to choose between a recipe familiar to all programmers and a recipe which greatly reduces the amount of code needed to accomplish a task, guess what I prefer …)

bhall2001 commented 7 years ago

And this is the hook that I use on a regular basis. Getting the query results to a datagrid is not what I'm trying to accomplish (and can't be done on server that I'm aware of).

The issue with the solution you propose is that in order to obtain the data in an array indexed by fields, you have to iterate over the query results a 2nd time. For small data sets this is fine. But with larger datasets this is waisting resources. With a BLOCKING interpreter, waisting time unnecessarily looping over data structures takes valuable time.

The query results are already being looped on in _rigFetchAssoc. _rigFetchAssoc at one point I'll bet returned both indexed by number and indexed by name results based on the comment out code I see. Adding a simple condition to rigDbQuery returns the results without an additional loop and is 100% backwards compatible.

put rigDbQuery("SELECT * FROM Table1",,true) into tQuery
set the dgData of group "DataGrid 1" to tQuery["resultarray"]

14 lines of code to 2. Hmmm...THAT'S IT! NO CODING NEEDED.

Now let's look at a real revIgniter example. Suppose you'd like to return an array indexed by name from a Model to a Controller. You're proposed solution:

put rigDbQuery("SELECT * FROM Table1") into tQuery
put tQuery["resultarray"] into tResultArray
queryResultArrayToArray tResultArray

## Return the Field Indexed array
return tResultArray

command queryResultArrayToArray @pArray
    local tKey, tRow, tName, tDataA

    repeat for each key tKey in pArray
        put rigDbRow(tKey) into tRow
        repeat for each key tName in tRow
            put tRow[tName] into tDataA[tKey][tName]
        end repeat
    end repeat

    put tDataA into pArray
end queryResultArrayToArray

And my proposed solution:

put rigDbQuery("SELECT * FROM Table1",,true) into tQuery
return tQuery["resultarray"]

QED

Since you prefer less code, I'll assume you're going to love my pull request ;-)

Bob

revig commented 7 years ago

Getting the query results to a datagrid is not what I'm trying to accomplish (and can't be done on server that I'm aware of).

Think it is irrelevant what you do with the array returned, I just picked up an example you brought into play (and actually I do populate datagrids using revIgniter’s database libraries). Crucial is that the result is the same, an array indexed by field name.

The issue with the solution you propose is that in order to obtain the data in an array indexed by fields, you have to iterate over the query results a 2nd time.

I didn’t propose anything, I just showed how one would convert a query to an array indexed by field names using the tools revIgniter currently provides.

For small data sets this is fine. But with larger datasets this is waisting resources. With a BLOCKING interpreter, waisting time unnecessarily looping over data structures takes valuable time.

???

Adding a simple condition to rigDbQuery returns the results without an additional loop and is 100% backwards compatible.

OK, now to your proposal. I reckon that 100% backwards compatibility is not tenable. The issue is the name of the key of the query result array. This name is already globally used for a different array. For example what happens with rigDbRowArray(), rigDbLastRow(), rigDbSetResultFromCache etc.? Also this name may lead to confusion regarding various user guide instructions such as instructions about populating HTML tables. I think to make all this work backwards compatible _rigFetchAssoc would need to return always the numeric array as before and optionally an array like you propose IN ADDITION, using a different array key name. If you implemented your solution this way all should be fine.

Now let's look at a real revIgniter example.

My snippet was definitely real, see above. No need to change the “set the dgData” line.

Since you prefer less code, I'll assume you're going to love my pull request ;-)

Provided that all is implemented as I described above so that backwards compatibility is guaranteed, yes.

bhall2001 commented 7 years ago

rigDBFirstRow, rigDbLastRow, rigDbNextRow, rigDbPreviousRow, rigDbCurrentRow, rigDbResultValue - no change in behavior if the 2nd index is text. These functions only work on the first index of resultarray and that is not changing.

I would expect that if someone has query results by field name and they call rigDBNextRow (or any of the other functions above), they would want the next row to also be a row indexed by field names.

Now the ones that are more interesting.

rigDbRow/rigDbRowArray - If results are retrieved with field names, these functions do nothing and take a lot more effort to do it. As I understand them, they take a row of data by field numbers and convert it to a row of data by field names. Someone will call these functions though and yes, I do handle this case with no problem.

Caching - I need to understand more how this works. I have not looked at the caching side of the solution at all. What I need to look at is what happens when cache is on and someone runs a query with default settings, then the same query with new switch on and vise-versa. This is on my todo list.

rigFieldNumbers - I have not tested this function when the switch is thrown but I'll put it on my list. I'm not sure why someone would use this if they have asked for the results to be by field name. I'm inclined to set this to "" if the query results are by field name as the field numbers are meaningless.

If someone throws the switch to get query results with field names it is intended that the data will always be used by name. If a User needs the results by field number, they should query the default way to get the field numbers and convert the row data to field names if they need to with existing functions. This is specified in the User Guide entry as a warning similar to how other caveats are addressed in the User Guide.

I've searched for resultarray index in the database source files and to me it looks like the real issue is with rigDbRow, rigDbArray, rigDBResultFromCache. I've got 2 out of 3 covered. I'll look at the caching. I honestly don't see anything that is going to cause an issue with any existing code folks have and will make a many users happier by being able to access query results by field names directly and efficiently.

bhall2001 commented 7 years ago

I'm closing issue. I am using LC direct DB calls and my own library to handle DB results as associative arrays.